RESOLVED FIXED 89351
HTML parser should queue MutationRecords for its operations
https://bugs.webkit.org/show_bug.cgi?id=89351
Summary HTML parser should queue MutationRecords for its operations
Adam Klein
Reported 2012-06-18 09:22:29 PDT
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.
Attachments
WIP (10.63 KB, patch)
2012-12-21 18:21 PST, Elliott Sprehn
no flags
Patch (10.92 KB, patch)
2013-02-01 14:49 PST, Elliott Sprehn
no flags
Patch (10.74 KB, patch)
2013-02-01 14:51 PST, Elliott Sprehn
no flags
Patch (12.96 KB, patch)
2013-02-06 17:37 PST, Elliott Sprehn
no flags
Patch (16.70 KB, patch)
2013-02-07 14:55 PST, Elliott Sprehn
no flags
Patch for landing (16.66 KB, patch)
2013-02-07 16:10 PST, Elliott Sprehn
no flags
Ojan Vafai
Comment 1 2012-06-18 09:48:55 PDT
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
Comment 2 2012-06-18 10:14:46 PDT
Please be sure to test the performance impact of your change using the html-parser.html PerformanceTest.
Eric Seidel (no email)
Comment 3 2012-06-19 17:05:26 PDT
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
Comment 4 2012-07-16 11:05:30 PDT
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
Comment 5 2012-11-08 02:24:21 PST
Note that this has been added to the HTML spec in http://html5.org/r/7484
Elliott Sprehn
Comment 6 2012-12-05 01:44:40 PST
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
Comment 7 2012-12-05 09:24:01 PST
(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
Comment 8 2012-12-05 10:21:52 PST
(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
Comment 9 2012-12-05 17:54:31 PST
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
Comment 10 2012-12-21 18:21:58 PST
Elliott Sprehn
Comment 11 2012-12-21 18:28:53 PST
(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 12 2012-12-23 11:16:02 PST
Comment on attachment 180581 [details] WIP I assuem you tested the various parser benchmarks (or plan to?) :)
Elliott Sprehn
Comment 13 2013-01-02 10:57:43 PST
(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 14 2013-01-02 14:59:10 PST
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
Comment 15 2013-01-07 17:23:08 PST
(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
Comment 16 2013-01-09 09:50:29 PST
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
Comment 17 2013-01-14 18:25:53 PST
(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 18 2013-01-14 23:01:09 PST
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
Comment 19 2013-01-15 10:35:40 PST
(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
Comment 20 2013-01-15 10:41:53 PST
Just to be clear, Elliot: you are saying there is no perf impact when the are no observers registered, right?
Elliott Sprehn
Comment 21 2013-01-15 10:45:17 PST
(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
Comment 22 2013-01-15 12:47:24 PST
> 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
Comment 23 2013-01-15 13:05:10 PST
We have tests in PerformanceTests/Mutation. You can manually run them by passing --force to run-pert-tests.
WebKit Review Bot
Comment 24 2013-01-15 14:18:31 PST
Comment on attachment 180581 [details] WIP Clearing flags on attachment: 180581 Committed r139790: <http://trac.webkit.org/changeset/139790>
WebKit Review Bot
Comment 25 2013-01-15 14:18:37 PST
All reviewed patches have been landed. Closing bug.
Levi Weintraub
Comment 26 2013-01-15 15:18:26 PST
Re-opened since this is blocked by bug 106948
Elliott Sprehn
Comment 27 2013-01-15 15:21:00 PST
(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
Comment 28 2013-01-22 15:10:39 PST
(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
Comment 29 2013-01-22 17:19:56 PST
(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
Comment 30 2013-02-01 14:44:39 PST
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
Comment 31 2013-02-01 14:49:47 PST
Elliott Sprehn
Comment 32 2013-02-01 14:51:01 PST
Eric Seidel (no email)
Comment 33 2013-02-01 14:57:29 PST
Comment on attachment 186148 [details] Patch Functionally looks fine. Do I need to ask perf questions? :)
Elliott Sprehn
Comment 34 2013-02-01 21:31:53 PST
(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 35 2013-02-01 21:32:35 PST
Comment on attachment 186148 [details] Patch OK.
Elliott Sprehn
Comment 36 2013-02-06 17:37:26 PST
Elliott Sprehn
Comment 37 2013-02-07 14:55:22 PST
WebKit Review Bot
Comment 38 2013-02-07 15:50:22 PST
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
Comment 39 2013-02-07 16:10:35 PST
Created attachment 187192 [details] Patch for landing
WebKit Review Bot
Comment 40 2013-02-07 16:36:55 PST
Comment on attachment 187192 [details] Patch for landing Clearing flags on attachment: 187192 Committed r142204: <http://trac.webkit.org/changeset/142204>
WebKit Review Bot
Comment 41 2013-02-07 16:37:02 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.