1450691 - Crash [@ nsPresContext::CacheAllLangs]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev c44f60c43432.

==4053==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000004c0 (pc 0x7f07d312b3a1 bp 0x7ffd9d49bde0 sp 0x7ffd9d49bc80 T0)
==4053==The signal is caused by a READ memory access.
==4053==Hint: address points to the zero page.
    #0 0x7f07d312b3a0 in nsPresContext::CacheAllLangs() /builds/worker/workspace/build/src/layout/base/nsPresContext.cpp
    #1 0x7f07d2e2713b in mozilla::ServoStyleSet::ResolveStyleLazily(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::StyleRuleInclusion) /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:646:3
    #2 0x7f07d2e70537 in nsComputedDOMStyle::UpdateCurrentStyleSources(bool) /builds/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:935:7
    #3 0x7f07d2e6f9e4 in nsComputedDOMStyle::Length() /builds/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:417:3
    #4 0x7f07cf38d855 in mozilla::dom::CSSStyleDeclarationBinding::get_length(JSContext*, JS::Handle<JSObject*>, nsICSSDeclaration*, JSJitGetterCallArgs) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/CSSStyleDeclarationBinding.cpp:127:25
    #5 0x7f07d069b38b in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2906:13
    #6 0x7f07d6f4fcc7 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:290:15
    #7 0x7f07d6f4fcc7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
    #8 0x7f07d6f521a3 in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:516:12
    #9 0x7f07d6f521a3 in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:535
    #10 0x7f07d6f521a3 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:650
    #11 0x7f07d7f10380 in CallGetter /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2176:16
    #12 0x7f07d7f10380 in GetExistingProperty<js::AllowGC::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2229
    #13 0x7f07d7f10380 in NativeGetPropertyInline<js::AllowGC::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2431
    #14 0x7f07d7f10380 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2467
    #15 0x7f07d7a73073 in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1640:12
    #16 0x7f07d7a73073 in JS_ForwardGetPropertyTo(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2690
    #17 0x7f07d0692ac3 in mozilla::dom::GetPropertyOnPrototype(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, bool*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2099:10
    #18 0x7f07ce402b1d in mozilla::dom::CSS2PropertiesBinding::DOMProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp:44209:8
    #19 0x7f07d7b4eb94 in getInternal /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:351:21
    #20 0x7f07d7b4eb94 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:361
    #21 0x7f07d7b75278 in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1639:16
    #22 0x7f07d7b75278 in js::ForwardingProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:154
    #23 0x7f07d7b31640 in js::CrossCompartmentWrapper::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:225:23
    #24 0x7f07d7b4eb94 in getInternal /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:351:21
    #25 0x7f07d7b4eb94 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:361
    #26 0x7f07d6f5b6b2 in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1639:16
    #27 0x7f07d6f5b6b2 in GetProperty /builds/worker/workspace/build/src/js/src/vm/JSObject.h:821
    #28 0x7f07d6f5b6b2 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4395
    #29 0x7f07d6f3d886 in GetPropertyOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:213:12
    #30 0x7f07d6f3d886 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2803
    #31 0x7f07d6f20bca in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:417:12
    #32 0x7f07d6f53164 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:700:15
    #33 0x7f07d6f538ef in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:732:12
    #34 0x7f07d7a94739 in ExecuteScript(JSContext*, JS::AutoVector<JSObject*>&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4721:12
    #35 0x7f07ce167336 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:268:8
    #36 0x7f07d2797dd1 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2326:25
    #37 0x7f07d279195b in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1955:10
    #38 0x7f07d278ed11 in mozilla::dom::ScriptLoader::ProcessInlineScript(nsIScriptElement*, mozilla::dom::ScriptKind) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1595:10
    #39 0x7f07d2772968 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1314:10
    #40 0x7f07d2771ae5 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:141:18
    #41 0x7f07ccef5afb in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:247:18
    #42 0x7f07ccef5afb in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:739
    #43 0x7f07cceeefc4 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:538:7
    #44 0x7f07ccefaeeb in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:121:18
    #45 0x7f07caeb6564 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:415:25
    #46 0x7f07caed5cc8 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1096:14
    #47 0x7f07caef2030 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #48 0x7f07cbdc122a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #49 0x7f07cbd10ff9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #50 0x7f07cbd10ff9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #51 0x7f07cbd10ff9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #52 0x7f07d29e71fa in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #53 0x7f07d6c6b2cb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #54 0x7f07cbd10ff9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #55 0x7f07cbd10ff9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #56 0x7f07cbd10ff9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #57 0x7f07d6c6acaa in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #58 0x4f50dc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #59 0x4f50dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #60 0x7f07eacd182f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1450691: Flushing the parent document can kill our shell.

