715112 - Remove duplicate document.close() state tracking

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

It turns out that nsHTMLDocument maintains document.close() state in addition to the HTML parser maintaining that state.

Maintaining that state clearly belongs in the parser. The duplicate state tracking should be removed from nsHTMLDocument in order to allow nsContentSink simplifications.

Comment 1

[Henri Sivonen (:hsivonen)]

The duplication exists to fix bug 98654 which has regressed in the mean time nonetheless.

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Remove the duplicate tracking

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Remove the duplicate tracking, v2

This patch makes us neither better nor worse off as far as bug 98654 goes.

Let's re-fix bug 98654 in a way that makes sense in the context of the HTML5 spec as a follow-up once it has been figured out what mechanism makes other browsers not have that bug. Filed bug 717180.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 587618 [details] [diff] [review]
Remove the duplicate tracking, v2


> HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement *content,
>                                      bool aMalformed)
>+  NS_NOTREACHED("Old HTMLContentSink is still used to run scripts?");
Could you use something stronger here. MOZ_ASSERT perhaps?

Would be perhaps good to know what has regressed bug 98654, since you're removing the
code which was added to fix that.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Or is it so that with HTML5 parser that code doesn't ever get executed?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

MOZ_NOT_REACHED exists, fwiw. (Note the extra _)

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Remove the duplicate tracking, v3

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 587618 [details] [diff] [review]
> Remove the duplicate tracking, v2
> 
> 
> > HTMLContentSink::ProcessSCRIPTEndTag(nsGenericHTMLElement *content,
> >                                      bool aMalformed)
> >+  NS_NOTREACHED("Old HTMLContentSink is still used to run scripts?");
> Could you use something stronger here. MOZ_ASSERT perhaps?

Used MOZ_NOT_REACHED.

> Would be perhaps good to know what has regressed bug 98654, since you're
> removing the
> code which was added to fix that.

HTML5-compliant insertion point management most likely, but I didn't go back to ancient nightlies to check. I'd expect to use a different fix when re-fixing anyway.

