1404134 - stylo: Assertion failure: aElement->GetComposedDoc()->GetServoRestyleRoot() [@ NoteDirtyElement]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: aElement->GetComposedDoc()->GetServoRestyleRoot(), at /src/dom/base/Element.cpp:4385

#0 NoteDirtyElement(mozilla::dom::Element*, unsigned int) /src/dom/base/Element.cpp:4399:31
#1 mozilla::ServoRestyleManager::SnapshotFor(mozilla::dom::Element*) /src/layout/base/ServoRestyleManager.cpp:1049:13
#2 mozilla::ServoRestyleManager::ContentStateChanged(nsIContent*, mozilla::EventStates) /src/layout/base/ServoRestyleManager.cpp:1245:36
#3 mozilla::PresShell::ContentStateChanged(nsIDocument*, nsIContent*, mozilla::EventStates) /src/layout/base/PresShell.cpp:4279:37
#4 nsDocument::ContentStateChanged(nsIContent*, mozilla::EventStates) /src/dom/base/nsDocument.cpp:5667:3
#5 mozilla::dom::Element::UpdateState(bool) /src/dom/base/Element.cpp:272:14
#6 mozilla::dom::Link::ResetLinkState(bool, bool) /src/dom/base/Link.cpp:792:15
#7 nsDocument::RefreshLinkHrefs() /src/dom/base/nsDocument.cpp:9598:23
#8 nsDocument::SetBaseURI(nsIURI*) /src/dom/base/nsDocument.cpp:3881:3
#9 mozilla::dom::SetBaseURIUsingFirstBaseWithHref(nsIDocument*, nsIContent*) /src/dom/html/HTMLSharedElement.cpp:201:14
#10 mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) /src/dom/html/HTMLSharedElement.cpp:294:7
#11 mozilla::dom::Element::UnbindFromTree(bool, bool) /src/dom/base/Element.cpp:1977:37
#12 nsGenericHTMLElement::UnbindFromTree(bool, bool) /src/dom/html/nsGenericHTMLElement.cpp:537:20
#13 nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) /src/dom/base/nsINode.cpp:1945:9
#14 mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) /src/dom/base/FragmentOrElement.cpp:1368:5
#15 nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:2432:5
#16 nsINode::ReplaceWith(mozilla::dom::Sequence<mozilla::dom::OwningNodeOrString> const&, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:1836:13
#17 mozilla::dom::ElementBinding::replaceWith(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/ElementBinding.cpp:3687:9
#18 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
#19 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#20 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#21 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#22 Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3084:18
#23 js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
#24 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
#25 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#26 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
#27 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
#28 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
#29 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
#30 mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /src/dom/events/JSEventHandler.cpp:215:12
#31 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1112:51
#32 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
#33 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:313:17
#34 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
#35 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
#36 nsDocumentViewer::LoadComplete(nsresult) /src/layout/base/nsDocumentViewer.cpp:1081:7
#37 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /src/docshell/base/nsDocShell.cpp:7760:21
#38 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /src/docshell/base/nsDocShell.cpp:7558:7
#39 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /src/docshell/base/nsDocShell.cpp:7455:13
#40 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /src/uriloader/base/nsDocLoader.cpp:1320:3
#41 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /src/uriloader/base/nsDocLoader.cpp:861:14
#42 nsDocLoader::DocLoaderIsEmpty(bool) /src/uriloader/base/nsDocLoader.cpp:750:9
#43 nsDocLoader::DocLoaderIsEmpty(bool) /src/uriloader/base/nsDocLoader.cpp:753:19
#44 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:632:5
#45 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:488:14
#46 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /src/netwerk/base/nsLoadGroup.cpp:629:28
#47 nsDocument::DoUnblockOnload() /src/dom/base/nsDocument.cpp:9234:18
#48 nsDocument::UnblockOnload(bool) /src/dom/base/nsDocument.cpp:9156:9
#49 nsDocument::DispatchContentLoadedEvents() /src/dom/base/nsDocument.cpp:5600:3
#50 mozilla::detail::RunnableMethodImpl<nsDocument*, void (nsDocument::*)(), true, (mozilla::RunnableKind)0>::Run() /src/obj-firefox/dist/include/nsThreadUtils.h:1192:13
#51 mozilla::SchedulerGroup::Runnable::Run() /src/xpcom/threads/SchedulerGroup.cpp:396:25
#52 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#53 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:524:10
#54 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#55 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#56 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#57 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#58 XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:880:22
#59 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:269:9
#60 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#61 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#62 XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:705:34
#63 content_process_main(mozilla::Bootstrap*, int, char**) /src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
#64 main /src/browser/app/nsBrowserApp.cpp:285:18
#65 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#66 _start (/home/user/workspace/browsers/m-c-1506533910-asan-debug/firefox+0x41eb24)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Yup, I guess this is my department now... We set the body as a restyle root, then unbind the body, then one of those children changes state again.

There's nothing safety-wise problematic now here, thankfully, so the key here is knowing whether we want to make that assertion hold.

The proper fix for this would be to not clear the flags or the assertion from the current ClearServoData() call, and do that only and the end of UnbindSubtree...

Comment 2

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 698252497ed8387d10de995262938316faaf41de
INFO: First bad revision: f44a80769c80ef7aec9d175f88dc68897928275d
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=698252497ed8387d10de995262938316faaf41de&tochange=f44a80769c80ef7aec9d175f88dc68897928275d

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1404134: Drop assertion that can be hit mid-unbind.

On that test-case we have something like:

  <body restyle-root>
    <element dirty-descendants>
      <element-2>
    </element>
  </body>

If we change the state of element-2 after unbinding the <body>, which clears the
body's flags, but not the children's flags, there'll be no restyle root, even
though our parent has the relevant flags anyway.

This doesn't compromise correctness in any way, so I'd rather drop the assertion
that shuffle a bunch of stuff so it holds.

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

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8914209 [details]
Bug 1404134: Drop assertion that can be hit mid-unbind.

https://reviewboard.mozilla.org/r/185532/#review190468

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f06d589bf31b
Drop assertion that can be hit mid-unbind. r=heycam

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f06d589bf31b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Did we want to land the testcase from this bug still? Also, please request Beta approval on this when you get a chance.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8914209 [details]
Bug 1404134: Drop assertion that can be hit mid-unbind.

I didn't manage to construct a reliable test-case.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1383332
[User impact if declined]: none
[Is this code covered by automated tests?]: no
[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]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just removes a debug-only assertion that doesn't hold during one of the edge-case situation.
[String changes made/needed]: none

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914209 [details]
Bug 1404134: Drop assertion that can be hit mid-unbind.

Stylo related, Beta57+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/711cce0acdcc