Last Comment Bug 715112 - Remove duplicate document.close() state tracking
: Remove duplicate document.close() state tracking
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
Depends on: 738614 753278
Blocks: 719285 715103
  Show dependency treegraph
 
Reported: 2012-01-04 05:53 PST by Henri Sivonen (:hsivonen)
Modified: 2012-05-09 16:21 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove the duplicate tracking (26.70 KB, patch)
2012-01-05 02:16 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Remove the duplicate tracking, v2 (26.56 KB, patch)
2012-01-11 00:26 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Remove the duplicate tracking, v3 (26.56 KB, patch)
2012-01-11 05:17 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Remove the duplicate tracking, v4 (27.04 KB, patch)
2012-01-12 01:27 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Remove the duplicate tracking, v5 (27.10 KB, patch)
2012-01-13 00:47 PST, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review
Test case showing DOMContentLoaded improvement (1018 bytes, patch)
2012-01-13 05:53 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Test case showing DOMContentLoaded improvement, with hg add (2.44 KB, patch)
2012-01-13 06:06 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Remove ProcessSCRIPTEndTag (6.22 KB, patch)
2012-01-14 01:08 PST, :Ms2ger (⌚ UTC+1/+2)
hsivonen: review+
Details | Diff | Splinter Review
Test case showing DOMContentLoaded improvement, with number of script elements check (2.54 KB, patch)
2012-01-20 01:07 PST, Henri Sivonen (:hsivonen)
bugs: review+
Details | Diff | Splinter Review

Description Henri Sivonen (:hsivonen) 2012-01-04 05:53:24 PST
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) 2012-01-05 00:40:47 PST
The duplication exists to fix bug 98654 which has regressed in the mean time nonetheless.
Comment 2 Henri Sivonen (:hsivonen) 2012-01-05 02:16:23 PST
Created attachment 586012 [details] [diff] [review]
Remove the duplicate tracking
Comment 3 Henri Sivonen (:hsivonen) 2012-01-11 00:26:46 PST
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.
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-11 04:25:39 PST
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] (vacation Aug 25-28) 2012-01-11 04:26:22 PST
Or is it so that with HTML5 parser that code doesn't ever get executed?
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-01-11 04:49:02 PST
MOZ_NOT_REACHED exists, fwiw. (Note the extra _)
Comment 7 Henri Sivonen (:hsivonen) 2012-01-11 05:17:34 PST
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.
Comment 8 Henri Sivonen (:hsivonen) 2012-01-11 06:04:25 PST
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) 2012-01-12 01:27:44 PST
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.
Comment 10 Henri Sivonen (:hsivonen) 2012-01-13 00:47:40 PST
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.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-01-13 01:52:53 PST
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) 2012-01-13 02:07:45 PST
(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) 2012-01-13 05:53:45 PST
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.
Comment 14 Henri Sivonen (:hsivonen) 2012-01-13 06:06:45 PST
Created attachment 588398 [details] [diff] [review]
Test case showing DOMContentLoaded improvement, with hg add
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-01-14 01:08:20 PST
Created attachment 588610 [details] [diff] [review]
Remove ProcessSCRIPTEndTag

Thought I might as well clean this up a bit more
Comment 16 Henri Sivonen (:hsivonen) 2012-01-15 23:25:32 PST
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] (vacation Aug 25-28) 2012-01-16 04:51:30 PST
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) 2012-01-16 05:04:11 PST
(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) 2012-01-17 03:09:27 PST
(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] (vacation Aug 25-28) 2012-01-19 13:35:59 PST
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] (vacation Aug 25-28) 2012-01-19 14:49:48 PST
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) 2012-01-20 01:07:33 PST
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.
Comment 23 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-20 02:58:05 PST
Comment on attachment 588336 [details] [diff] [review]
Remove the duplicate tracking, v5

But please back out the EndLoad part in case of regressions.
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2012-01-25 07:32:08 PST
Comment on attachment 588610 [details] [diff] [review]
Remove ProcessSCRIPTEndTag

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

Note You need to log in before you can comment on or make changes to this bug.