1425866 - Crash near null in [@ nsStyleLinkElement::DoUpdateStyleSheet]

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 5572465c08a9.

==29290==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000000aa (pc 0x7fe0e4f36b1c bp 0x7ffc1c99c2a0 sp 0x7ffc1c99bae0 T0)
==29290==The signal is caused by a READ memory access.
==29290==Hint: address points to the zero page.
    #0 0x7fe0e4f36b1b in GetEnabled /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/css/Loader.h:429:30
    #1 0x7fe0e4f36b1b in nsStyleLinkElement::DoUpdateStyleSheet(nsIDocument*, mozilla::dom::ShadowRoot*, nsICSSLoaderObserver*, bool*, bool*, bool) /builds/worker/workspace/build/src/dom/base/nsStyleLinkElement.cpp:469
    #2 0x7fe0e4f38ac5 in nsStyleLinkElement::UpdateStyleSheetInternal(nsIDocument*, mozilla::dom::ShadowRoot*, bool) /builds/worker/workspace/build/src/dom/base/nsStyleLinkElement.cpp:344:10
    #3 0x7fe0e83952aa in mozilla::dom::SVGStyleElement::UnbindFromTree(bool, bool) /builds/worker/workspace/build/src/dom/svg/SVGStyleElement.cpp:89:3
    #4 0x7fe0e4b62f1a in mozilla::dom::Element::UnbindFromTree(bool, bool) /builds/worker/workspace/build/src/dom/base/Element.cpp:2129:14
    #5 0x7fe0e761b419 in nsGenericHTMLElement::UnbindFromTree(bool, bool) /builds/worker/workspace/build/src/dom/html/nsGenericHTMLElement.cpp:537:20
    #6 0x7fe0e4b4b3e7 in mozilla::dom::FragmentOrElement::cycleCollection::Unlink(void*) /builds/worker/workspace/build/src/dom/base/FragmentOrElement.cpp:1601:16
    #7 0x7fe0e19b8e44 in nsCycleCollector::CollectWhite() /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3396:26
    #8 0x7fe0e19bbb7d in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3764:24
    #9 0x7fe0e19bf7c0 in nsCycleCollector_collect(nsICycleCollectorListener*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4310:21
    #10 0x7fe0e4e984dc in nsJSContext::CycleCollectNow(nsICycleCollectorListener*) /builds/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1505:3
    #11 0x7fe0e49b7eab in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*) /builds/worker/workspace/build/src/dom/base/nsDOMWindowUtils.cpp:1299:3
    #12 0x7fe0e1b551a1 in NS_InvokeByIndex /builds/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:106
    #13 0x7fe0e35a7ead in Invoke /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1948:12
    #14 0x7fe0e35a7ead in Call /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1267
    #15 0x7fe0e35a7ead in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1234
    #16 0x7fe0e35ae744 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:929:12
    #17 0x7fe0ed989264 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #18 0x7fe0ed989264 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #19 0x7fe0ed96f348 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #20 0x7fe0ed96f348 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #21 0x7fe0ed95bab0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #22 0x7fe0ed98979c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
    #23 0x7fe0ed98a2c2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
    #24 0x7fe0ee485d41 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2936:12
    #25 0x7fe0e34be8f2 in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:315:18
    #26 0x7fe0ed989264 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #27 0x7fe0ed989264 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #28 0x7fe0ed96f348 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #29 0x7fe0ed96f348 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #30 0x7fe0ed95bab0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #31 0x7fe0ed98979c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
    #32 0x7fe0ed98a2c2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
    #33 0x7fe0ee4881bc in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2995:12
    #34 0x7fe0e68471b3 in mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/FunctionBinding.cpp:36:8
    #35 0x7fe0e4a4420b in Call<nsCOMPtr<nsISupports> > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FunctionBinding.h:72:12
    #36 0x7fe0e4a4420b in nsGlobalWindowInner::RunTimeoutHandler(mozilla::dom::Timeout*, nsIScriptContext*) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:6446
    #37 0x7fe0e4c78767 in mozilla::dom::TimeoutManager::RunTimeout(mozilla::TimeStamp const&, mozilla::TimeStamp const&) /builds/worker/workspace/build/src/dom/base/TimeoutManager.cpp:876:42
    #38 0x7fe0e4c77aa4 in mozilla::dom::TimeoutExecutor::MaybeExecute() /builds/worker/workspace/build/src/dom/base/TimeoutExecutor.cpp:171:11
    #39 0x7fe0e4c79b16 in Notify /builds/worker/workspace/build/src/dom/base/TimeoutExecutor.cpp:239:5
    #40 0x7fe0e4c79b16 in non-virtual thunk to mozilla::dom::TimeoutExecutor::Notify(nsITimer*) /builds/worker/workspace/build/src/dom/base/TimeoutExecutor.cpp
    #41 0x7fe0e1b49c7c in nsTimerImpl::Fire(int) /builds/worker/workspace/build/src/xpcom/threads/nsTimerImpl.cpp:704:40
    #42 0x7fe0e1b194b9 in nsTimerEvent::Run() /builds/worker/workspace/build/src/xpcom/threads/TimerThread.cpp:286:11
    #43 0x7fe0e1b397cf in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /builds/worker/workspace/build/src/xpcom/threads/ThrottledEventQueue.cpp:193:22
    #44 0x7fe0e1b392ff in mozilla::ThrottledEventQueue::Inner::Executor::Run() /builds/worker/workspace/build/src/xpcom/threads/ThrottledEventQueue.cpp:79:15
    #45 0x7fe0e1b02870 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:395:25
    #46 0x7fe0e1b290e6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14
    #47 0x7fe0e1b44bd0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10
    #48 0x7fe0e29cf434 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:125:5
    #49 0x7fe0e2922de9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #50 0x7fe0e2922de9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #51 0x7fe0e2922de9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #52 0x7fe0e8f68f8a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #53 0x7fe0ed6a05cb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:875:22
    #54 0x7fe0e2922de9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #55 0x7fe0e2922de9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #56 0x7fe0e2922de9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #57 0x7fe0ed69ffbd in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:701:34
    #58 0x4f2dfc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #59 0x4f2dfc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #60 0x7fe100c6282f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #61 0x42243c in _start (/home/forb1dden/builds/mc-asan/firefox+0x42243c)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/css/Loader.h:429:30 in GetEnabled
