Closed Bug 1403028 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: aElement->IsInComposedDoc() [@ NoteDirtyElement]

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase)

Attachments

(4 files, 2 obsolete files)

test_case.html
498 bytes, text/html
Details
More reduced testcase
226 bytes, text/html
Details
0001-Bug-1403028-Ensure-the-parent-is-in-the-composed-doc.patch
1.46 KB, patch
heycam
: review+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
0002-Bug-1403028-Crashtest.-r-heycam.patch
1.23 KB, patch
heycam
: review+
Details | Diff | Splinter Review
Attached file test_case.html (obsolete) —
Interesting: Assertion failure: aElement->IsInComposedDoc(), at /builds/worker/workspace/build/src/dom/base/Element.cpp:4353 NoteDirtyElement|hg:hg.mozilla.org/mozilla-central:dom/base/Element.cpp:33b7b8e81b4b|4366|0x0 mozilla::ServoRestyleManager::SnapshotFor|hg:hg.mozilla.org/mozilla-central:layout/base/ServoRestyleManager.cpp:33b7b8e81b4b|1056|0x8 mozilla::ServoRestyleManager::ContentStateChanged|hg:hg.mozilla.org/mozilla-central:layout/base/ServoRestyleManager.cpp:33b7b8e81b4b|1252|0xb mozilla::PresShell::ContentStateChanged|hg:hg.mozilla.org/mozilla-central:layout/base/RestyleManagerInlines.h:33b7b8e81b4b|51|0x15 nsDocument::ContentStateChanged|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:33b7b8e81b4b|5666|0x1e mozilla::dom::Element::UpdateState|hg:hg.mozilla.org/mozilla-central:dom/base/Element.cpp:33b7b8e81b4b|272|0x9 nsDocument::RefreshLinkHrefs|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:33b7b8e81b4b|9569|0x5 nsDocument::SetBaseURI|hg:hg.mozilla.org/mozilla-central:dom/base/nsDocument.cpp:33b7b8e81b4b|3880|0x8 mozilla::dom::SetBaseURIUsingFirstBaseWithHref|hg:hg.mozilla.org/mozilla-central:dom/html/HTMLSharedElement.cpp:33b7b8e81b4b|201|0xf mozilla::dom::HTMLSharedElement::UnbindFromTree|hg:hg.mozilla.org/mozilla-central:dom/html/HTMLSharedElement.cpp:33b7b8e81b4b|294|0xa mozilla::dom::Element::UnbindFromTree|hg:hg.mozilla.org/mozilla-central:dom/base/Element.cpp:33b7b8e81b4b|1992|0x18 nsGenericHTMLElement::UnbindFromTree|hg:hg.mozilla.org/mozilla-central:dom/html/nsGenericHTMLElement.cpp:33b7b8e81b4b|537|0x10 nsINode::doRemoveChildAt|hg:hg.mozilla.org/mozilla-central:dom/base/nsINode.cpp:33b7b8e81b4b|1945|0x16 mozilla::dom::FragmentOrElement::RemoveChildAt|hg:hg.mozilla.org/mozilla-central:dom/base/FragmentOrElement.cpp:33b7b8e81b4b|1372|0x12 nsINode::ReplaceOrInsertBefore|hg:hg.mozilla.org/mozilla-central:dom/base/nsINode.cpp:33b7b8e81b4b|2432|0x17 mozilla::dom::NodeBinding::replaceChild|hg:hg.mozilla.org/mozilla-central:dom/base/nsINode.h:33b7b8e81b4b|1849|0x1a mozilla::dom::GenericBindingMethod|hg:hg.mozilla.org/mozilla-central:dom/bindings/BindingUtils.cpp:33b7b8e81b4b|3055|0x9 js::CallJSNative|hg:hg.mozilla.org/mozilla-central:js/src/jscntxtinlines.h:33b7b8e81b4b|293|0x6 js::InternalCallOrConstruct|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|495|0xb InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|540|0xd Interpret|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|546|0xf js::RunScript|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|435|0xb js::InternalCallOrConstruct|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|513|0xb InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|540|0xd js::Call|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:33b7b8e81b4b|559|0x5
Flags: in-testsuite?
INFO: Last good revision: 08e7d627c2017392af5ba26086e682a61cbc88dd INFO: First bad revision: 7f9883dd37feac26fb95b629ad1010107f04603c INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=08e7d627c2017392af5ba26086e682a61cbc88dd&tochange=7f9883dd37feac26fb95b629ad1010107f04603c
Blocks: 1400936
Has Regression Range: --- → yes
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(emilio)
Summary: Assertion failure: aElement->IsInComposedDoc() [@ NoteDirtyElement] → stylo: Assertion failure: aElement->IsInComposedDoc() [@ NoteDirtyElement]
Interesting, related to bug 1402684?
Assignee: nobody → emilio
Priority: -- → P2
Attached file test_case.html (obsolete) —
This test case also triggers a crash.
Attached file test_case.html —
Attachment #8912066 - Attachment is obsolete: true
Attachment #8912072 - Attachment is obsolete: true
Severity: normal → critical
Keywords: crash
Looking, this is hopefully just a dupe of bug 1402684, or this problem was being wallpapered somehow before https://hg.mozilla.org/integration/autoland/rev/9ef85da5aea4
Attached file More reduced testcase —
(While I wait for a build, I may as well tidy it and reduce it). This smells a lot like what would've been wallpapered by the patch above. We would've cleared style data early in the whole <body>, which covered this bug.
Attachment #8912093 - Attachment mime type: text/plain → text/html
Flags: needinfo?(emilio)
Well, so, cool part is that this is indeed a wallpaper that https://hg.mozilla.org/integration/autoland/rev/9ef85da5aea4 uncovers. That means that a patch like: diff --git a/dom/base/FragmentOrElement.cpp b/dom/base/FragmentOrElement.cpp index 41b8066b3ec3..b0851fee1188 100644 --- a/dom/base/FragmentOrElement.cpp +++ b/dom/base/FragmentOrElement.cpp @@ -1330,7 +1330,7 @@ FragmentOrElement::SetXBLInsertionParent(nsIContent* aContent) // We just changed the flattened tree, so any Servo style data is now invalid. // We rely on nsXBLService::LoadBindings to re-traverse the subtree afterwards. - if (oldInsertionParent != aContent && + if (// oldInsertionParent != aContent && IsStyledByServo() && IsElement() && AsElement()->HasServoData()) { ServoRestyleManager::ClearServoDataFromSubtree(AsElement()); } Will fix all these crashes. However, that still feels pretty wallpaper-ish to me. Perhaps we should just do that for now. Also cool is that we now have a pretty nice test-case for this, which is super nice. Worse part is that I still don't have a particularly great idea of how to fix this properly and I need to think a bit more through it. The problem is this bit: http://searchfox.org/mozilla-central/source/layout/base/ServoRestyleManager.cpp#1055 `parent` has been already unbound from the tree, even though aElement hasn't. We _can_ special-case this just checking |parent->IsInComposedDoc()|, I guess, but that's not particularly great...
Bobby, any opinion on the above before you go to sleep?
Flags: needinfo?(bobbyholley)
I guess another option would be to ditch that code and teach the traversal code how to invalidate siblings of the root...
I discussed this with cam, we're going to land an easy fix that adds a check to avoid the crash, and do the proper (but more risky) fix in another bug.
Flags: needinfo?(bobbyholley)
See Also: → 1403078
Mozreview is down... shrug.
Attachment #8912126 - Flags: review?(cam)
Attachment #8912126 - Flags: review?(cam) → review-
Attachment #8912126 - Flags: review- → review+
Attachment #8912127 - Flags: review?(cam) → review+
Please request uplift on this when you get a chance.
Flags: needinfo?(emilio)
Comment on attachment 8912126 [details] [diff] [review] 0001-Bug-1403028-Ensure-the-parent-is-in-the-composed-doc.patch Approval Request Comment [Feature/Bug causing the regression]: bug 1400936 (though the bug is pre-existing, really) [User impact if declined]: Crashes [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: nope [Why is the change risky/not risky?]: Just an extra condition that prevents the crash and doesn't affect correctness. Proper fix is bug 1403078, but it's not needed for correctness. [String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8912126 - Flags: approval-mozilla-beta?
Comment on attachment 8912126 [details] [diff] [review] 0001-Bug-1403028-Ensure-the-parent-is-in-the-composed-doc.patch Fix an assert, taking it. Should be in 57b4
Attachment #8912126 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: