Crash near null in [@ nsStyleLinkElement::DoUpdateStyleSheet]

RESOLVED FIXED in Firefox 61

Status

()

defect
P3
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jkratzer, Assigned: emilio)

Tracking

(Depends on 1 bug, Blocks 1 bug, {crash, testcase})

59 Branch
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Posted file 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
Flags: in-testsuite?
Component: DOM → DOM: CSS Object Model
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?
Flags: needinfo?(cam)
Priority: -- → P3
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.)
(Assignee)

Comment 3

a year ago
Yeah, that looks a ton like it really wants to use GetComposedDoc()...
Since this is Shadow DOM related, can I pass this one to you Emilio?
Flags: needinfo?(cam) → needinfo?(emilio)
(Assignee)

Comment 5

a year ago
Sure, keeping ni? so I get to it.
(Assignee)

Updated

a year ago
Assignee: nobody → emilio
Flags: needinfo?(emilio)
(Assignee)

Comment 6

a year ago
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?
Flags: needinfo?(bugs)
(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
Flags: needinfo?(bugs)
Depends on: 1456987
(Assignee)

Comment 8

a year ago
(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
unlink is called on all the objects in the garbage graph, including ShadowRoot, and it will unbind its child nodes.
Comment hidden (mozreview-request)

Comment 11

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

https://reviewboard.mozilla.org/r/240190/#review246000
Attachment #8971444 - Flags: review?(cam) → review+

Comment 12

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a42293258ab3
Just null-check the CSS Loader for now. r=heycam

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a42293258ab3
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Duplicate of this bug: 1427820
You need to log in before you can comment on or make changes to this bug.