==29290==ABORTING

Comment 1

[Jet Villegas (inactive)]

I see createShadowRoot() in the test case, but it's cycle-collecting this element that's crashing:
o2 = document.createElementNS('http://www.w3.org/2000/svg', 'style')

Cam: anything special about that one?

Comment 2

[Cameron McCormack (:heycam)]

I guess we're in the middle of CCing the document, and so we have unlinked the nsIDocument and thus cleared its mCSSLoader.  Then as part of unbinding the <style> while unlinking, we look at document->CSSLoader() and assume it's not null.

Maybe without Shadow DOM involved, we would normally return early here:

https://searchfox.org/mozilla-central/rev/f6f1731b1b7fec332f86b55fa40e2c9ae67ac39b/dom/base/nsStyleLinkElement.cpp#467-471

since doc would be null.  I would have thought that by this point we we've cleared the NODE_IS_IN_SHADOW_TREE flag on the <style> element (in Element::UnbindFromTree, which should have been called just before the DoUpdateStyleSheet).

(Leaving NI on myself to investigate when I'm back.)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Yeah, that looks a ton like it really wants to use GetComposedDoc()...

Comment 4

[Cameron McCormack (:heycam)]

Since this is Shadow DOM related, can I pass this one to you Emilio?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Sure, keeping ni? so I get to it.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Hmm, so I thought I had a fix, but it's actually a bit more intrincate... So what's going on is that during unlinking, we never unbind its children with the aNullParent argument.

Olli, my idea at this point is replacing the two booleans to UnbindFromTree with a struct (UnbindOptions?), and set a specific flag during unlinking to unlink the Shadow DOM children too. Does that sound fine?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05) from comment #6)
> Hmm, so I thought I had a fix, but it's actually a bit more intrincate... So
> what's going on is that during unlinking, we never unbind its children with
> the aNullParent argument.
If we unlink a whole DOM subtree and don't use ContentUnbinder (to break the tree asynchronously) we do
end up unbinding all the elements with nullparent. Remember that unlink will be called on all the nodes and in random order.


> Olli, my idea at this point is replacing the two booleans to UnbindFromTree
> with a struct (UnbindOptions?), and set a specific flag during unlinking to
> unlink the Shadow DOM children too. Does that sound fine?
Shadow DOM children are unlinked during unlink (assuming ContentUnbinder doesn't kick in).
I think I don't quite understand what you're proposing.


https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/dom/base/nsIDocument.h#1619 is wrong. It is definitely not guaranteed to have non-null cssloader per
https://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#2076

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #7)
> (In reply to Emilio Cobos Álvarez [:emilio] (Away-ish from 27/04 to 09/05)
> from comment #6)
> > Hmm, so I thought I had a fix, but it's actually a bit more intrincate... So
> > what's going on is that during unlinking, we never unbind its children with
> > the aNullParent argument.
> If we unlink a whole DOM subtree and don't use ContentUnbinder (to break the
> tree asynchronously) we do
> end up unbinding all the elements with nullparent. Remember that unlink will
> be called on all the nodes and in random order.

Not if that node has a ShadowRoot. In that case, to unbind the shadow root children, we end up here, which does NOT pass aNullParent = true:

  https://searchfox.org/mozilla-central/rev/36dec78aecc40539ecc8d78e91612e38810f963c/dom/base/Element.cpp#2038

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

unlink is called on all the objects in the garbage graph, including ShadowRoot, and it will unbind its child nodes.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1425866: Just null-check the CSS Loader for now.

After discussing with Olli there isn't any kind of severe problem out of this.
Shadow subtrees will be disconnected just like the rest, and they shouldn't
assume that the document hasn't been disconnected first.

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

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8971444 [details]
Bug 1425866: Just null-check the CSS Loader for now.

https://reviewboard.mozilla.org/r/240190/#review246000

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a42293258ab3
Just null-check the CSS Loader for now. r=heycam

Comment 13

[Cosmin Sabou [:CosminS]]

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