1505875 - crash near null in [@ NoteDirtyElement]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20181107215152
SourceStamp=5836a60614764631436bf5030c5baa34c676c7a2

==37384==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000748 (pc 0x7f96475329fa bp 0x7ffdf372b1f0 sp 0x7ffdf372b0e0 T0)
==37384==The signal is caused by a READ memory access.
==37384==Hint: address points to the zero page.
    #0 0x7f96475329f9 in get src/obj-firefox/dist/include/nsCOMPtr.h:919:48
    #1 0x7f96475329f9 in operator nsINode * src/obj-firefox/dist/include/nsCOMPtr.h:927
    #2 0x7f96475329f9 in GetServoRestyleRoot src/obj-firefox/dist/include/nsIDocument.h:3705
    #3 0x7f96475329f9 in NoteDirtyElement(mozilla::dom::Element*, unsigned int) src/dom/base/Element.cpp:4580
    #4 0x7f964dcdd26e in LazilyStyleNewChildRange src/layout/base/nsCSSFrameConstructor.cpp:6861:27
    #5 0x7f964dcdd26e in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsCSSFrameConstructor::InsertionKind) src/layout/base/nsCSSFrameConstructor.cpp:6981
    #6 0x7f964dc06569 in mozilla::PresShell::ContentAppended(nsIContent*) src/layout/base/PresShell.cpp:4543:22
    #7 0x7f964792a87a in nsNodeUtils::ContentAppended(nsIContent*, nsIContent*) src/dom/base/nsNodeUtils.cpp:192:3
    #8 0x7f9647799c03 in nsINode::InsertChildBefore(nsIContent*, nsIContent*, bool) src/dom/base/nsINode.cpp:1467:7
    #9 0x7f96478b79c7 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) src/dom/base/nsINode.cpp:2638:14
    #10 0x7f9647975721 in InsertBefore src/dom/base/nsINode.h:1798:12
    #11 0x7f9647975721 in nsRange::InsertNode(nsINode&, mozilla::ErrorResult&) src/dom/base/nsRange.cpp:2694
    #12 0x7f9648ad83ea in mozilla::dom::Range_Binding::insertNode(JSContext*, JS::Handle<JSObject*>, nsRange*, JSJitMethodCallArgs const&) src/obj-firefox/dom/bindings/RangeBinding.cpp:1157:9
    #13 0x7f964a819174 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3320:13
    #14 0x7f96538db29d in CallJSNative src/js/src/vm/Interpreter.cpp:468:15
    #15 0x7f96538db29d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:560
    #16 0x7f96538c4b8d in CallFromStack src/js/src/vm/Interpreter.cpp:620:12
    #17 0x7f96538c4b8d in Interpret(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:3461
    #18 0x7f96538a84e6 in js::RunScript(JSContext*, js::RunState&) src/js/src/vm/Interpreter.cpp:447:12
    #19 0x7f96538dbc41 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:587:15
    #20 0x7f96538dd8c2 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:633:10
    #21 0x7f965297c106 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2975:12
    #22 0x7f9649e38c39 in 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:265:37
    #23 0x7f964b0ac699 in void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:363:12
    #24 0x7f964b0a9929 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) src/dom/events/JSEventHandler.cpp:214:12
    #25 0x7f964b05d98a in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) src/dom/events/EventListenerManager.cpp:1107:52
    #26 0x7f964b05ff87 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) src/dom/events/EventListenerManager.cpp:1309:15
    #27 0x7f964b041b16 in HandleEvent src/obj-firefox/dist/include/mozilla/EventListenerManager.h:390:5
    #28 0x7f964b041b16 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:425
    #29 0x7f964b03fd98 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) src/dom/events/EventDispatcher.cpp:642:16
    #30 0x7f964b0467f0 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:1164:11
    #31 0x7f964dd20d5e in nsDocumentViewer::LoadComplete(nsresult) src/layout/base/nsDocumentViewer.cpp:1167:7
    #32 0x7f9650dc4233 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) src/docshell/base/nsDocShell.cpp:7079:21
    #33 0x7f9650dbfa59 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp:6870:7
    #34 0x7f9650dc8867 in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) src/docshell/base/nsDocShell.cpp
    #35 0x7f9645e479e5 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) src/uriloader/base/nsDocLoader.cpp:1309:3
    #36 0x7f9645e465cc in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:852:14
    #37 0x7f9645e41f08 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:741:9
    #38 0x7f9645e4485e in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp:630:5
    #39 0x7f9645e460f4 in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) src/uriloader/base/nsDocLoader.cpp
    #40 0x7f96437e1087 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:630:28
    #41 0x7f96477d10d7 in DoUnblockOnload src/dom/base/nsDocument.cpp:8517:18
    #42 0x7f96477d10d7 in nsDocument::UnblockOnload(bool) src/dom/base/nsDocument.cpp:8439
    #43 0x7f96477ab182 in nsIDocument::DispatchContentLoadedEvents() src/dom/base/nsDocument.cpp:5332:3
    #44 0x7f964790b09b in applyImpl<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1191:12
    #45 0x7f964790b09b in apply<nsIDocument, void (nsIDocument::*)()> src/obj-firefox/dist/include/nsThreadUtils.h:1197
    #46 0x7f964790b09b in mozilla::detail::RunnableMethodImpl<nsIDocument*, void (nsIDocument::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1242
    #47 0x7f964351d6e5 in mozilla::SchedulerGroup::Runnable::Run() src/xpcom/threads/SchedulerGroup.cpp:337:32
    #48 0x7f964355abe1 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1246:14
    #49 0x7f964356398d in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:530:10
    #50 0x7f96447d569f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #51 0x7f96446d1b5e in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #52 0x7f96446d1b5e in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #53 0x7f96446d1b5e in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #54 0x7f964d4a23f3 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
    #55 0x7f9651bb16ae in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:939:22
    #56 0x7f96446d1b5e in RunInternal src/ipc/chromium/src/base/message_loop.cc:325:10
    #57 0x7f96446d1b5e in RunHandler src/ipc/chromium/src/base/message_loop.cc:318
    #58 0x7f96446d1b5e in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298
    #59 0x7f9651bb070b in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:765:34
    #60 0x557751f21864 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #61 0x557751f21864 in main src/browser/app/nsBrowserApp.cpp:301
    #62 0x7f9666a1182f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #63 0x557751e46eec in _start (firefox+0x2deec)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Tim, this is a regression from bug 1496242. I'll try to repro the crash without UA widget (unlikely to repro), but even with that this is very scary, the summary is getting inserted inside the UA widget shadow tree.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Moved the ni? to bug 1505887.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1505875 - Clear out the ShadowRoot host pointer when unattaching it.

As expected, this is specific to the UA widget stuff.

What's going on here is that given we don't clear out the host when unattaching
the shadow tree, mutating that shadow tree still notifies all the way up to the
document, and that gets all the other code confused, thinking that the node is
connected.

Indeed, the first assertion that fails when loading that test-case in a debug
build is:

  https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/dom/base/nsNodeUtils.cpp#93

This seems the best fix to avoid confusion. Also clear the mutation observer,
to completely forget about the host.

Chrome code dealing with UA widgets needs to be careful, but I think this is
safe. All the code that assumes that GetHost() doesn't return null is in code
dealing with connected shadow trees only (style system / layout), or in
mutation observer notifications from the host.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/853e4a06fb03
Clear out the ShadowRoot host pointer when unattaching it. r=smaug

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6644d7251e9
Remove an assertion because I forgot of nsXMLPrettyPrinter.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

We can make XML pretty printer Shadow DOM an UA Widget too. That make more sense.

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/853e4a06fb03
https://hg.mozilla.org/mozilla-central/rev/d6644d7251e9