[diptomistry]

Summary

Fixes #2052.

Replacing a non-integer size with max-content, fit-content, or stretch could make the getter return a different keyword. YGNodeStyleSetHeight(node, 0.5f), then YGNodeStyleSetWidth(node, 0.25f), then YGNodeStyleSetWidthMaxContent(node) made YGNodeStyleGetWidth return YGUnitFitContent instead of YGUnitMaxContent.

A non-integer size is stored in the value pool, and the handle keeps that pool index in its value bits. storeKeyword changed the type to Keyword but left the indexed bit set, so isKeyword compared the pool index with the keyword enum (MaxContent = 0, FitContent = 1, Stretch = 2). The same mismatch affects height, min and max size, and flex-basis. A pool index of 3 or more matches no keyword and hits the debug assert in getSize.

auto, undefined, margin, and padding read the type bits only, so those getters were already correct. storeKeyword still writes the keyword into the handle's existing pool slot. getSize now reads that slot when the indexed bit is set, so the handle keeps the slot and the getter returns the stored keyword.

Test plan

• YogaTest.width_keyword_replaces_pooled_number covers the Wrong style value retrieved from style that was changed from a number to a keyword. #2052 sequence.
• StyleValuePool.store_keywords_after_large_int checks max-content, fit-content, and stretch in pool slots 0 through 3.
• StyleValuePool.store_large_int_and_keyword_repeatedly switches a pooled number and max-content 5000 times and checks that the slot is reused.
• ./unit_tests Debug: 880 passed, 0 failed. Baseline before these tests was 877 passed, 0 failed.

Changelog:
[General][Fixed] - Fix reading back max-content, fit-content, and stretch after a non-integer size was stored

[intergalacticspacehighway]

Thanks for the quick fix and the clear root-cause write-up. The analysis is correct: isKeyword() compares the pool index with the keyword enum.

I'd like to fix it on the read side instead, because clearing the indexed bit makes the handle forget its pool slot. SmallValueBuffer has no way to free a slot, so the design relies on a handle reusing its slot: storeValue() and storeKeyword() both write into the existing slot when the indexed bit is set. With this PR, every switch between a keyword and a non-integer value pushes a new slot:

StyleValuePool pool;
StyleValueHandle handle;
for (int i = 0; i < 5000; i++) {
  pool.store(handle, StyleSizeLength::points(262144));
  pool.store(handle, StyleSizeLength::ofMaxContent());
}

On main this stays at one slot. With this PR, a Debug build stops with:

Assertion failed: (index < 4096 && "SmallValueBuffer can only hold up to 4096 chunks"), function push, file SmallValueBuffer.h, line 34.

storeKeyword() already writes the keyword into the slot. Only the reader is missing. Could you revert the storeKeyword() change and check the keyword through the pool instead? The patch below is against main:

Patch

diff --git a/yoga/style/StyleValueHandle.h b/yoga/style/StyleValueHandle.h
--- a/yoga/style/StyleValueHandle.h
+++ b/yoga/style/StyleValueHandle.h
@@ -76,10 +76,6 @@ class StyleValueHandle {
   // Intentionally leaving out auto as a fast path
   enum class Keyword : uint8_t { MaxContent, FitContent, Stretch };
 
-  constexpr bool isKeyword(Keyword keyword) const {
-    return type() == Type::Keyword && value() == static_cast<uint16_t>(keyword);
-  }
-
   constexpr Type type() const {
     return static_cast<Type>(repr_ & kHandleTypeMask);
   }
diff --git a/yoga/style/StyleValuePool.h b/yoga/style/StyleValuePool.h
--- a/yoga/style/StyleValuePool.h
+++ b/yoga/style/StyleValuePool.h
@@ -89,11 +89,11 @@ class StyleValuePool {
       return StyleSizeLength::undefined();
     } else if (handle.isAuto()) {
       return StyleSizeLength::ofAuto();
-    } else if (handle.isKeyword(StyleValueHandle::Keyword::MaxContent)) {
+    } else if (isKeyword(handle, StyleValueHandle::Keyword::MaxContent)) {
       return StyleSizeLength::ofMaxContent();
-    } else if (handle.isKeyword(StyleValueHandle::Keyword::FitContent)) {
+    } else if (isKeyword(handle, StyleValueHandle::Keyword::FitContent)) {
       return StyleSizeLength::ofFitContent();
-    } else if (handle.isKeyword(StyleValueHandle::Keyword::Stretch)) {
+    } else if (isKeyword(handle, StyleValueHandle::Keyword::Stretch)) {
       return StyleSizeLength::ofStretch();
     } else {
       assert(
@@ -132,6 +132,17 @@ class StyleValuePool {
   }
 
  private:
+  bool isKeyword(StyleValueHandle handle, StyleValueHandle::Keyword keyword)
+      const {
+    if (handle.type() != StyleValueHandle::Type::Keyword) {
+      return false;
+    }
+    // storeKeyword() keeps an existing pool slot, so the keyword may be there
+    uint32_t value = handle.isValueIndexed() ? buffer_.get32(handle.value())
+                                             : handle.value();
+    return value == static_cast<uint32_t>(keyword);
+  }
+
   void storeValue(
       StyleValueHandle& handle,
       float value,
diff --git a/tests/StyleValuePoolTest.cpp b/tests/StyleValuePoolTest.cpp
--- a/tests/StyleValuePoolTest.cpp
+++ b/tests/StyleValuePoolTest.cpp
@@ -143,4 +143,38 @@ TEST(StyleValuePool, store_keywords) {
   EXPECT_EQ(pool.getSize(handleStretch), StyleSizeLength::ofStretch());
 }
 
+TEST(StyleValuePool, store_keywords_after_large_int) {
+  StyleValuePool pool;
+  std::array<StyleValueHandle, 4> handles;
+
+  // Each handle takes its own pool slot, from 0 to 3
+  for (auto& handle : handles) {
+    pool.store(handle, StyleSizeLength::points(262144));
+  }
+
+  for (auto keyword :
+       {StyleSizeLength::ofMaxContent(),
+        StyleSizeLength::ofFitContent(),
+        StyleSizeLength::ofStretch()}) {
+    for (auto& handle : handles) {
+      pool.store(handle, keyword);
+      EXPECT_EQ(pool.getSize(handle), keyword);
+    }
+  }
+}
+
+TEST(StyleValuePool, store_large_int_and_keyword_repeatedly) {
+  StyleValuePool pool;
+  StyleValueHandle handle;
+
+  // The handle keeps its pool slot, so the pool must not grow on each store
+  for (int i = 0; i < 5000; i++) {
+    pool.store(handle, StyleSizeLength::points(262144));
+    pool.store(handle, StyleSizeLength::ofMaxContent());
+  }
+  pool.store(handle, StyleSizeLength::points(262144));
+
+  EXPECT_EQ(pool.getSize(handle), StyleSizeLength::points(262144));
+}
+
 } // namespace facebook::yoga

With this patch and your tests, all unit tests pass in Debug.

On the tests: all size properties go through the same getSize(), so the per-property tests cover one code path. Could you keep width_keyword_replaces_pooled_number as the public API repro for #2052 and replace the others with the two pool tests above? They cover each keyword in slots 0 to 3, and the slot reuse.

[diptomistry]

Updated in 0ccbfe1. storeKeyword keeps the existing pool slot, and getSize reads the keyword from that slot. I kept width_keyword_replaces_pooled_number and replaced the other public tests with the two StyleValuePool tests.

[intergalacticspacehighway]

Thanks, the code and tests look good. Could you update the title and description to match the read-side fix and the current tests?