Closed
Bug 1403028
Opened 7 years ago
Closed 7 years ago
stylo: Assertion failure: aElement->IsInComposedDoc() [@ NoteDirtyElement]
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
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)
498 bytes,
text/html
|
Details | |
226 bytes,
text/html
|
Details | |
1.46 KB,
patch
|
heycam
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•7 years ago
|
||
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]
Comment 2•7 years ago
|
||
Interesting, related to bug 1402684?
Assignee: nobody → emilio
Priority: -- → P2
Reporter | ||
Comment 3•7 years ago
|
||
This test case also triggers a crash.
Reporter | ||
Comment 4•7 years ago
|
||
Attachment #8912066 -
Attachment is obsolete: true
Attachment #8912072 -
Attachment is obsolete: true
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Attachment #8912093 -
Attachment mime type: text/plain → text/html
Flags: needinfo?(emilio)
Assignee | ||
Comment 7•7 years ago
|
||
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...
Assignee | ||
Comment 8•7 years ago
|
||
Bobby, any opinion on the above before you go to sleep?
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 9•7 years ago
|
||
I guess another option would be to ditch that code and teach the traversal code how to invalidate siblings of the root...
Comment 10•7 years ago
|
||
Also seen in bughunter on Windows, Linux Beta/57, Nightly/58: https://developers.google.com/youtube/v3/docs/search/list https://developers.google.com/youtube/v3/docs/search/list#try-it https://4it.me/getlistip?cityid=9750 (This actually reproduced bug 1401224 locally once)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
Mozreview is down... shrug.
Attachment #8912126 -
Flags: review?(cam)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8912127 -
Flags: review?(cam)
Updated•7 years ago
|
Attachment #8912126 -
Flags: review?(cam) → review-
Updated•7 years ago
|
Attachment #8912126 -
Flags: review- → review+
Updated•7 years ago
|
Attachment #8912127 -
Flags: review?(cam) → review+
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40bfa0d02096 https://hg.mozilla.org/mozilla-central/rev/48eb17111ec7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 16•7 years ago
|
||
Please request uplift on this when you get a chance.
Flags: needinfo?(emilio)
Assignee | ||
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
Comment 19•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c363a41fc899 https://hg.mozilla.org/releases/mozilla-beta/rev/f8eb0cd96943
Flags: in-testsuite? → in-testsuite+
Comment 20•7 years ago
|
||
(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-
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•