Closed Bug 1403028 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
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)

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
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+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/40bfa0d02096
Ensure the parent is in the composed doc before marking it as dirty. r=heycam
https://hg.mozilla.org/integration/autoland/rev/48eb17111ec7
Crashtest. r=heycam
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.