Closed Bug 1707116 Opened 5 months ago Closed 5 months ago

Replacing shadow DOM style results in inconsistent computed style

Categories

(Core :: CSS Parsing and Computation, defect)

Firefox 88
defect

Tracking

()

VERIFIED FIXED
90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox88 --- wontfix
firefox89 --- verified
firefox90 --- verified

People

(Reporter: nlawson, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

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.

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.

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

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 :).

Status: UNCONFIRMED → NEW
Component: DOM: Core & HTML → CSS Parsing and Computation
Ever confirmed: true
Flags: needinfo?(emilio)

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.

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

Assignee: nobody → emilio
Flags: needinfo?(emilio)

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 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
Attachment #9217936 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Attached file Reduced test-case.

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

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9f94f9b8e3cc
Make sure to consume the flusher when we hit the cascade data cache. r=boris
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28725 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
QA Whiteboard: [qa-triaged]
Upstream PR merged by moz-wptsync-bot

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.

Regressed by: 1696447

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.

Attachment #9217936 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.