Closed Bug 1400936 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot)

Categories

(Core :: CSS Parsing and Computation, 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, testcase)

Attachments

(5 files)

Attached file test_case.html
Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot), at /builds/worker/workspace/build/src/dom/base/nsIDocumentInlines.h:71

Found with m-c 
Changeset: afdce00aacb4c053f783016ca00c8f649a50b8c0
Build ID: 20170914185408

#0 0x7fd1356f50b0 in nsIDocument::SetServoRestyleRoot(nsINode*, unsigned int) /src/dom/base/nsIDocumentInlines.h:69:3
#1 0x7fd1356cbbd5 in NoteDirtyElement(mozilla::dom::Element*, unsigned int) /src/dom/base/Element.cpp:4434:12
#2 0x7fd138e0b45d in mozilla::ServoRestyleManager::SnapshotFor(mozilla::dom::Element*) /src/layout/base/ServoRestyleManager.cpp:1059:15
#3 0x7fd138e0c6fc in mozilla::ServoRestyleManager::ContentStateChanged(nsIContent*, mozilla::EventStates) /src/layout/base/ServoRestyleManager.cpp:1252:36
#4 0x7fd138dd8aae in mozilla::PresShell::ContentStateChanged(nsIDocument*, nsIContent*, mozilla::EventStates) /src/layout/base/PresShell.cpp:4279:37
#5 0x7fd13585a2fd in nsDocument::ContentStateChanged(nsIContent*, mozilla::EventStates) /src/dom/base/nsDocument.cpp:5664:3
#6 0x7fd1356b12ab in mozilla::dom::Element::UpdateState(bool) /src/dom/base/Element.cpp:272:14
#7 0x7fd1356aadfa in mozilla::dom::Element::SetDirectionality(mozilla::Directionality, bool) /src/obj-firefox/dist/include/mozilla/dom/Element.h:463:7
#8 0x7fd1356ab195 in mozilla::WalkDescendantsSetDirectionFromText(mozilla::dom::Element*, bool, nsINode*) /src/dom/base/DirectionalityUtils.cpp:417:13
#9 0x7fd1356f1ebf in mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection(nsPtrHashKey<mozilla::dom::Element>*, void*) /src/dom/base/DirectionalityUtils.cpp:540:21
#10 0x7fd1356f1591 in nsCheapSet<nsPtrHashKey<mozilla::dom::Element> >::EnumerateEntries(nsCheapSetOperator (*)(nsPtrHashKey<mozilla::dom::Element>*, void*), void*) /src/xpcom/ds/nsCheapSets.h:79:13
#11 0x7fd1356f1d5f in mozilla::nsTextNodeDirectionalityMap::ResetAutoDirection(nsINode*) /src/dom/base/DirectionalityUtils.cpp:578:15
#12 0x7fd1356ab46d in mozilla::nsTextNodeDirectionalityMap::ResetTextNodeDirection(nsTextNode*, nsTextNode*) /src/dom/base/DirectionalityUtils.cpp:620:37
#13 0x7fd135996757 in nsTextNode::UnbindFromTree(bool, bool) /src/dom/base/nsTextNode.cpp:155:3
#14 0x7fd1356bd444 in mozilla::dom::Element::UnbindFromTree(bool, bool) /src/dom/base/Element.cpp:1992:37
#15 0x7fd13762fe4a in nsGenericHTMLElement::UnbindFromTree(bool, bool) /src/dom/html/nsGenericHTMLElement.cpp:537:20
#16 0x7fd135912918 in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) /src/dom/base/nsINode.cpp:1945:9
#17 0x7fd1356e3885 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) /src/dom/base/FragmentOrElement.cpp:1369:5
#18 0x7fd13590c30d in nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:604:3
#19 0x7fd135e8fa15 in mozilla::dom::NodeBinding::removeChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/NodeBinding.cpp:1012:45
#20 0x7fd13703c40e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
#21 0x7fd13c00ced1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#22 0x7fd13c00ca7d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#23 0x7fd13c00d915 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#24 0x7fd13c002375 in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3084:18
#25 0x7fd13bfedfd1 in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
#26 0x7fd13c00ca00 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
#27 0x7fd13c00d915 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#28 0x7fd13c00db2c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
#29 0x7fd13c8c32eb in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2965:12
#30 0x7fd136b23139 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventListenerBinding.cpp:47:8
#31 0x7fd137365641 in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:65:12
#32 0x7fd1373651c6 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1109:9
#33 0x7fd137366525 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
#34 0x7fd137352e3b in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:313:17
#35 0x7fd1373524ff in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
#36 0x7fd137353fdd in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
#37 0x7fd137334f40 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /src/dom/events/EventDispatcher.cpp:888:12
#38 0x7fd13591005e in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /src/dom/base/nsINode.cpp:1341:5
#39 0x7fd135533bd4 in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, bool, bool, bool, bool*, bool) /src/dom/base/nsContentUtils.cpp:4514:18
#40 0x7fd13553397b in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsTSubstring<char16_t> const&, bool, bool, bool*) /src/dom/base/nsContentUtils.cpp:4482:10
#41 0x7fd135859043 in nsDocument::DispatchContentLoadedEvents() /src/dom/base/nsDocument.cpp:5505:3
#42 0x7fd1358d1ed5 in mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192:13
#43 0x7fd1333e471f in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#44 0x7fd1333e9a90 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
#45 0x7fd133f5a865 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#46 0x7fd133eaf7a7 in MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#47 0x7fd133eaf639 in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#48 0x7fd13886b85a in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#49 0x7fd13ba86641 in nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#50 0x7fd13bbeab01 in XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
#51 0x7fd13bbec7ca in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
#52 0x7fd13bbed6b8 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
#53 0x4ed398 in do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:236:22
#54 0x4eccb0 in main /src/browser/app/nsBrowserApp.cpp:309:16
#55 0x7fd1525c082f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#56 0x41e9e4 in _start (firefox+0x41e9e4)
Flags: in-testsuite?
INFO: Last good revision: eff91b4e33156ebcb56b52eb22a87cf034cd8619
INFO: First bad revision: 8daf204f77fbf183e3660fd89e170f076967d245
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=eff91b4e33156ebcb56b52eb22a87cf034cd8619&tochange=8daf204f77fbf183e3660fd89e170f076967d245
Blocks: 1394935
Has Regression Range: --- → yes
Flags: needinfo?(emilio)
Summary: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot) → stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot)
Priority: -- → P2
(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> INFO: Last good revision: eff91b4e33156ebcb56b52eb22a87cf034cd8619
> INFO: First bad revision: 8daf204f77fbf183e3660fd89e170f076967d245
> INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/
> pushloghtml?fromchange=eff91b4e33156ebcb56b52eb22a87cf034cd8619&tochange=8daf
> 204f77fbf183e3660fd89e170f076967d245

That patch added the assertion, so... :)

