Bug 89351 – HTML parser should queue MutationRecords for its operations

• Component: DOM
• Status: RESOLVED FIXED

[Adam Klein]

Mozilla's implementation of MutationObservers queues MutationRecords for DOM operations executed by the parser, not just those initiated through DOM APIs and editing.  Per http://lists.w3.org/Archives/Public/public-webapps/2012AprJun/1198.html, it seems likely that we'll want to change WebKit to have the same behavior to match Firefox.

[Ojan Vafai]

I think this is probably the right API decision. It's not clear to me when we deliver the records though. Do we deliver them after each time the parser yields? If so, is that exposing too much implementation detail? Do we need to define when mutations are delivered during parsing?

[Adam Barth]

Please be sure to test the performance impact of your change using the html-parser.html PerformanceTest.

[Eric Seidel (no email)]

I think it would make the most sense to deliver them when the parser yields, yes.  Presumably just before any other script runs?  The parser may yield at any arbitrary time, so it seems it would be difficult for someone to end up depending on WebKit's delivery behavior (but given how large the interblägs are, I would not be surprised when some page does...).

[Adam Klein]

Whatever we decide to do here, it should be resolved before unprefixing since it amounts to a fairly noticeable difference with Mozilla's unprefixed version.

[Adam Klein]

Note that this has been added to the HTML spec in http://html5.org/r/7484

[Elliott Sprehn]

I was looking at this recently and my plan is:

Add a ChildListMutationScope to ContainerNode::parserAppendChild, parserInsertBefore and parserRemoveChild.

Add the logic from Element::willModifyAttribute for enqueuing mutation records to Element::parserSetAttributes.

Call MutationObserver::deliverAllMutations() somewhere in HTMLScriptRunner.

HTMLScriptRunner is really confusing to me and it doesn't seem to have a single entry point for all scripts so that'll require more research.

[Adam Klein]

(In reply to comment #6)
> I was looking at this recently and my plan is:

Excellent, if we can get something landed here I think we can (finally) safely drop the prefix and likely drop the ENABLE flag as well.

> Add a ChildListMutationScope to ContainerNode::parserAppendChild, parserInsertBefore and parserRemoveChild.

This sounds like a good first step, but may result in very noisy notifications. There's another approach that more directly involves the parser in keeping something like a ChildListMutationScope (I've got a hacked up beginnings of this in a git client somewhere, and some whiteboard-drawing can probably better explain what I mean). But I think it's probably worth starting with your suggested approach and improving over time (so at least we're spec compliant before we start optimizing).

> Add the logic from Element::willModifyAttribute for enqueuing mutation records to Element::parserSetAttributes.

I'm not sure this is necessary: do we ever call parserSetAttributes after an Element is in the tree?

> Call MutationObserver::deliverAllMutations() somewhere in HTMLScriptRunner.
> 
> HTMLScriptRunner is really confusing to me and it doesn't seem to have a single entry point for all scripts so that'll require more research.

[Elliott Sprehn]

(In reply to comment #7)
> (In reply to comment #6)
> > I was looking at this recently and my plan is:
> 
> Excellent, if we can get something landed here I think we can (finally) safely drop the prefix and likely drop the ENABLE flag as well.
> 
> > Add a ChildListMutationScope to ContainerNode::parserAppendChild, parserInsertBefore and parserRemoveChild.
> 
> This sounds like a good first step, but may result in very noisy notifications. There's another approach that more directly involves the parser in keeping something like a ChildListMutationScope (I've got a hacked up beginnings of this in a git client somewhere, and some whiteboard-drawing can probably better explain what I mean). But I think it's probably worth starting with your suggested approach and improving over time (so at least we're spec compliant before we start optimizing).

Okay.

> > Add the logic from Element::willModifyAttribute for enqueuing mutation records to Element::parserSetAttributes.
> 
> I'm not sure this is necessary: do we ever call parserSetAttributes after an Element is in the tree?

Nope, you're right this isn't needed. In parserSetAttributes we do assert about it:

ASSERT(!inDocument());
ASSERT(!parentNode());

[Adam Barth]

That sounds like a good approach.  One design issue to look out for here is that we'll likely want to move the HTML parser to a background thread at some point (maybe even soon).  Watching for these mutations in the parser* functions should work well in that future.

[Elliott Sprehn]

Created attachment 180581 [details]
WIP

[Elliott Sprehn]

(In reply to comment #10)
> Created an attachment (id=180581) [details]
> Patch

This patch needs more tests for insertBefore and removeChild inside the parser, and also we need to figure out when the mutations are really supposed to be delivered. I can't find reference about all this in the spec.

[Eric Seidel (no email)]

Comment on attachment 180581 [details]
WIP

I assuem you tested the various parser benchmarks (or plan to?) :)

[Elliott Sprehn]

(In reply to comment #12)
> (From update of attachment 180581 [details])
> I assuem you tested the various parser benchmarks (or plan to?) :)

I plan to run the html5-full-parse test.

[Eric Seidel (no email)]

Comment on attachment 180581 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=180581&action=review

The change looks OK.  I just worry about perf, and would like to see the perf numbers before approving. :)

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:297
> +        // FIXME: This may be too agressive as we always deliver mutations at
> +        // every script element, even if it's not ready to execute yet. There's
> +        // unfortuantely no obvious way to tell if prepareScript is going to
> +        // execute the script from out here.

You certainly can tell if it *wont* execute the script. :)  It's harder to tell if it will, since you could technically be racing with the loader.  But detecting synchronously-loaded URLs, or cached-scripts which are ready to execute is doable, and I believe part of that is already done in this code, as we detect if a script is ready to execute somewhere...

[Elliott Sprehn]

(In reply to comment #14)
> (From update of attachment 180581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180581&action=review
> 
> ...
>
> You certainly can tell if it *wont* execute the script. :)  It's harder to tell if it will, since you could technically be racing with the loader.  But detecting synchronously-loaded URLs, or cached-scripts which are ready to execute is doable, and I believe part of that is already done in this code, as we detect if a script is ready to execute somewhere...

Hmm, I couldn't find a trivial way without duplicating the big logic tree in prepareScript to know if it won't execute script. How would you detect this?

[Adam Klein]

I don't think we ought to block this patch on the script execution question. It still seems a strict improvement to just queue the records during parse; whether we then deliver them too often or not often enough can be dealt with as a followup.

[Elliott Sprehn]

(In reply to comment #16)
> I don't think we ought to block this patch on the script execution question. It still seems a strict improvement to just queue the records during parse; whether we then deliver them too often or not often enough can be dealt with as a followup.

I ran the perf tests and I don't see any regressions.

[Eric Seidel (no email)]

Comment on attachment 180581 [details]
WIP

I'm surprised this has no perf impact.  But I guess ojan's hawk-like survey of the perf bots will confirm your testing.  If you update the patch you shoudl consider metnioning your perf testing in teh ChangeLog.

[Elliott Sprehn]

(In reply to comment #18)
> (From update of attachment 180581 [details])
> I'm surprised this has no perf impact.  But I guess ojan's hawk-like survey of the perf bots will confirm your testing.  If you update the patch you shoudl consider metnioning your perf testing in teh ChangeLog.

My understanding is that adamk already perf tuned the ChildListMutationScope so that's it's super fast if there's no mutation observers present so they didn't regress Node::appendChild, insertBefore and removeChild (re: Dromaeo). :)

I'll update the ChangeLog before submitting.

[Rafael Weinstein]

Just to be clear, Elliot: you are saying there is no perf impact when the are no observers registered, right?

[Elliott Sprehn]

(In reply to comment #20)
> Just to be clear, Elliot: you are saying there is no perf impact when the are no observers registered, right?

Correct, we don't have a PerformanceTests/Parser test with MutationObserver's to my knowledge.

[Adam Barth]

> Correct, we don't have a PerformanceTests/Parser test with MutationObserver's to my knowledge.

I'd be curious to know what the perf impact is in the case when mutation observers do exist, but that doesn't need to hold up this patch.

[Ryosuke Niwa]

We have tests in PerformanceTests/Mutation. You can manually run them by passing --force to run-pert-tests.

[WebKit Review Bot]

Comment on attachment 180581 [details]
WIP

Clearing flags on attachment: 180581

Committed r139790: <http://trac.webkit.org/changeset/139790>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

Re-opened since this is blocked by bug 106948

[Elliott Sprehn]

(In reply to comment #26)
> Re-opened since this is blocked by bug 106948

Even tough this went through the commit queue it seems the test consistently fails on all the bots and also locally for leviw. :( Looking into what happened...

[Adam Klein]

(In reply to comment #27)
> (In reply to comment #26)
> > Re-opened since this is blocked by bug 106948
> 
> Even tough this went through the commit queue it seems the test consistently fails on all the bots and also locally for leviw. :( Looking into what happened...

Any idea what went wrong with this? Test looks super-simple...

[Elliott Sprehn]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Re-opened since this is blocked by bug 106948
> > 
> > Even tough this went through the commit queue it seems the test consistently fails on all the bots and also locally for leviw. :( Looking into what happened...
> 
> Any idea what went wrong with this? Test looks super-simple...

Discussed this with adamk and we're going to update this patch to use takeRecords() and reland it, then solve delivery on script execution in another patch as that seems to be the problem.

[Elliott Sprehn]

So funny thing here, the reason this broke was because the MUTATION_OBSERVERS flag was removed and my code was ifdef'ed out so the patch was fine, all the code I added was just turned off. Unfortunately it took me a whole day to realize that none of my code actually runs :(

[Elliott Sprehn]

Created attachment 186147 [details]
Patch

[Elliott Sprehn]

Created attachment 186148 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 186148 [details]
Patch

Functionally looks fine. Do I need to ask perf questions? :)

[Elliott Sprehn]

(In reply to comment #33)
> (From update of attachment 186148 [details])
> Functionally looks fine. Do I need to ask perf questions? :)

Nah, perf is the same as before when I checked and it was fine. I do want to add more tests before we land this though!

[Eric Seidel (no email)]

Comment on attachment 186148 [details]
Patch

OK.

[Elliott Sprehn]

Created attachment 186956 [details]
Patch

[Elliott Sprehn]

Created attachment 187183 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187183 [details]
Patch

Rejecting attachment 187183 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 187183, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16438370

[Elliott Sprehn]

Created attachment 187192 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187192 [details]
Patch for landing

Clearing flags on attachment: 187192

Committed r142204: <http://trac.webkit.org/changeset/142204>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.