The default bug view has changed. See this FAQ.

Remove duplicate document.close() state tracking

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
The duplication exists to fix bug 98654 which has regressed in the mean time nonetheless.
(Assignee)

Comment 2

5 years ago
Created attachment 586012 [details] [diff] [review]
Remove the duplicate tracking
(Assignee)

Comment 3

5 years ago
Created attachment 587618 [details] [diff] [review]
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.
Attachment #586012 - Attachment is obsolete: true
Attachment #587618 - Flags: review?(bugs)
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.
Or is it so that with HTML5 parser that code doesn't ever get executed?
MOZ_NOT_REACHED exists, fwiw. (Note the extra _)
(Assignee)

Comment 7

5 years ago
Created attachment 587661 [details] [diff] [review]
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.
Attachment #587618 - Attachment is obsolete: true
Attachment #587618 - Flags: review?(bugs)
Attachment #587661 - Flags: review?(bugs)
(Assignee)

Comment 8

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

Whoa. Looks like I broke the patch accidentally on the way.
Attachment #587661 - Flags: review?(bugs)
(Assignee)

Comment 9

5 years ago
Created attachment 587971 [details] [diff] [review]
Remove the duplicate tracking, v4

Restored wyciwyg channel removal to nsHTMLDocument::Close to keep the code bugwards-compatible with its old state.
Attachment #587661 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 588336 [details] [diff] [review]
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.
Attachment #587971 - Attachment is obsolete: true
Attachment #588336 - Flags: review?(bugs)
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!
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 588395 [details] [diff] [review]
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.
Attachment #588395 - Flags: review?(bugs)
(Assignee)

Comment 14

5 years ago
Created attachment 588398 [details] [diff] [review]
Test case showing DOMContentLoaded improvement, with hg add
Attachment #588395 - Attachment is obsolete: true
Attachment #588395 - Flags: review?(bugs)
Attachment #588398 - Flags: review?(bugs)

Updated

5 years ago
Attachment #588398 - Attachment is patch: true
Created attachment 588610 [details] [diff] [review]
Remove ProcessSCRIPTEndTag

Thought I might as well clean this up a bit more
Attachment #588610 - Flags: review?(hsivonen)
(Assignee)

Comment 16

5 years ago
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.
Attachment #588610 - Flags: review?(hsivonen) → review+
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.
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 719285
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 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.)
(Assignee)

Comment 22

5 years ago
Created attachment 590138 [details] [diff] [review]
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.
Attachment #588398 - Attachment is obsolete: true
Attachment #588398 - Flags: review?(bugs)
Attachment #590138 - Flags: review?(bugs)
Comment on attachment 588336 [details] [diff] [review]
Remove the duplicate tracking, v5

But please back out the EndLoad part in case of regressions.
Attachment #588336 - Flags: review?(bugs) → review+
Attachment #590138 - Flags: review?(bugs) → review+
(Assignee)

Comment 24

5 years ago
Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/994b74a4a909
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b68e3ab05a
https://hg.mozilla.org/mozilla-central/rev/994b74a4a909
https://hg.mozilla.org/mozilla-central/rev/f2b68e3ab05a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Comment on attachment 588610 [details] [diff] [review]
Remove ProcessSCRIPTEndTag

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

Updated

5 years ago
Depends on: 738614

Updated

5 years ago
Depends on: 753278
You need to log in before you can comment on or make changes to this bug.