But will take a look regardless.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
So, this happens because we update the directionality of the <body> element from a text node's UnbindFromTree.

By the time we've gotten to that text node's unbind, the parent is unbound, but the child which is the restyle root still isn't (because we haven't got around to it), and thus we still haven't cleared out the root.

I don't have a particular plan to fix this (and this of course is unexpected both with and without my patches). Changing state from UnbindFromTree is pretty unfortunate.

Obvious fix is clearing the restyle root from the topmost UnbindFromTree if it's a descendant of us, but that isn't great.

I can also just silence the assertion if the root we're setting isn't an element, but I think we end up leaving some bits around which isn't good...

I can also update the directionality off a scriptrunner, I guess...

Bobby, opinions on what I propose above? I'm not in love with any of those :/
Flags: needinfo?(bobbyholley)
Emilio and I discussed a bit on IRC. I'm not in favor of leaving the bits around. I'm somewhat drawn towards the "delay it" approach, though a scriptrunner seems like potentially too much delay. We could build a bespoke mechanism to delay it until the end of the root unbind.

Emilio said he'd think about this more at the airport.
Flags: needinfo?(bobbyholley)
Comment on attachment 8911430 [details]
Bug 1400936: Remove dumb null check.

https://reviewboard.mozilla.org/r/182892/#review188106
Attachment #8911430 - Flags: review?(bobbyholley) → review+
Comment on attachment 8911431 [details]
Bug 1400936: Only tear down the servo data in SetXBLInsertionParent if the parent actually changed.

