Closed Bug 1511138 Opened 11 months ago Closed 10 months ago

crash near null in [@ collect_normal_rules_from_containing_shadow_tree]

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(6 files)

Attached file testcase.html
==69600==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x7f6776e2a5d6 bp 0x000000000000 sp 0x7fff65042010 T0)
==69600==The signal is caused by a READ memory access.
==69600==Hint: address points to the zero page.
    #0 0x7f6776e2a5d5 in _$LT$style..rule_collector..RuleCollector$LT$$u27$a$C$$u20$$u27$b$C$$u20$E$C$$u20$F$GT$$GT$::collect_normal_rules_from_containing_shadow_tree::hc6465809d2630c8f src/servo/components/style/rule_collector.rs:222:16
    #1 0x7f6776e2a5d5 in _$LT$style..rule_collector..RuleCollector$LT$$u27$a$C$$u20$$u27$b$C$$u20$E$C$$u20$F$GT$$GT$::collect_all::ha1550e657e348e26 src/servo/components/style/rule_collector.rs:386
    #2 0x7f6776e2a5d5 in style::stylist::Stylist::push_applicable_declarations::h66da088a9d41b41b src/servo/components/style/stylist.rs:1125
    #3 0x7f6776e2a5d5 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::match_primary::h0c2e888ee2041b0a src/servo/components/style/style_resolver.rs:435
    #4 0x7f6776edf64d in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_primary_style::hf1e863802eba5fa6 src/servo/components/style/style_resolver.rs:162:30
    #5 0x7f6776edf64d in style::traversal::resolve_style::h2564578a9b75e517 src/servo/components/style/traversal.rs:359
    #6 0x7f6776edf64d in Servo_ResolveStyleLazily src/servo/ports/geckolib/glue.rs:4931
    #7 0x7f67706384d0 in mozilla::ServoStyleSet::ResolveStyleLazilyInternal(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::StyleRuleInclusion) src/layout/style/ServoStyleSet.cpp:1395:5
    #8 0x7f677069d0ba in nsComputedDOMStyle::UpdateCurrentStyleSources(bool) src/layout/style/nsComputedDOMStyle.cpp:981:7
    #9 0x7f677069b662 in nsComputedDOMStyle::GetPropertyValue(nsTSubstring<char16_t> const&, nsTSubstring<char16_t>&) src/layout/style/nsComputedDOMStyle.cpp:450:3
    #10 0x7f676ba29528 in GetPropertyValue src/layout/style/nsICSSDeclaration.h:91:10
    #11 0x7f676ba29528 in mozilla::dom::CSSStyleDeclaration_Binding::getPropertyValue(JSContext*, JS::Handle<JSObject*>, nsICSSDeclaration*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/CSSStyleDeclarationBinding.cpp:289
    #12 0x7f676d3347b4 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3317:13
    #13 0x2f35387b5ebf  (<unknown module>)
Flags: in-testsuite?
Flags: needinfo?(emilio)
Huh, this is a really recent regression, and I still don't know how it can happen... I can't repro it with a build from yesterday, has anything suspicious landed today?
https://drafts.csswg.org/cssom/#dom-window-getcomputedstyle says:

> If elt is connected, part of the flat tree, and its shadow-including root...

WebKit and Blink already do this, and we do it already except for cross-document
situations, where we can end up with a PresShell even if GetPresShellForContent
returns null.

