Closed Bug 1461726 Opened 6 years ago Closed 6 years ago

Crash on document.write to an already existing ImageDocument

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1462789

People

(Reporter: jkratzer, Assigned: qdot)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(4 files)

trigger.html
752 bytes, text/html
Details
invalid-disposal-method-2.gif
167 bytes, image/gif
Details
Simplified crash test
352 bytes, text/html
Details
Bug 1461726 - Move synthetic doc creation to StartDocumentLoad, add null checks
59 bytes, text/x-review-board-request
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev cf3ee14023483cbbb57129479537c713e22c1980.

==11846==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f73cc6e157b bp 0x7ffd81066eb0 sp 0x7ffd81066de0 T0)
==11846==The signal is caused by a READ memory access.
==11846==Hint: address points to the zero page.
    #0 0x7f73cc6e157a in AppendChildTo /builds/worker/workspace/build/src/obj-firefox/dist/include/nsINode.h:884:12
    #1 0x7f73cc6e157a in mozilla::dom::MediaDocument::LinkStylesheet(nsTSubstring<char16_t> const&) /builds/worker/workspace/build/src/dom/html/MediaDocument.cpp:333
    #2 0x7f73cc6e010d in mozilla::dom::ImageDocument::SetScriptGlobalObject(nsIScriptGlobalObject*) /builds/worker/workspace/build/src/dom/html/ImageDocument.cpp:294:7
    #3 0x7f73c950635d in nsGlobalWindowOuter::SetNewDocument(nsIDocument*, nsISupports*, bool) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:1932:14
    #4 0x7f73cc73e068 in nsHTMLDocument::Open(JSContext*, mozilla::dom::Optional<nsTSubstring<char16_t> > const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1432:22
    #5 0x7f73cc74260d in nsHTMLDocument::WriteCommon(JSContext*, nsTSubstring<char16_t> const&, bool, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1703:38
    #6 0x7f73cc7418d9 in nsHTMLDocument::WriteCommon(JSContext*, mozilla::dom::Sequence<nsTString<char16_t> > const&, bool, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1621:5
    #7 0x7f73cb77a4ea in mozilla::dom::HTMLDocumentBinding::write(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:367:9
    #8 0x7f73cbc344d1 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3260:13
    #9 0x7f73d24e0167 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:280:15
    #10 0x7f73d24e0167 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
    #11 0x7f73d24e1162 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:535:10
    #12 0x7f73d30ffa55 in js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:176:12
    #13 0x7f73d30c5653 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:358:23
    #14 0x7f73d30dab51 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:510:21
    #15 0x7f73d30dd914 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:769:12
    #16 0x7f73d24e08b0 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:280:15
    #17 0x7f73d24e08b0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:449
    #18 0x7f73d24ca960 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:522:12
    #19 0x7f73d24ca960 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3086
    #20 0x7f73d24b1123 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:417:12
    #21 0x7f73d24e3604 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
    #22 0x7f73d24e3d8d in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:732:12
    #23 0x7f73d3035627 in ExecuteScript(JSContext*, JS::AutoVector<JSObject*>&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4763:12
    #24 0x7f73c9913ce6 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:268:8
    #25 0x7f73cdd2b976 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2327:25
    #26 0x7f73cdd2582b in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1956:10
    #27 0x7f73cdd22be1 in mozilla::dom::ScriptLoader::ProcessInlineScript(nsIScriptElement*, mozilla::dom::ScriptKind) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1595:10
    #28 0x7f73cdd06dc8 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1314:10
    #29 0x7f73cdd05f45 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:141:18
    #30 0x7f73c861f95b in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:247:18
    #31 0x7f73c861f95b in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:738
    #32 0x7f73c8618dff in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:537:7
    #33 0x7f73c8624d4b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:121:18
    #34 0x7f73c65e9ee1 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:337:32
    #35 0x7f73c6608ba3 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1090:14
    #36 0x7f73c6624770 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #37 0x7f73c7502c6a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #38 0x7f73c7456959 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #39 0x7f73c7456959 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #40 0x7f73c7456959 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #41 0x7f73cdf90d5a in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #42 0x7f73d21f849b in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:22
    #43 0x7f73c7456959 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #44 0x7f73c7456959 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #45 0x7f73c7456959 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #46 0x7f73d21f7e60 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:719:34
    #47 0x4f50dc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #48 0x4f50dc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282
    #49 0x7f73e629082f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
Flags: in-testsuite?
On a first glance, seems to be a nullptr dereference crash because of a missing check here: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/dom/html/MediaDocument.cpp#333