(In reply to Olli Pettay [:smaug] from comment #5)
> Or is it so that with HTML5 parser that code doesn't ever get executed?

The old document.close() tracking code executes, but AFAICT has no observable effect if it is removed. That is, the removed neither affect the test case for bug 98654 nor any in-tree unit tests.

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 587661 [details] [diff] [review]
Remove the duplicate tracking, v3

Whoa. Looks like I broke the patch accidentally on the way.

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Remove the duplicate tracking, v4

Restored wyciwyg channel removal to nsHTMLDocument::Close to keep the code bugwards-compatible with its old state.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Remove the duplicate tracking, v5

Restored the wyciwyg channel removal in nsHTMLDocument::Close. It's bogus but this patch doesn't make it more bogus that it was without this patch.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 588336 [details] [diff] [review]
Remove the duplicate tracking, v5

> nsHTMLDocument::Close()
> {
>-  return NS_OK;
>+  ++mWriteLevel;
>+  nsresult rv = mParser->Parse(EmptyString(), mParser->GetRootContextKey(),
>+                               GetContentTypeInternal(), true);
> ...
>+  return rv;

This rv doesn't seem to be propagated currently, did you intentionally change that?

And thanks a lot for fixing this!

Comment 12

[Henri Sivonen (:hsivonen)]

(In reply to Ms2ger from comment #11)
> This rv doesn't seem to be propagated currently, did you intentionally
> change that?

I thought it was bad form not to propagate rv. It can propagate NS_ERROR_OUT_OF_MEMORY in the case the parser has been marked as broken. Dunno if that matters.

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Test case showing DOMContentLoaded improvement

OK, so there is a testable difference with the patch, but the difference is for the better. This test never finishes without the fix for this bug.

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Test case showing DOMContentLoaded improvement, with hg add

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Remove ProcessSCRIPTEndTag

Thought I might as well clean this up a bit more

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 588610 [details] [diff] [review]
Remove ProcessSCRIPTEndTag

> case eHTMLTag_script:

Please move the MOZ_NOT_REACHED to under this switch-case. With that, r=hsivonen.

Note that if you are interested, it's now safe to remove everything in nsHTMLContentSink.cpp that isn't
 a) used for creating HTML element nodes from all around the app
or
 b) used when building the DOM for about:blank.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Would it be possible to have a patch for Bug 717180. That is so much related to this bug
that would be good to see how that will be fixed.

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #17)
> Would it be possible to have a patch for Bug 717180. That is so much related
> to this bug
> that would be good to see how that will be fixed.

I'm waiting for a spec change on that one. (The spec saying what the fix should be.) I'd rather not just make stuff up while waiting. I'd prefer to proceed with this patch, since this one neither fixes nor breaks the other bug.

Comment 19

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #16)
> Comment on attachment 588610 [details] [diff] [review]
> Remove ProcessSCRIPTEndTag
> 
> > case eHTMLTag_script:
> 
> Please move the MOZ_NOT_REACHED to under this switch-case. With that,
> r=hsivonen.

Oops. My mistake. It's already in the right place. Nothing to see here. r=hsivonen.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 588398 [details] [diff] [review]
Test case showing DOMContentLoaded improvement, with hg add

Should this fire eventually 2 DOMContentLoaded events?
One for the document before.close() and one for the document opened
when more stuff is written?
(The test ofc checks only one)

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 588336 [details] [diff] [review]
Remove the duplicate tracking, v5


>-nsHTMLDocument::ScriptExecuted(nsIScriptElement *aScript)
>-{
>-  if (mWriteState == eNotWriting) {
>-    return;
>-  }
>-
>-  mPendingScripts.RemoveElement(aScript);
>-  if (mPendingScripts.IsEmpty() && mWriteState == ePendingClose) {
>-    // The last pending script just finished, terminate our parser now.
>-    mWriteState = eDocumentClosed;
>-  }
>-}
So does something else now give the same logic what this method has?


(Sorry, this patch is surprisingly hard to review, and I have a gut feeling that this
 might cause regressions.)

Comment 22

[Henri Sivonen (:hsivonen)]

Attachment: Test case showing DOMContentLoaded improvement, with number of script elements check

(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 588398 [details] [diff] [review]
> Test case showing DOMContentLoaded improvement, with hg add
> 
> Should this fire eventually 2 DOMContentLoaded events?

Nope.

> One for the document before.close() and one for the document opened
> when more stuff is written?

The parse doesn't end when document.close() is called. The parser just gets mDocumentClosed set to true. document.close doesn't get to call ParseUntilBlocked(), because the call to document.close is itself document.written, so mInDocumentWrite is true. The parser processes the EOF from the main event loop afterwards.

Attaching an updated version of the test case that checks that the document has 3 script elements when DOMContentLoaded fires.

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 588336 [details] [diff] [review]
> Remove the duplicate tracking, v5
> 
> 
> >-nsHTMLDocument::ScriptExecuted(nsIScriptElement *aScript)
> >-{
> >-  if (mWriteState == eNotWriting) {
> >-    return;
> >-  }
> >-
> >-  mPendingScripts.RemoveElement(aScript);
> >-  if (mPendingScripts.IsEmpty() && mWriteState == ePendingClose) {
> >-    // The last pending script just finished, terminate our parser now.
> >-    mWriteState = eDocumentClosed;
> >-  }
> >-}
> So does something else now give the same logic what this method has?

mDocumentClosed and mInDocumentWrite in nsHtml5Parser.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 588336 [details] [diff] [review]
Remove the duplicate tracking, v5

But please back out the EndLoad part in case of regressions.

Comment 24

[Henri Sivonen (:hsivonen)]

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/994b74a4a909
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b68e3ab05a

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/994b74a4a909
https://hg.mozilla.org/mozilla-central/rev/f2b68e3ab05a

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 588610 [details] [diff] [review]
Remove ProcessSCRIPTEndTag

https://hg.mozilla.org/mozilla-central/rev/90480621f7c5