1698735 - [wpt-sync] Sync PR 28035 - Handle secondary edges in custom property cycles

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task, P4)

Description

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Sync web-platform-tests PR 28035 into mozilla-central (this bug is closed when the sync is complete).

PR: https://github.com/web-platform-tests/wpt/pull/28035
Details from upstream follow.

b'Anders Hartvoll Ruud <[e-mail]>' wrote:

Handle secondary edges in custom property cycles

The cycle detection in StyleCascade is not correct. Any secondary edges
coming from a cycle are also detected as in-cycle, even if they aren't.

Explanation of how the new cycle detection works:

• AutoLock is used to "lock" a property that's currently being resolved.
  • It's instantiated at the start of ResolveCustomProperty,
    ResolveVariableReference, and ResolvePendingSubstitution. No other
    places.
  • All "lock" means is that the property we're resolving is pushed to
    CascadeResolver.stack_.
• When we're trying to resolve a var(), a cycle is detected by looking
  for a given property in that stack. If the property is found, the
  stack index is set as the cycle_start_, and the current size of the
  stack is set as the cycle_end_. The idea is that all the properties
  in the range [cycle_start_, cycle_end_) are in a cycle, and are
  therefore invalid.
• As we come back up the regular call stack, we check the InCycle flag
  to see if a (nested) variable resolution has detected a cycle, and if
  we're still inside it.

Where the old code went wrong, was that we assumed that once a cycle was
detected, we would not push anything new onto CascadeResolver.stack_
until we were out of the cycle. Hence, once a cycle was detected,
anything reachable from the start of the cycle would be detected as
in-cycle.

This is regression from the StyleCascade project (since M82), although
nobody reported the issue.

Unfortunately this exposed that two tests relied on the buggy cycle
detection. These tests have been temporarily disabled/rebaselined.

Change-Id: If7bbaad084c001c4f3349a7a4050b0a5bd33139b
Fixed: 1187282
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2749195
Reviewed-by: Rune Lillesveen \<[e-mail]>
Reviewed-by: Xiaocheng Hu \<[e-mail]>
Commit-Queue: Anders Hartvoll Ruud \<[e-mail]>
Cr-Commit-Position: refs/heads/master@{#862369}

Comment 1

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Pushed to try (stability) https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2181bef37e148131458c530cb91c7240310d211

Comment 2

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

CI Results

Ran 15 Firefox configurations based on mozilla-central, and Firefox, Chrome, and Safari on GitHub CI

Total 1 tests and 11 subtests

Status Summary

Firefox

OK : 1
PASS: 11

Chrome

OK : 1
PASS: 6
FAIL: 5

Safari

OK : 1
PASS: 8
FAIL: 3

Links

Gecko CI (Treeherder)
GitHub PR Head
GitHub PR Base

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e597f6159b6
[wpt PR 28035] - Handle secondary edges in custom property cycles, a=testonly

Comment 4

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9e597f6159b6