Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove duplicate document.close() state tracking

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
6 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)

27.10 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.22 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
2.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
The duplication exists to fix bug 98654 which has regressed in the mean time nonetheless.
Created attachment 586012 [details] [diff] [review]
Remove the duplicate tracking
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 4

6 years ago
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

6 years ago
Or is it so that with HTML5 parser that code doesn't ever get executed?
MOZ_NOT_REACHED exists, fwiw. (Note the extra _)
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)
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)
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
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!
(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.
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)
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

6 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)
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.
(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.
(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.
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.)
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+

Updated

6 years ago
Attachment #590138 - Flags: review?(bugs) → review+
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: 6 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.