513194 - [HTML5]HTML5 parser ends up parsing inline stylesheets twice

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I was doing some profiling of an inline stylesheet parse and noticed that with the HTML5 parser we parse it twice: once from nsDocument::EndUpdate running the UpdateStyleSheet runnable that BindToTree posts and once from nsHTML5TreeOperation::Perform calling nsHTML5Parser::UpdateStyleSheet.

Are we reenabling updates on the stylesheet before we end the update batch wrapped around the BindToTree call?  That's the only way I can explain this behavior....

Comment 1

[Henri Sivonen (:hsivonen)]

(In reply to comment #0)
> Are we reenabling updates on the stylesheet before we end the update batch
> wrapped around the BindToTree call?  That's the only way I can explain this
> behavior....

UpdateStyleSheet is called from within the update batch, yes.

What would be the right way to fix? Should I create the inverse of MOZ_AUTO_DOC_UPDATE that uses a scope for ending an update and reopening it?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> UpdateStyleSheet is called from within the update batch, yes.

That's fine; I'm talking about SetEnableUpdates(PR_TRUE).  That's also called within the update batch?  Can we move it to after somehow?

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to comment #3)
> > UpdateStyleSheet is called from within the update batch, yes.
> 
> That's fine; I'm talking about SetEnableUpdates(PR_TRUE).  That's also called
> within the update batch?  Can we move it to after somehow?

It would be possible to gather all the style sheets that need updating into an nsTArray and defer the calls to SetEnableUpdates(PR_TRUE) out of the update batch.

Does it matter if SetEnableUpdates(PR_TRUE) is taken out of order relative to the other tree ops in the tree op batch?

Comment 5

[Henri Sivonen (:hsivonen)]

Also, is it bad if UpdateStyleSheet is deferred out of the batch also? That is, can I defer all this:

void
nsHtml5TreeOpExecutor::UpdateStyleSheet(nsIContent* aElement)
{
  nsCOMPtr<nsIStyleSheetLinkingElement> ssle(do_QueryInterface(aElement));
  if (ssle) {
    ssle->SetEnableUpdates(PR_TRUE);
    PRBool willNotify;
    PRBool isAlternate;
    nsresult rv = ssle->UpdateStyleSheet(this, &willNotify, &isAlternate);
    if (NS_SUCCEEDED(rv) && willNotify && !isAlternate) {
      ++mPendingSheetCount;
      mScriptLoader->AddExecuteBlocker();
    }
  }
}

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Does it matter if SetEnableUpdates(PR_TRUE) is taken out of order relative to
> the other tree ops in the tree op batch?
...
> Also, is it bad if UpdateStyleSheet is deferred out of the batch also?

Hmm.  Can a later op in the tree op batch be a script insertion?  We'd want to have done the UpdateStyleSheet and execute blocker addition before we get the script to a point where it might run, right?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #6)
> > Does it matter if SetEnableUpdates(PR_TRUE) is taken out of order relative to
> > the other tree ops in the tree op batch?
> ...
> > Also, is it bad if UpdateStyleSheet is deferred out of the batch also?
> 
> Hmm.  Can a later op in the tree op batch be a script insertion?  We'd want to
> have done the UpdateStyleSheet and execute blocker addition before we get the
> script to a point where it might run, right?

Yes, a later operation can be a script insertion. However, scripts are special and don't run from within the update batch, so I'd defer UpdateStyleSheet relative to tree that do run within the update batch. The operations that currently don't run in the batch are scripts, charset switches and EOF.

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to comment #7)
> so I'd defer UpdateStyleSheet
> relative to tree that do run within the update batch.

s/tree/the tree operations/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, deferring the UpdateStyleSheet and SetEnableUpdates to the end of the update batch (or rather right after the end of said batch) seems like the right approach then.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Test case (shows duplicate warning in error console when parsed twice)

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Defer UpdateStyleSheet calls out of the update batch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 408373 [details] [diff] [review]
Defer UpdateStyleSheet calls out of the update batch

Looks fine; can you add your testcase as a mochitest?

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to comment #12)
> (From update of attachment 408373 [details] [diff] [review])
> Looks fine;

Thanks.

> can you add your testcase as a mochitest?

How do I reach the error console from a mochitest?

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/test/test_errorPropagation.html?force=1 has some example code doing that.

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: Another attempt that doesn't work

Dogfooding showed that the previous patch breaks some sites by making styles not apply (e.g. the twitter front page shown when not logged in but not the twitter user pages).

So I thought I'd try to break out the doc update and reopen it. However, the attached patch still causes styles to be parser once from EndUpdate even though updates are enabled only afterwards.

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Better fix

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm a little confused as to the code ordering here now.  The old code ordering (what's currently in the tree) seems to be:

1)  Set attributes
2)  Disable updates
3)  Insert node
4)  Reenable updates and do manual update

with steps 3 and 4 happening inside an update batch, right?

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to comment #17)
> I'm a little confused as to the code ordering here now.  The old code ordering
> (what's currently in the tree) seems to be:
> 
> 1)  Set attributes
> 2)  Disable updates
> 3)  Insert node
> 4)  Reenable updates and do manual update
> 
> with steps 3 and 4 happening inside an update batch, right?

Yes, all those steps (not just 3 and 4) happening inside and update batch.

The code I'm patching is:

0) Incorrectly return without disabling updates early if there are no attributes.
1) Set attributes.
2) Disable updates.
3) Insert node
4) Reenable updates and do manual update

All inside an update batch.

(In reply to comment #0)
> once from nsDocument::EndUpdate running the
> UpdateStyleSheet runnable that BindToTree posts

Is it useful to post the runnable on parser-inserted nodes at all?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Is it useful to post the runnable on parser-inserted nodes at all?

No, but BindToTree doesn't really know that the node is parser-inserted, right?

Comment 20

[Henri Sivonen (:hsivonen)]

(In reply to comment #19)
> > Is it useful to post the runnable on parser-inserted nodes at all?
> 
> No, but BindToTree doesn't really know that the node is parser-inserted, right?

Couldn't it check mUpdatesEnabled?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ah, so you can enable updates right after bind, not after the end of the batch?

Sure, that would work.

Note that you would still not want to call UpdateStyleSheet inside the batch.

Comment 22

[Henri Sivonen (:hsivonen)]

Attachment: Don't call UpdateStyleSheet inside batch, handle nsIParser::Terminate()

(In reply to comment #21)
> Ah, so you can enable updates right after bind, not after the end of the batch?
> 
> Sure, that would work.
> 
> Note that you would still not want to call UpdateStyleSheet inside the batch.

Since there's a need to break out of the update batch anyway, I didn't add code for avoiding the existence of the runnable.

Comment 23

[Henri Sivonen (:hsivonen)]

Attachment: Don't call UpdateStyleSheet inside batch, handle nsIParser::Terminate(), v2

Comment 24

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest

Comment 25

[Henri Sivonen (:hsivonen)]

Attachment: Deal with nsIParser::Terminate() better

Comment 26

[Henri Sivonen (:hsivonen)]

Attachment: Avoid compiler warnings

Comment 27

[Henri Sivonen (:hsivonen)]

The general idea from this patch is needed anyway and will be elaborated upon in the patch for bug 497861.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 410214 [details] [diff] [review]
Mochitest

s/Did't/Didn't/

Add an unregisterListener() under finish()?

With those changes, looks fine.

Comment 29

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest with review comments addressed

Thank you. Attaching the fixed test case patch for reference.

Comment 30

[Henri Sivonen (:hsivonen)]

Code: http://hg.mozilla.org/mozilla-central/rev/2e5220ba8898
Test: http://hg.mozilla.org/mozilla-central/rev/47c00081fcd2