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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: 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)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: eff91b4e33156ebcb56b52eb22a87cf034cd8619
INFO: First bad revision: 8daf204f77fbf183e3660fd89e170f076967d245
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=eff91b4e33156ebcb56b52eb22a87cf034cd8619&tochange=8daf204f77fbf183e3660fd89e170f076967d245

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 3

[Emilio Cobos Álvarez (: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 :/

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1400936: Remove dumb null check.

Review commit: https://reviewboard.mozilla.org/r/182892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182892/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1400936: Only tear down the servo data in SetXBLInsertionParent if the parent actually changed.

This is the reason similar assertion failures can't be reproduced with elements
and stuff like fieldset and form validity changes.

nsBindingManager::ContentRemoved calls SetXBLInsertionParent, which clears all
the Servo data from the subtree eagerly, which is a waste when the actual
binding parent is the same (null).

Review commit: https://reviewboard.mozilla.org/r/182894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182894/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: 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 is the actual fix, and makes sure that the state is consistent even if we
notify of state changes on a parent during unbind.

We potentially do a bit more work than needed given we set the document as the
root in that case instead of the parent which could potentially be the root
itself, but that's not a huge deal I think, given these cases are rare.

If this happens to be a perf problem, we may want to just drop the root during
UnbindFromTree if aNullParent == true and the root is a descendant of `this`.

Review commit: https://reviewboard.mozilla.org/r/182896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182896/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1400936: Crashtests.

Review commit: https://reviewboard.mozilla.org/r/182898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182898/

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8911430 [details]
Bug 1400936: Remove dumb null check.

https://reviewboard.mozilla.org/r/182892/#review188106

Comment 10

[Bobby Holley (:bholley)]

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

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8911433 [details]
Bug 1400936: Crashtests.

https://reviewboard.mozilla.org/r/182898/#review188110

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8911430 [details]
Bug 1400936: Remove dumb null check.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182892/diff/1-2/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8911431 [details]
Bug 1400936: Only tear down the servo data in SetXBLInsertionParent if the parent actually changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182894/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182896/diff/1-2/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8911433 [details]
Bug 1400936: Crashtests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182898/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6a25312a3cb1
https://hg.mozilla.org/mozilla-central/rev/9ef85da5aea4
https://hg.mozilla.org/mozilla-central/rev/dba974e6b1b4
https://hg.mozilla.org/mozilla-central/rev/7f9883dd37fe

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(And presumably bug 1402684 with it)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

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

Comment 22

[Sylvestre Ledru [:Sylvestre]]

I will consider the uplift once bug 1402684 landed!

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911430 [details]
Bug 1400936: Remove dumb null check.

[Triage Comment]

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d4c045f3066
https://hg.mozilla.org/releases/mozilla-beta/rev/6b96a0fab002
https://hg.mozilla.org/releases/mozilla-beta/rev/ca0193447310
https://hg.mozilla.org/releases/mozilla-beta/rev/99992ed2b3f3