The style system should be able to rely on ShadowRoots having a non-null shadow
host.
Blocks: 1508000
Flags: needinfo?(emilio)
Assignee: nobody → emilio
This only fixes the style crash, but not the root of the problem. Let's use bug 1511130 for that.
See D13472 for spec quotes and such. Other browsers don't allow
getting computed styles in disconnected subtrees and we agreed to follow suit
(it does make sense because when you're not on the flat tree it's not defined
what you're supposed to inherit from, specially in presence of Shadow DOM).

Also, it allows the style system to rely on the DOM being in a sane state.
This probably deserves a comment as of why it belongs to this bug.

This patch series caused a single, reproducible timeout on
browser_ext_themes_toolbars.js, where the transitionend event it awaits for
stops triggering.

I got fascinated by it and I decided to poke around it in rr instead of just
removing the await line, and here's what's going on.

In the previous implementation of _sanitizeCSSColor, we were not flushing style
because of the optimization bug 1363805 introduced (which wasn't supposed to
deal with out-of-document elements, but it accidentally did so).

In any case, the fact that we were not flushing style in _sanitizeCSSColor
caused us to flush style sometime later when the lwtheme attribute was already
set up, and thus the selector in here matched:

  https://searchfox.org/mozilla-central/rev/cfaa5a1d48d6bc6552199e73004ecb05d0a9c921/browser/themes/shared/browser.inc.css#40

And thus caused the transition rule to apply at a time where the
background-color change happened.

Now we were flushing on getComputedStyle on every call, and in the most
inefficient way possible (changing a custom property on the root before each
property change, which causes us to restyle the whole document to propagate it
down to all descendants).

Furthermore, we were flushing style at a time where the lwtheme attribute
change had not yet happened, and thus when the background-color changed, there
was no transition rule applicable, and the transition didn't fire.

This patch changes LightweightThemeConsumer to avoid restyling the whole
document over and over.

Also, while at it I realized that you could fool the sanitizer with !important
in an experiment stylesheet or with other !important rule in the page really.
It's not clear why you'd do that, but it may be worth to just making that
function completely sound, so I did that and added a test for it.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e601a2fbd077
Never return styles for disconnected elements. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2d3e7a822b5
Fix / update tests. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e03afbff55f3
Fix LightweightThemeConsumer's use of getComputedStyle. r=jaws,mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5d1c50a342
Improve performance of LightweightThemeConsumer when setting properties, and also avoid _sanitizeCSSColor from getting fooled. r=jaws
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14401 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e99e6399b9
Fix lint and OSX-only test failure on a CLOSED TREE.
Backed out for browser chrome failures on browser_selectpopup_colors.js. 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=browser%2Cchrome&fromchange=5f5d1c50a34213414f3f6a1f05b575e4f2176daf&tochange=90189bd84466d74e64d4eb2a33138152e84b241c&selectedJob=215755953

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=215767528&repo=mozilla-inbound&lineNumber=1940

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/90189bd84466d74e64d4eb2a33138152e84b241c

16:53:05     INFO - TEST-PASS | browser/base/content/test/forms/browser_selectpopup_colors.js | Item 3 has correct background color - 
16:53:05     INFO - Buffered messages finished
16:53:05     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/forms/browser_selectpopup_colors.js | Item 4 should not have any custom option styling - 
16:53:05     INFO - Stack trace:
16:53:05     INFO - chrome://mochikit/content/browser-test.js:test_ok:1292
16:53:05     INFO - chrome://mochitests/content/browser/browser/base/content/test/forms/browser_selectpopup_colors.js:testOptionColors:213
16:53:05     INFO - chrome://mochitests/content/browser/browser/base/content/test/forms/browser_selectpopup_colors.js:testSelectColors:290
16:53:05     INFO - chrome://mochitests/content/browser/browser/base/content/test/forms/browser_selectpopup_colors.js:test_colors_applied_to_popup_items:312
16:53:05     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1093
16:53:05     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1084
16:53:05     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:982
16:53:05     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
16:53:05     INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(emilio)
Upstream PR was closed without merging
I missed the failure in browser_selectpopup_colors.js since it doesn't run on
Linux. Fix the getComputedStyle usage in that code by using
getDefaultComputedStyle, which is what it really wants.

Also, do a bit of cleanup while at it: uaBackgroundColor was unused, and uaColor
was wrong (we don't override the ua color of the <option> element, it just
inherits, so it's the same as the <select> color, and that's what we were
comparing it against anyway).
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4df286b234b3
Never return styles for disconnected elements. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef293b90887
Fix / update tests. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99600391704
Fix LightweightThemeConsumer's use of getComputedStyle. r=jaws,mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/d23c9c3e1566
Improve performance of LightweightThemeConsumer when setting properties, and also avoid _sanitizeCSSColor from getting fooled. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/daee82295b3c
Fix getComputedStyle usage of SelectChild. r=jaws,mconley
Err, last commit shouldn't have landed with r=jaws, whoops... Anyway, he said on IRC he was fine as long as that was pointed out in the bug.
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7159c6b7e745
Never return styles for disconnected elements. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1879534289a4
Fix / update tests. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5967565fc6
Fix LightweightThemeConsumer's use of getComputedStyle. r=jaws,mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a2ea1539b8
Improve performance of LightweightThemeConsumer when setting properties, and also avoid _sanitizeCSSColor from getting fooled. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/f228599d5992
Fix getComputedStyle usage of SelectChild. r=mconley
Upstream PR was closed without merging
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged
Flags: in-testsuite? → in-testsuite+
Depends on: 1533392
You need to log in before you can comment on or make changes to this bug.