https://reviewboard.mozilla.org/r/182894/#review188108
Attachment #8911431 - Flags: review?(bobbyholley) → review+
Comment on attachment 8911433 [details]
Bug 1400936: Crashtests.

https://reviewboard.mozilla.org/r/182898/#review188110
Attachment #8911433 - Flags: review?(bobbyholley) → review+
Comment on attachment 8911432 [details]
Bug 1400936: Clear servo data after children data is cleared, and allow setting the root as the document if the tree is mid-unbind.

https://reviewboard.mozilla.org/r/182896/#review188112

::: dom/base/nsIDocumentInlines.h:70
(Diff revision 1)
>    MOZ_ASSERT(aDirtyBits);
>    MOZ_ASSERT((aDirtyBits & ~Element::kAllServoDescendantBits) == 0);
>    MOZ_ASSERT((aDirtyBits & mServoRestyleRootDirtyBits) == mServoRestyleRootDirtyBits);
>  
> +  // NOTE(emilio): The !aRoot->IsElement() check allows us to handle cases where
> +  // we change the restyle root during unbinding of a subtree where the root is.

I would say "clear" rather than "change", to make it more obvious.

::: dom/base/nsIDocumentInlines.h:72
(Diff revision 1)
> +  // In that case the tree can be in a somewhat inconsistent state (with the
> +  // document no longer being the subtree root of the current root, but the root
> +  // not having being unbound first).

Make a note of how this function handles that inconstent state.
Attachment #8911432 - Flags: review?(bobbyholley) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6a25312a3cb1
Remove dumb null check. r=bholley
https://hg.mozilla.org/integration/autoland/rev/9ef85da5aea4
Only tear down the servo data in SetXBLInsertionParent if the parent actually changed. r=bholley
https://hg.mozilla.org/integration/autoland/rev/dba974e6b1b4
Clear servo data after children data is cleared, and allow setting the root as the document if the tree is mid-unbind. r=bholley
https://hg.mozilla.org/integration/autoland/rev/7f9883dd37fe
Crashtests. r=bholley
Depends on: 1402684
Please request Beta approval on this when you get a chance.
Flags: needinfo?(emilio)
Flags: in-testsuite?
Flags: in-testsuite+
(And presumably bug 1402684 with it)
Comment on attachment 8911432 [details]
Bug 1400936: Clear servo data after children data is cleared, and allow setting the root as the document if the tree is mid-unbind.

This request goes for all the patches in the bug.

Note again, that this isn't completely necessary for correctness since we happen to clear the flags when we're bound to the tree again, but this is pretty trivial and I think it's worth to uplift alongside bug 1402684.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1383332
[User impact if declined]: none
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: bug 1402684
[Is the change risky?]: not much
[Why is the change risky/not risky?]: combined with the patch in bug 1402684, the final diff ends up being removing some flags again, which makes it a trivial change.
[String changes made/needed]: none
Flags: needinfo?(emilio)
Attachment #8911432 - Flags: approval-mozilla-beta?
I will consider the uplift once bug 1402684 landed!
Comment on attachment 8911432 [details]
Bug 1400936: Clear servo data after children data is cleared, and allow setting the root as the document if the tree is mid-unbind.

Fix an assert, ok, let's take it!
Should be in 57b3
Attachment #8911432 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8911430 [details]
Bug 1400936: Remove dumb null check.

[Triage Comment]
Attachment #8911430 - Flags: approval-mozilla-beta+
Attachment #8911431 - Flags: approval-mozilla-beta+
Attachment #8911433 - Flags: approval-mozilla-beta+
Depends on: 1403028
Depends on: 1405880
Depends on: 1405547
Depends on: 1413670
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: