Detect complex custom property cycles involving multiple loops

anttijk · anttijk · commit fc0d8fbdae06 · 2023-01-27T11:35:32.000-08:00
https://bugs.webkit.org/show_bug.cgi?id=251269 <rdar://problem/104744972> Reviewed by Simon Fraser. We fail in some complex cycle cases. * LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-cycles-expected.txt: * Source/WebCore/style/StyleBuilder.cpp: (WebCore::Style::Builder::applyCustomProperty): Track property cycles with a separate m_inCycleCustomProperties map instead of eagerly computing them to an invalid value on first detection. This allows the detection of multiple cycles passing through the same property. It also simplifies the code. * Source/WebCore/style/StyleBuilderState.h: Canonical link: https://commits.webkit.org/259506@main
diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-cycles-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-cycles-expected.txt
@@ -6,8 +6,8 @@ PASS Cycle that starts in the middle of a chain
 PASS Cycle with extra edge
 PASS Cycle with extra edge (2)
 PASS Cycle with extra edge (3)
-FAIL Cycle with secondary cycle assert_equals: --a expected "" but got "cycle"
-FAIL Cycle with overlapping secondary cycle assert_equals: --a expected "" but got "cycle"
-FAIL Cycle with deeper secondary cycle assert_equals: --c expected "" but got "cycle"
+PASS Cycle with secondary cycle
+PASS Cycle with overlapping secondary cycle
+PASS Cycle with deeper secondary cycle
 PASS Cycle via fallback
 
diff --git a/Source/WebCore/style/StyleBuilder.cpp b/Source/WebCore/style/StyleBuilder.cpp
@@ -174,11 +174,20 @@ void Builder::applyCustomProperty(const AtomString& name)
     if (!property.cssValue[SelectorChecker::MatchDefault])
         return;
 
-    SetForScope levelScope(m_state.m_currentProperty, &property);
-
     Ref customPropertyValue = downcast<CSSCustomPropertyValue>(*property.cssValue[SelectorChecker::MatchDefault]);
 
     bool inCycle = !m_state.m_inProgressCustomProperties.add(name).isNewEntry;
+    if (inCycle) {
+        auto isNewCycle = m_state.m_inCycleCustomProperties.add(name).isNewEntry;
+        if (isNewCycle) {
+            // Continue resolving dependencies so we detect cycles for them as well.
+            resolveCustomPropertyValueWithVariableReferences(customPropertyValue.get());
+        }
+        return;
+    }
+    
+    // There may be multiple cycles through the same property. Avoid interference from any previously detected cycles.
+    auto savedInCycleProperties = std::exchange(m_state.m_inCycleCustomProperties, { });
 
     auto createInvalidOrUnset = [&] {
         // https://drafts.csswg.org/css-variables-2/#invalid-variables
@@ -193,35 +202,18 @@ void Builder::applyCustomProperty(const AtomString& name)
         return CSSCustomPropertyValue::createWithID(name, CSSValueUnset);
     };
 
-    auto valueToApply = [&]() -> RefPtr<CSSCustomPropertyValue> {
-        if (inCycle)
-            return createInvalidOrUnset();
-
-        auto resolvedValue = resolveCustomPropertyValueWithVariableReferences(customPropertyValue.get());
-        if (m_state.m_appliedCustomProperties.contains(name))
-            return nullptr; // There was a cycle and the value was already resolved, so bail.
-
-        if (!resolvedValue)
-            return createInvalidOrUnset();
+    auto resolvedValue = resolveCustomPropertyValueWithVariableReferences(customPropertyValue.get());
 
-        return resolvedValue;
-    }();
-
-    if (!valueToApply) {
-        ASSERT(!m_state.m_inProgressCustomProperties.contains(name));
-        return;
-    }
+    if (!resolvedValue || m_state.m_inCycleCustomProperties.contains(name))
+        resolvedValue = createInvalidOrUnset();
 
+    SetForScope levelScope(m_state.m_currentProperty, &property);
     SetForScope scopedLinkMatchMutation(m_state.m_linkMatch, SelectorChecker::MatchDefault);
-    applyProperty(CSSPropertyCustom, *valueToApply, SelectorChecker::MatchDefault);
+    applyProperty(CSSPropertyCustom, *resolvedValue, SelectorChecker::MatchDefault);
 
     m_state.m_inProgressCustomProperties.remove(name);
     m_state.m_appliedCustomProperties.add(name);
-
-    if (inCycle) {
-        // Resolve this value so that we reset its dependencies.
-        resolveCustomPropertyValueWithVariableReferences(customPropertyValue.get());
-    }
+    m_state.m_inCycleCustomProperties.formUnion(WTFMove(savedInCycleProperties));
 }
 
 inline void Builder::applyCascadeProperty(const PropertyCascade::Property& property)
diff --git a/Source/WebCore/style/StyleBuilderState.h b/Source/WebCore/style/StyleBuilderState.h
@@ -134,6 +134,7 @@ class BuilderState {
 
     HashSet<String> m_appliedCustomProperties;
     HashSet<String> m_inProgressCustomProperties;
+    HashSet<String> m_inCycleCustomProperties;
     Bitmap<numCSSProperties> m_inProgressProperties;
     Bitmap<numCSSProperties> m_inUnitCycleProperties;
 