Bug 1443492 made us flush the parent document for cross-doc getComputedStyle
situations. That can kill the shell we're flushing, let's handle that case
gracefully.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8964366 [details]
Bug 1450691: Flushing the parent document can kill our shell.

https://reviewboard.mozilla.org/r/233058/#review238602

::: layout/style/crashtests/1450691.html:2
(Diff revision 1)
> +  try { o0 = document.createElement('iframe') } catch (e) {}
> +  try { o1 = document.createElement('iframe') } catch (e) {}
> +  try { document.documentElement.appendChild(o0) } catch (e) {}
> +  try { o2 = o0.getSVGDocument() } catch (e) {}
> +  try { o3 = o2.childNodes[0] } catch (e) {}
> +  try { o3.appendChild(o1) } catch (e) {}
> +  try { o3.scrollHeight } catch (e) {}
> +  try { o3.hidden = true } catch (e) {}
> +  try { o4 = window.getComputedStyle(o1.contentDocument.childNodes[0].childNodes[1], ":z") } catch (e) {}
> +  try { o4.length } catch (e) {}

We don't really need that many `try { } catch {}`. Could you remove them?

::: layout/style/nsComputedDOMStyle.cpp:866
(Diff revision 1)
>    mPresShell = document->GetShell();
>    if (!mPresShell || !mPresShell->GetPresContext()) {

Could flushing on `presShellForContent` also kill this document? Should we also check whether `mPresShell` is destroying as well?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> Comment on attachment 8964366 [details]
> Bug 1450691: Flushing the parent document can kill our shell.
> 
> https://reviewboard.mozilla.org/r/233058/#review238602
> 
> ::: layout/style/crashtests/1450691.html:2
> (Diff revision 1)
> > +  try { o0 = document.createElement('iframe') } catch (e) {}
> > +  try { o1 = document.createElement('iframe') } catch (e) {}
> > +  try { document.documentElement.appendChild(o0) } catch (e) {}
> > +  try { o2 = o0.getSVGDocument() } catch (e) {}
> > +  try { o3 = o2.childNodes[0] } catch (e) {}
> > +  try { o3.appendChild(o1) } catch (e) {}
> > +  try { o3.scrollHeight } catch (e) {}
> > +  try { o3.hidden = true } catch (e) {}
> > +  try { o4 = window.getComputedStyle(o1.contentDocument.childNodes[0].childNodes[1], ":z") } catch (e) {}
> > +  try { o4.length } catch (e) {}
> 
> We don't really need that many `try { } catch {}`. Could you remove them?

Yup, I'll try.

> ::: layout/style/nsComputedDOMStyle.cpp:866
> (Diff revision 1)
> >    mPresShell = document->GetShell();
> >    if (!mPresShell || !mPresShell->GetPresContext()) {
> 
> Could flushing on `presShellForContent` also kill this document? Should we
> also check whether `mPresShell` is destroying as well?

We do check that already with GetPresContext(), though it's kind of implicit.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa17423a375
Flushing the parent document can kill our shell. r=xidorn

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> > ::: layout/style/nsComputedDOMStyle.cpp:866
> > (Diff revision 1)
> > >    mPresShell = document->GetShell();
> > >    if (!mPresShell || !mPresShell->GetPresContext()) {
> > 
> > Could flushing on `presShellForContent` also kill this document? Should we
> > also check whether `mPresShell` is destroying as well?
> 
> We do check that already with GetPresContext(), though it's kind of implicit.

I don't see how it works. As far as I can see, GetPresContext() doesn't check whether the PresShell is destroying, and PresShell::Destroy() which is where mIsDestroying is set, doesn't reset mPresContext. Actually, PresShell::Destroy() still tries to use mPresContext near its end.

So it doesn't seem to me there is any guarantee that GetPresContext() returns null when the PresShell is destroying. Am I missing something?

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Err, you're right, sorry. This works because GetShell doesn't return a destroyed shell, and we do document->GetShell() after flushing, not before. I guess I can change the scheme to avoid using GetPresShellForContent and just use GetComposedDoc, may be cleaner...

Anyway, will file a new bug for that.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Oh, okay, I see how GetShell() works.

We should probably add a comment somewhere that nsIDocument::GetShell() doesn't return a destroying shell.

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2aa17423a375