Jason Kratzer [:jkratzer] Reporter
	Description • 7 years ago

Testcase found while fuzzing mozilla-central rev d734e6acf777.

==14112==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fa2b7475ec5 bp 0x7ffceb49d030 sp 0x7ffceb49cf40 T0)
==14112==The signal is caused by a WRITE memory access.
==14112==Hint: address points to the zero page.
    #0 0x7fa2b7475ec4 in nsDocument::BeginLoad() /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:5396:28
    #1 0x7fa2b66d76f7 in nsHtml5TreeOpExecutor::WillBuildModel(nsDTDMode) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:107:18
    #2 0x7fa2b66c27f8 in nsHtml5Parser::Parse(nsTSubstring<char16_t> const&, void*, nsTSubstring<char> const&, bool, nsDTDMode) /builds/worker/workspace/build/src/parser/html/nsHtml5Parser.cpp:251:20
    #3 0x7fa2b92c8857 in nsHTMLDocument::WriteCommon(JSContext*, nsTSubstring<char16_t> const&, bool) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1998:56
    #4 0x7fa2b92c8182 in nsHTMLDocument::WriteCommon(JSContext*, mozilla::dom::Sequence<nsTString<char16_t> > const&, bool, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:1900:10
    #5 0x7fa2b8a1866b in mozilla::dom::HTMLDocumentBinding::writeln(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:740:9
    #6 0x7fa2b8caa3ae in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3040:13
    #7 0x7fa2bdc75671 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #8 0x7fa2bdc7524a in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473:16
    #9 0x7fa2bdc762f5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:522:12
    #10 0x7fa2bdc7650c 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
    #11 0x7fa2be7afc1e in js::ForwardingProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/Wrapper.cpp:175:12
    #12 0x7fa2be770508 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const /builds/worker/workspace/build/src/js/src/proxy/CrossCompartmentWrapper.cpp:359:23
    #13 0x7fa2be79aba1 in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:512:21
    #14 0x7fa2be79cd66 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/js/src/proxy/Proxy.cpp:771:12
    #15 0x7fa2bdc75671 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #16 0x7fa2bdc7538c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:455:16
    #17 0x7fa2bdc762f5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:522:12
    #18 0x7fa2bdc6a88f in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3067:18
    #19 0x7fa2bdc55fb4 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #20 0x7fa2bdc77f72 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:706:15

	Ryan VanderMeulen [:RyanVM]
	Comment 1 • 7 years ago

Two different issues with this testcase.

Stuck in a permaloading state after bug 1343728 landed:
INFO: Last good revision: 5cac74206e4e96e652289c80f2499827c0907162
INFO: First bad revision: a1e773337202d436865cbdd1fa375277efada840
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5cac74206e4e96e652289c80f2499827c0907162&tochange=a1e773337202d436865cbdd1fa375277efada840

Asserting after bug 1387243 added the assert:
INFO: Last good revision: f636e51b75dd59c65ddcbd9e6502fcf1d1710da5
INFO: First bad revision: 1eb4231c236166c76f353d43e36045b7bb560123
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f636e51b75dd59c65ddcbd9e6502fcf1d1710da5&tochange=1eb4231c236166c76f353d43e36045b7bb560123

	Brad Werth [:bradwerth] Assignee
	Comment 2 • 7 years ago

I can reproduce; I'll see if I can fix.

	Brad Werth [:bradwerth] Assignee
	Comment 3 • 7 years ago

Yeah, this is a weird one. I think I can describe it. The test case replaces the content of a document, but that document contains a frame with an onUnload handler that ALSO modifies the document. In nsHTMLDocument::Open (triggered by the content replacement), the onUnload handler is called before the parser has been created. So then two parsers are created for the same document, one overwriting the other. But not before the overwritten parser is invoked enough to set the mDidCallBeginLoad flag on the document. So the main issue, I believe, is the overwriting of the parser. If the second call to nsHTMLDocument::Open reused the parser, we might be fine.

I'll try to find a modification for nsHTMLDocument::Open that can account for this odd calling pattern, and which doesn't throw away any of the written content.

	Brad Werth [:bradwerth] Assignee
	Comment 6 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83a34d8970b2c8e44d9377e147267ce2b29a601

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8923594 [details]
Bug 1412173 Part 1: Implement the Document ignore-opens-during-unload counter required by spec.

https://reviewboard.mozilla.org/r/194722/#review200778

::: dom/html/nsHTMLDocument.cpp:1623
(Diff revision 1)
> -        // We don't want to unload, so stop here, but don't throw an
> -        // exception.
> +        // There's a few things that could have happened in the
> +        // unload handler that would cause us to exit. We might have
> +        // been told not to unload, or we might have triggered another
> +        // call to ::Open which may have already created another
> +        // parser. In these cases, exit but don't throw an exception.
> +        if (!okToUnload || mParser) {

This is ignoring the wrong (outer) open() call, whereas per spec the inner one should be ignored.

I can see taking this as a spot-fix if really neded, because this is better than violating invariants, but would prefer just following the spec.

I wonder whether there are web platform tests for this stuff...

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8923595 [details]
Bug 1412173 Part 2: Add a crashtest to check we can handle overlapping document open calls.

https://reviewboard.mozilla.org/r/194724/#review200780

::: dom/html/crashtests/1412173.html:10
(Diff revision 1)
> +  var o2;
> +
> +  var o1 = document.createElement('frame');
> +  o1.onload = function() {
> +    o1.contentDocument.body.onbeforeunload = function () { o1.contentDocument.write('<p>beforeUnload</p>') };
> +    o2 = this.contentDocument.createRange();

You shouldn't really need a range here.  It's just being used as a complicated way to get at `this.contentDocument`, via o2.endContainer.

::: dom/html/crashtests/1412173.html:14
(Diff revision 1)
> +    o1.contentDocument.body.onbeforeunload = function () { o1.contentDocument.write('<p>beforeUnload</p>') };
> +    o2 = this.contentDocument.createRange();
> +  };
> +  document.documentElement.appendChild(o1);
> +
> +  let emptyWindow = window.open('', '', 'just-a-non-empty-string');

And _this_ is being used as a complicated way to spin the event loop, and hence get the load event to fire on the iframe.   Super-fragile.

It would be much better to just reftest-wait in this test, wait for the iframe's load event to fire, then do something like this:

    o1.onload = () => {
      o1.contentDocument.body.onbeforeunload = function () { o1.contentDocument.write('<p>beforeUnload</p>') };
      o1.contentDocument.write('<p>Let's go</p>');
    }

or so.

	Brad Werth [:bradwerth] Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

Comment on attachment 8923594 [details]
Bug 1412173 Part 1: Implement the Document ignore-opens-during-unload counter required by spec.

https://reviewboard.mozilla.org/r/194722/#review200778

> This is ignoring the wrong (outer) open() call, whereas per spec the inner one should be ignored.
> 
> I can see taking this as a spot-fix if really neded, because this is better than violating invariants, but would prefer just following the spec.
> 
> I wonder whether there are web platform tests for this stuff...

I agree with you in principle, but I argue that this DOES follow the spec. That is, if the comment earlier in the function definition is correct:

 // The WHATWG spec says: "If the document has an active parser that isn't
 // a script-created parser, and the insertion point associated with that
 // parser's input stream is not undefined (that is, it does point to
 // somewhere in the input stream), then the method does nothing. Abort
 // these steps and return the Document object on which the method was
 // invoked."
    
The inner call occurs at a time when the Document does not have a parser. There's no spec reason to reject that call. Indeed if we did, then the inner call could proceed with actions that would rely on the presence of a parser that would then fail. The fact that the parser creation from the outer call happens second and the inner call happens first is counterintuitive, but it's the inner call that must succeed, not the outer one.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 7 years ago

> That is, if the comment earlier in the function definition is correct:

Note that this comment not longer matches any part of the spec.

> The inner call occurs at a time when the Document does not have a parser.

That's true.

> There's no spec reason to reject that call.

Yes, there is.  I had a nice long comment pointing to the relevant spec bits, before I did the review, but I see that it's not in the bug.  I have no idea what Bugzilla did with it.  :(  So let's do this again...

The relevant spec for open() is https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-open-steps and the relevant step is step 6:

   Similarly, if document's ignore-opens-during-unload counter is greater than 0, then return document.

(without doing anything else).  The ignore-opens-during-unload counter is defined at https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#ignore-opens-during-unload-counter and is nonzero during the firing of beforeunload, unload, and pagehide events, in the spec.  In our implementation we check for unload in http://searchfox.org/mozilla-central/rev/423b2522c48e1d654e30ffc337164d677f934ec3/dom/html/nsHTMLDocument.cpp#1557-1562 but don't check for the other two events.  The testcase does an (implicit) open() from inside beforeunload, so per spec that open() call should be ignored but in our implementation it's not, because we don't implement the "ignore during unload" checks per spec.

Of course per spec, the write() call shouldn't even reach the "document open steps", because https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-write-steps step 4 substep 1 should early-return.  We don't do that either.

> then the inner call could proceed with actions that would rely on the presence of a parser that would then
> fail.

Well, it would no-op, if we implemented the spec, because it's happening under beforeunload.

	Brad Werth [:bradwerth] Assignee
	Comment 13 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=392ed8ee58be5362d91a3ac61bb9872bca3e70f0

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8923594 [details]
Bug 1412173 Part 1: Implement the Document ignore-opens-during-unload counter required by spec.

https://reviewboard.mozilla.org/r/194722/#review201662

Thank you for cleaning this up!

::: layout/base/nsDocumentViewer.cpp:1192
(Diff revision 2)
>      return NS_OK;
>    }
>  
>    NS_ASSERTION(nsContentUtils::IsSafeToRunScript(), "This is unsafe");
>  
> +  // Create an RAII object on mDocument that will increment the

I think the key thing to document here is that we want to scope the increment in the counter across all the beforeunload events for everything in this subtree.  A pointer to https://html.spec.whatwg.org/multipage/browsing-the-web.html#prompt-to-unload-a-document steps might not be a bad idea.

::: layout/base/nsDocumentViewer.cpp:1408
(Diff revision 2)
>        return NS_ERROR_NULL_POINTER;
>      }
>  
> +    // Create an RAII object on mDocument that will increment the
> +    // should-ignore-opens-during-unload counter on initialization
> +    // and decrement it again when it goes out of scope.

