This bug is for crash report bp-87858696-6ff3-4e41-909d-a9dc10190509.

Top 10 frames of crashing thread:

0 xul.dll nsHTMLDocument::Open dom/html/nsHTMLDocument.cpp:1029
1 xul.dll static bool mozilla::dom::HTMLDocument_Binding::open dom/bindings/HTMLDocumentBinding.cpp:155
2 xul.dll mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3153
3 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:535
4 xul.dll static bool InternalCall js/src/vm/Interpreter.cpp:590
5 xul.dll static bool Interpret js/src/vm/Interpreter.cpp:3082
6 xul.dll js::RunScript js/src/vm/Interpreter.cpp:423
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:563
8 xul.dll static bool InternalCall js/src/vm/Interpreter.cpp:590
9 xul.dll js::Call js/src/vm/Interpreter.cpp:606

There are 3 crashes (from 2 installations) in nightly 68 with buildid 20190509033505. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 325352.

[1] https://hg.mozilla.org/mozilla-central/rev?node=264fe248bca5

Well, the MOZ_RELEASE_ASSERT(!mParser) is failing...

OK, so we're calling Terminate(), which can land in either nsHtml5Parser::Terminate or nsParser::Terminate (the latter only in the about:blank case, I think).

mParser gets nulled out in Document::EndLoad. So the question is whether there are ways we could fail to reach that, or create a new parser after it nulls out mParser.

In the nsHtml5Parser case, we could fail to reach EndLoad in the following situations:

1. mExecutor->IsComplete() returns true. I'd think in this case we would not be the mParser of the document...
2. mRunsToCompletion is true in nsHtml5TreeOpExecutor::DidBuildModel. Should be possible to open() such a document while its parser is running.
3. nsHtml5TreeOpExecutor's mParser is null after DidBuildModelImpl, etc. Not sure when that can happen.
4. nsHtml5TreeOpExecutor's mStarted is false.

In the nsParser case, we could fail to reach EndLoad like so:

5. The state is NS_ERROR_HTMLPARSER_STOPPARSING.
6. IsComplete() is false in nsParser::DidBuildModel. Not sure what that would mean.
7. mParserContext is null or has an mPrevContext in nsParser::DidBuildModel.

That seems to be it, at first glance.

OK, I have found a way to get into the "mStarted is false" state. That can happen if we do an open() with no write() following it (because it's the first write() that sets the mStarted state). So this testcase:

<iframe></iframe>
<script>
  var doc = frames[0].document;
  doc.open();
  doc.open();
</script>

And indeed, that testcase crashes for me in today's nightly, with exactly this assertion failure: https://crash-stats.mozilla.org/report/index/bbc59e0e-0321-48b5-9c6d-4bffa0190509

Henri, thoughts? Should we be changing something about the parser here, or should I just remove the assert, document when mParser might still not be null, and null it out explicitly?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

OK, I have found a way to get into the "mStarted is false" state. That can happen if we do an open() with no write() following it (because it's the first write() that sets the mStarted state). So this testcase:

<iframe></iframe>
<script>
  var doc = frames[0].document;
  doc.open();
  doc.open();
</script>

And indeed, that testcase crashes for me in today's nightly, with exactly this assertion failure: https://crash-stats.mozilla.org/report/index/bbc59e0e-0321-48b5-9c6d-4bffa0190509

Henri, thoughts? Should we be changing something about the parser here,

We could either make nsHTMLDocument::Open call the parser's document.write overload of nsHtml5Parser::Parse with an empty string as input, which seems hacky, or we could factor the parser starting code there into a separate method, leave a release assert that the parser has been started in its place and have nsHTMLDocument::Open call the new method.

I don't see any obvious downsides with this latter approach, but it's been a while since I've thought about this stuff particularly intensively. Do you see downsides to this latter approach?

or should I just remove the assert, document when mParser might still not be null, and null it out explicitly?

Maybe that would work, but it seems scary and harder to reason about.

[Tracking Requested - why for this release]: we're close to the merge day and the crash volume is quite high for a nightly.

I think we should just back out bug 325352 while I work on a fix for this. Filed bug 1550811 to track that.

The code has mostly moved, but there are a few simplifications:

1. If !GetStreamParser(), then GetChannel() always returns null and hence we
   never set isSrcdoc to true. Which is good, because we don't want to apply the
   special srcdoc-parsing rules to document.open() stuff. So we just pass false
   to setIsSrcdocDocument(): It's the same behavior as before, but a lot clearer.
   I've confirmed that code coverage says the "isSrcdoc =
   NS_IsSrcdocChannel(channel)" line is unreached in our tests.

2. In the document.write-after-document.open case, aContentType is now always
   "text/html" (because that's what document.open sets mContentTypeForWriteCalls
   to. So the block checking for it not being "text/html" was dead code (also
   confirmed via code coverage results) and I'm just removing it.

This is the #2 topcrash in the Windows nightlies of 20190509, with around 100 crashes reported in a considerable number of different installations.

The fix for bug 1550811 is on m-c now and should fix this crash. I'd like to keep this bug open to track the real fix, for after the branchpoint.

Not tracking for 68 given the backout.

https://hg.mozilla.org/mozilla-central/rev/0c1f4aee5de0
https://hg.mozilla.org/mozilla-central/rev/3eef21fc64b7
https://hg.mozilla.org/mozilla-central/rev/68b3e0acc503