1707116 - Replacing shadow DOM style results in inconsistent computed style

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nolan Lawson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.85 Safari/537.36

Steps to reproduce:

1. Go to http://bl.ocks.org/nolanlawson/raw/a713fe937c34302df55b65175a7658cc/
2. Refresh the page a few times (it's inconsistent, but it should only take ~5 tries max)

Actual results:

The test will occasionally print out:

margin-left: 10px
margin-right: 0px
margin-left: 10px
margin-right: 10px

In this case, the element with the text "B" will have margin-left 10px and margin-right 10px.

Expected results:

In Chrome and Safari, the result is consistently:

margin-left: 10px
margin-right: 0px
margin-left: 0px
margin-right: 10px

And the text "B" has a margin-left of 0px.

This appears to be a race condition. My team noticed it in our integration tests staring in Firefox 88 running on Windows. On my macOS machine, I can repro in both Firefox 88 and Firefox Nightly 90.0a1 (2021-04-22).

The test is essentially removing a <style> inside of a shadow DOM tree and replacing it with another <style>. The style changes from:

div {
    margin-left: 10px;
}

to

div {
    margin-right: 10px;
}

For some reason, occasionally, Firefox will end up with the computed style margin-left: 10px and margin-right: 10px, even though the first style was removed.

The test prints out the margin-left/margin-right both before and after the <style> is changed.

Comment 1

[Nolan Lawson]

Attachment: Screen Shot 2021-04-22 at 2.45.49 PM.png

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Hmm... If it started with 88, I suspect bug 1696447. It'd be extra-nice to have a simpler test-case, since those scripts are massive, but I'll give a shot at reducing it myself anyways :).

Comment 4

[Nolan Lawson]

I took another look at it, and I think I have a truly reduced repro: http://bl.ocks.org/nolanlawson/raw/9eb92053f3df5cfa84e319027b87ab4f/

Your comment about style sharing gave me the hint I needed. 🙂 It may have something to do with the two different components sharing the exact same CSS in their shadow roots. If the strings are even slightly different (e.g. whitespace) then the bug doesn't seem to repro.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yeah, that makes sense, I see what's going on, thanks for the reduced test-case!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1707116 - Make sure to consume the flusher when we hit the cascade data cache. r=boris,#style,#layout-reviewers

So that sheets are properly marked as committed. The issue is that we
have this "committed" optimization, to avoid doing work if somebody adds
and removes an stylesheet without us flushing in the meantime:

https://searchfox.org/mozilla-central/rev/f018480dfed4fc583703a5770a6db9ab9dc0fb99/servo/components/style/stylesheet_set.rs#308-319

The "committed" bit is set when we consume the flusher (in each(..)).

However when we hit the cache, before this patch, we wouldn't consume
it, which means that we may fail to do some full rebuilds even though we
need them.

Fix it by making sure we call each() in that case. That's really a
one-liner, the rest of the patch is adding assertions that would've
caught this, and tweaking the user-agent cascade data to uphold these
assertions (though note that given the UA cascade data doesn't do
partial rebuilds, it's not really fixing a correctness issue).

Also, of course, we add a test (the test would show a red square before
this patch, and a lime square with the fix).

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9217936 [details]
Bug 1707116 - Make sure to consume the flusher when we hit the cascade data cache. r=boris,#style,#layout-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Wrong styling in some edge cases involving Shadow DOM.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0, or I can prepare an even more reduced test-case.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fix is super-simple (one liner, plus some debug asserts to prevent this error in the future).
• String changes made/needed: none

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

Here's the reduced test-case I came up with, which consistently shows red instead of lime :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9f94f9b8e3cc
Make sure to consume the flusher when we hit the cascade data cache. r=boris

Comment 10

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

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28725 for changes under testing/web-platform/tests

Comment 11

[Iulian Moraru]

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

Comment 12

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

Upstream PR merged by moz-wptsync-bot

Comment 13

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce this issue on Firefox 89.0b5, under Windows 10x64.
The issue is no longer reproducible on Firefox 90.0a1 (2021-04-29).
Tests were performed under Windows 10x64, Ubuntu 20.04x64 and under macOS 10.15.

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9217936 [details]
Bug 1707116 - Make sure to consume the flusher when we hit the cascade data cache. r=boris,#style,#layout-reviewers

Low risk simple patch with tests fixing an 88 regression, on nightly for 9 days, approved for 89 beta 10, thanks.

Comment 15

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/420c61e6b1e8

Comment 16

[Mihai Boldan, Desktop QA [:mboldan]]

I confirm that the issue is no longer reproducible on Firefox 89.0b10.
Tests were performed under Windows 10x64, Ubuntu20.04 x64 and under macOS 10.15.