516406 - [HTML5] Make document.write() parser and stream parser have distinct tokenizers

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

nsHtml5Parser and nsHtml5StreamParser need to have their own copies of nsHtml5Tokenizer and nsHtml5TreeBuilder.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Backup of WIP

Comment 2

[Henri Sivonen (:hsivonen)]

It turns out that supporting defer scripts the Gecko way is trouble with off-the-main thread parsing. Need to break them here and bring them back with HTML5 compliant behavior in bug 518104.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Backup of WIP. Almost there. Code polish needed.

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 402587 [details] [diff] [review]
Backup of WIP. Almost there. Code polish needed.

Todo: release memory properly from the new tree ops.

Comment 5

[Henri Sivonen (:hsivonen)]

Something here makes BlockOnload() and UnblockOnload() counts not match on www.cnn.com and EndLoad() in nsHTMLDocument is called too early.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Make www.cnn.com less broken

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Fix onload blocking behavior when innerHTML is set during load

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Flush stream parser more often

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Fix merge conflicts caused by earlier patches in the queue

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 404804 [details] [diff] [review]
Fix merge conflicts caused by earlier patches in the queue

This doesn't handle tokenizer state copying just right.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Make state syncing better

At this point, the tokenizer still isn't quite right when someone document.writes a partial doctype token in the middle of the document but now it's harder to backport that parser core fix to this patch, so I'd rather fix it in a follow-up bug.

Comment 12

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #11)
> Created an attachment (id=405485) [details]
> Make state syncing better
> 
> At this point, the tokenizer still isn't quite right when someone
> document.writes a partial doctype token in the middle of the document but now
> it's harder to backport that parser core fix to this patch, so I'd rather fix
> it in a follow-up bug.

Does that mean you still want me to review the latest patch?  (Sorry for the delay.)

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Fix bitrot, backport stuff from ahead in the queue

It would be good if you could review this patch (with the assumption that the state syncing is wrong inside doctype tokens and will get fixed later).

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Fix doctype syncing

I realized there was nothing blocking me from fixing the doctype syncing at this point in the patch queue, so here it is.

Comment 15

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Feedback on "406446: Fix doctype syncing"

Summary:

- nsHtml5StreamParser::mSnapshot can get overwritten before the corresponding tree operation has been delivered, leading to memory corruption.

- Need to add back the call to mExecutor->SetLifeCycle(TERMINATED) in nsHtml5Parser::ParseFragment, I think.

- I'd prefer a little more assurance that uninitialized eHtml5TreeOperations are actually intended to be eTreeOpAppends.  If you agree that this could be a source of bugs, I have an idea for a DEBUG-only wrapper for mOpCode that ensures it is reassigned exactly once.

- The previous patch didn't apply completely cleanly because the aBuilder->HaveNotified(node) expression here

  case eTreeOpDoneAddingChildren: {
    nsIContent* node = *(mOne.node);
    node->DoneAddingChildren(aBuilder->HaveNotified(node));
    return rv;
  }

was simply PR_FALSE.  Since nsHtml5TreeBuilder::HaveNotified appears no longer to exist, and PR_FALSE seems like the safe value to pass, I assume the PR_FALSE version is what you intended, but maybe I messed up the patch sequence.

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Address review comments

(In reply to comment #15)
> Created an attachment (id=406644) [details]
> Feedback on "406446: Fix doctype syncing"
> 
> Summary:
> 
> - nsHtml5StreamParser::mSnapshot can get overwritten before the corresponding
> tree operation has been delivered, leading to memory corruption.

Yeah, sorry, this wasn't well thought out at all, because it was going to go away in a subsequent patch. 

Fixed.

> - Need to add back the call to mExecutor->SetLifeCycle(TERMINATED) in
> nsHtml5Parser::ParseFragment, I think.

Fixed.

> - I'd prefer a little more assurance that uninitialized eHtml5TreeOperations
> are actually intended to be eTreeOpAppends.  If you agree that this could be a
> source of bugs, I have an idea for a DEBUG-only wrapper for mOpCode that
> ensures it is reassigned exactly once.

Fixed by adding a debug-only enum value.

> - The previous patch didn't apply completely cleanly because the
> aBuilder->HaveNotified(node) expression here
> 
>   case eTreeOpDoneAddingChildren: {
>     nsIContent* node = *(mOne.node);
>     node->DoneAddingChildren(aBuilder->HaveNotified(node));
>     return rv;
>   }
> 
> was simply PR_FALSE.  Since nsHtml5TreeBuilder::HaveNotified appears no longer
> to exist, and PR_FALSE seems like the safe value to pass, I assume the PR_FALSE
> version is what you intended, but maybe I messed up the patch sequence.

That happened, because I landed the patch for bug 509851 out of sequence. The patch on this bug is correctly synced. The attached patch on bug 515338 might have a conflict on that line. Conceptually, the change from bug 509851 should be preserved.

I noticed a new problem:

Starting with this patch, framesets are rendered twice. The frameset visible in the viewport appears empty. There's another rendering of the frameset below it (scrolled out of view). The lower frameset rendering renders the frame content. Resizing frames in the upper rendering resizes frames in the lower one, too.

Seems like a double CSS frame construction problem, but I don't yet have no idea what changed in a way that'd cause double CSS frame construction.

Comment 17

[Henri Sivonen (:hsivonen)]

Attachment: Stylistic improvements

Still haven't figured out the frameset double rendering.

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: Really doing the improvements

Wrong patch in the previous upload. Sorry for the spam.

Comment 19

[Henri Sivonen (:hsivonen)]

Attachment: Defer DidBuildModel call out of the doc update

This fixes the frameset issue.

Comment 20

[Ben Newman (:bnewman) (:benjamn)]

These two assertions (I added them) are failing for me on facebook pages:

  diff --git a/parser/html/nsHtml5TreeOpExecutor.cpp b/parser/html/nsHtml5TreeOpExecutor.cpp
  --- a/parser/html/nsHtml5TreeOpExecutor.cpp
  +++ b/parser/html/nsHtml5TreeOpExecutor.cpp
  @@ -437,16 +437,18 @@ nsHtml5TreeOpExecutor::ExecuteScript()
     // need executing.
     nsresult rv = mScriptElement->DoneAddingChildren(PR_TRUE);
     // If the act of insertion evaluated the script, we're fine.
     // Else, block the parser till the script has loaded.
     if (rv == NS_ERROR_HTMLPARSER_BLOCK) {
       mScriptElements.AppendObject(sele);
       mParser->BlockParser();
     } else {
  +    NS_ASSERTION(mLifeCycle != TERMINATED, "Didn't we, like, just check for this?");
  +    NS_ASSERTION(mParser, "Uh-oh.");
       // This may have already happened if the script executed, but in case
       // it didn't then remove the element so that it doesn't get stuck forever.
       htmlDocument->ScriptExecuted(sele);
       static_cast<nsHtml5Parser*> (mParser.get())->MaybePostContinueEvent();
     }
     mScriptElement = nsnull;
   }
   
Any idea if this represents a larger problem, or if the appropriate fix is just to re-check mLifeCycle and/or mParser after script execution?

Comment 21

[Henri Sivonen (:hsivonen)]

Attachment: Add a termination check

(In reply to comment #20)
> These two assertions (I added them) are failing for me on facebook pages:

Added a termination check. It can go away later in the patch queue, though.

> Any idea if this represents a larger problem, or if the appropriate fix is just
> to re-check mLifeCycle and/or mParser after script execution?

It's a larger problem. I posted about it to .platform newsgroup. The general problem is that nsHtml5TreeOpExecutor::Flush() calls out to something and somewhere outside the parser this causes a call back to nsIParser::Terminate(). I don't yet understand the assumptions the rest of Gecko has about parser behavior well enough to know what the best fix is (deferring aspects of termination vs. checking for termination all the time).

I'd like to address that general problem as a follow-up bug after flushing the current patch queue. It's an old problem that isn't introduced by these patches.

Comment 22

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 406986 [details] [diff] [review]
Add a termination check 

Looks good to me!

Comment 23

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #22)
> (From update of attachment 406986 [details] [diff] [review])
> Looks good to me!

Er, now I'm not so sure.  I can reliably trigger

  ###!!! ASSERTION: Had a script element and DidBuildModel call: '!mCallDidBuildModel', file /Users/ben/dev/debug/parser/html/nsHtml5TreeOpExecutor.cpp, line 320

by visiting this page:

  http://people.mozilla.org/~bnewman/bugs/516406/springer.html

Comment 24

[Henri Sivonen (:hsivonen)]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 406986 [details] [diff] [review] [details])
> > Looks good to me!
> 
> Er, now I'm not so sure.  I can reliably trigger
> 
>   ###!!! ASSERTION: Had a script element and DidBuildModel call:
> '!mCallDidBuildModel', file
> /Users/ben/dev/debug/parser/html/nsHtml5TreeOpExecutor.cpp, line 320
> 
> by visiting this page:
> 
>   http://people.mozilla.org/~bnewman/bugs/516406/springer.html

This goes away with the later patches in the queue. I have applied patches for bug 514661, bug 515338, bug 516406, bug 482918, bug 482919, bug 503473, bug 521970 and bug 523087, and I don't see the problem.

Bug 503473 and bug 523087 cover a whole lot of problems.

At this point, I wonder if the the above queue should be treated as an atomic review and landing unit. It seems like a bad use of time to keep backporting stuff to the earlier patches in the queue. Yesterday, I lost some time because I forgot to revert a change I did at comment 16 here but that needed to go away again at bug 482919.

Comment 25

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #24)
> At this point, I wonder if the the above queue should be treated as an atomic
> review and landing unit. It seems like a bad use of time to keep backporting
> stuff to the earlier patches in the queue. Yesterday, I lost some time because
> I forgot to revert a change I did at comment 16 here but that needed to go away
> again at bug 482919.

Yeah, I definitely agree that this should all land at once.  I'll try to do a better job of considering the whole stack when I review.  No need to fold it all together yet, though; I've got all the patches applied in the right order on my machine, and that's easier to manage than one huge patch.

Comment 26

[Henri Sivonen (:hsivonen)]

Thank you for the review.

Landed as http://hg.mozilla.org/mozilla-central/rev/7380c012628e