Again, a pointer to https://html.spec.whatwg.org/multipage/browsing-the-web.html#unload-a-document steps would be good here.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8923595 [details]
Bug 1412173 Part 2: Add a crashtest to check we can handle overlapping document open calls.

https://reviewboard.mozilla.org/r/194724/#review201664

::: dom/html/crashtests/1412173.html:14
(Diff revision 2)
> +  let emptyWindow = window.open('', '', 'just-a-non-empty-string');
> +  o2.endContainer.writeln('<p>end of script</p>');

Again, you shouldn't need this.  See comment 8...

	Brad Werth [:bradwerth] Assignee
	Comment 18 • 7 years ago
mozreview-review-reply

Comment on attachment 8923595 [details]
Bug 1412173 Part 2: Add a crashtest to check we can handle overlapping document open calls.

https://reviewboard.mozilla.org/r/194724/#review201664

> Again, you shouldn't need this.  See comment 8...

Whoops. I thought I had posted an updated test in the last push. The new test does what you suggested, reftest-wait, etc.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8923595 [details]
Bug 1412173 Part 2: Add a crashtest to check we can handle overlapping document open calls.

https://reviewboard.mozilla.org/r/194724/#review202010

r=me assuming this testcase fails pre-patch.

	Pulsebot
	Comment 20 • 7 years ago

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97de1e0cd693
Part 1: Implement the Document ignore-opens-during-unload counter required by spec. r=bz
https://hg.mozilla.org/integration/autoland/rev/68de62dd7a9b
Part 2: Add a crashtest to check we can handle overlapping document open calls. r=bz

	Phil Ringnalda (:philor)
	Comment 21 • 7 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/7a87cbe5d9ad for a unexpected pass in wpt, https://treeherder.mozilla.org/logviewer.html#?job_id=142622865&repo=autoland

	Brad Werth [:bradwerth] Assignee
	Comment 26 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f88391261ad8b5694164012700916832aeb7d8fe

	Code Review Bot [:reviewbot]
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8923594 [details]
Bug 1412173 Part 1: Implement the Document ignore-opens-during-unload counter required by spec.

https://reviewboard.mozilla.org/r/194722/#review202470


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8926063 [details]
Bug 1412173 Part 3: Restore a WPT test to expected pass.

https://reviewboard.mozilla.org/r/197298/#review202694

	Pulsebot
	Comment 29 • 7 years ago

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3bc3d8b72947
Part 1: Implement the Document ignore-opens-during-unload counter required by spec. r=bz
https://hg.mozilla.org/integration/autoland/rev/315b2e8320b2
Part 2: Add a crashtest to check we can handle overlapping document open calls. r=bz
https://hg.mozilla.org/integration/autoland/rev/6e6b4edc850c
Part 3: Restore a WPT test to expected pass. r=bz

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 30 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3bc3d8b72947
https://hg.mozilla.org/mozilla-central/rev/315b2e8320b2
https://hg.mozilla.org/mozilla-central/rev/6e6b4edc850c