qdot, do you have time for this?
Flags: needinfo?(kyle)
Priority: -- → P1
While this specific crash happens due to the missing checks in LinkStylesheet (and would happen in LinkScript too, LinkStylesheet just happens to come first everywhere), fixing that check causes an assertion failure at https://searchfox.org/mozilla-central/source/dom/html/MediaDocument.cpp#196. I'm not familiar with how document readystate handling works, so ni?'ing :hsvionen to get more info there.
Assignee: nobody → kyle
Flags: needinfo?(kyle) → needinfo?(hsivonen)
A bit more background here. My fix was just checking if head was null (which it is) and returning NS_ERROR_FAILURE in LinkStylesheet/LinkScript. Due to the function that runs during the frame onload event removing the head/body nodes, we don't have a head element to attach scripts to, which is why that fails.

However, I'm not sure how we should handle the call to BecomeInteractive() at https://searchfox.org/mozilla-central/source/dom/html/ImageDocument.cpp#300. For some reason, the initial document never reaches READYSTATE_COMPLETE, and just sits loading forever. Gonna wait for :hsivonen's input on that before going any further.
ImageDocument::SetScriptGlobalObject() is supposed to get nullptr when it calls GetRootElement() and then call MediaDocument::CreateSyntheticDocument() before it gets linking style sheets.

So the root cause is someone creating a root element too early. Did we change over to using a public document creation API that creates the root element when creating the document like the JS-exposed document creation does? If so, we either need to revert that or replace the GetRootElement() check with a check that checks if the root element has more than zero children.
Flags: needinfo?(hsivonen)
Attached file Simplified crash test — 
I've simplified the fuzzer test to something a bit more human-readable, as it was doing things like using window.open as a delay to make sure the document.write call came after the frame onload had executed.

With this, the STR is down to:

- Create an ImageDocument via setting the src to an image
- After load, use document.write to write a null string to the imagedocument

Since we're doing document.write, the document contents gets scrapped and rewritten but the document itself is not rebuilt. Since it was created as a ImageDocument we expect there to be no root node yet. Still getting to the bottom of where the root node creation happens in that case and how to fix it.
AFAICT, the reason we get a root element here is because we've already created an image document, and we're trying to reuse that document. in
Summary: Crash [@ AppendChildTo] → Crash on document.write to an already existing ImageDocument
AFAICT, the reason we get a root element here is because we've already created an image document, and we're trying to reuse that document. This error wasn't coming up on VideoDocument because it creates its synthetic document as part of StartDocumentLoad, which isn't called on this path.

To fix that, I made ImageDocument process similarly to VideoDocument, creating the synthetic document in StartDocumentLoad. Between that and the null checks, it fixes the release crashes. That's included in the WIP patch I've attached to this bug.

The problem that remains is the BecomeInteractive() call. This asserts on debug in both VideoDocument and ImageDocument, because ReadyState is always nsIDocument::READYSTATE_LOADING due to the ReadyState set in nsHTMLDocument::Open (https://searchfox.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp#1538). If I skip the BecomeInteractive() call or comment out the assert, the document sits in loading state forever. 

:hsivonen, any ideas on how to fix this? I'm not quite sure when things should be kicked over to READYSTATE_COMPLETE
Flags: needinfo?(hsivonen)
(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #9)
> AFAICT, the reason we get a root element here is because we've already
> created an image document, and we're trying to reuse that document.

So we 
 1) create the document
 2) call document.write()
 3) try to set the script global object as if it was still a MediaDocument

Right?

It seems we should somehow make step #3 not do the MediaDocument stuff if Open has been called on the doc.

Does Open work on MediaDocument analogs in other browsers? (Sadly, the spec says to mark the document as an HTML document. Making it XHTML would nicely avoid this whole problem.)

> The problem that remains is the BecomeInteractive() call. This asserts on
> debug in both VideoDocument and ImageDocument, because ReadyState is always
> nsIDocument::READYSTATE_LOADING due to the ReadyState set in
> nsHTMLDocument::Open
> (https://searchfox.org/mozilla-central/source/dom/html/nsHTMLDocument.
> cpp#1538).

I don't follow. Open() sets it to READYSTATE_LOADING and BecomeInteractive wants to see it as READYSTATE_LOADING. What does BecomeInteractive see instead?

> :hsivonen, any ideas on how to fix this? I'm not quite sure when things
> should be kicked over to READYSTATE_COMPLETE

It's been 6 years, so I don't recall the details, but I believe we're supposed to rely on https://searchfox.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#1034 for both MediaDocuments and normal documents.
Flags: needinfo?(hsivonen)
bz fixed this in bug 1462789.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: