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....

(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?

> 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?

(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?

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(); } } }

> 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?

(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.

(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/

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 on attachment 408373 [details] [diff] [review] Defer UpdateStyleSheet calls out of the update batch Looks fine; can you add your testcase as a mochitest?

(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?

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

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.

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?

(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?

> 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?

(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?

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.

(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.

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

Comment on attachment 410214 [details] [diff] [review] Mochitest s/Did't/Didn't/ Add an unregisterListener() under finish()? With those changes, looks fine.

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

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