Bug 82631 – Delay post-insertion notifications until new DOM tree is complete

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Adam Klein]

EDelay post-insertion notifications until new DOM tree is complete

[Adam Klein]

Created attachment 134626 [details]
Patch

[WebKit Review Bot]

Attachment 134626 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Klein]

Created attachment 135454 [details]
Patch

[Adam Klein]

Created attachment 135658 [details]
Patch

[Adam Klein]

Now ready for review. The big idea here is to ensure that inserting a document fragment doesn't have any side-effects _while the DOM tree is being constructed_.

[Ojan Vafai]

Comment on attachment 135658 [details]
Patch

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

I'm uncomfortably excited about this change. :)

> Source/WebCore/dom/ContainerNode.cpp:169
>          // Due to arbitrary code running in response to a DOM mutation event it's
> -        // possible that "next" is no longer a child of "this".
> -        // It's also possible that "child" has been inserted elsewhere.
> -        // In either of those cases, we'll just stop.
> -        if (next->parentNode() != this)
> -            break;
> -        if (child->parentNode())
> +        // possible that "child" has been inserted elsewhere.

Is the only case this happens now if you're appending a script element?

> Source/WebCore/dom/ContainerNode.cpp:175
>          treeScope()->adoptIfNeeded(child);

Here and below, it might be worth adding a comment that adoptIfNeeded will never fire sync JS events because we've already removed the child from it's existing parent and so there are no DOM mutations involved here.

> Source/WebCore/dom/ContainerNode.cpp:1090
> +        // FIXME: Attachment should be the first operation in this function, but some code

Nit: s/this function/this loop/

[Adam Klein]

Comment on attachment 135658 [details]
Patch

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

>> Source/WebCore/dom/ContainerNode.cpp:169
>> +        // possible that "child" has been inserted elsewhere.
> 
> Is the only case this happens now if you're appending a script element?

No, this is still checking for the case of gaining a parent due to events fired during reparenting, above. I left this here to avoid a second loop.

I think the cleanest thing will be to move this check (and the vector resizing) into collectChildrenAndRemoveFromOldParent, but that can be easily accomplished in a followup change; I wanted to keep the restructuring to a minimum in this one.

>> Source/WebCore/dom/ContainerNode.cpp:175
>>          treeScope()->adoptIfNeeded(child);
> 
> Here and below, it might be worth adding a comment that adoptIfNeeded will never fire sync JS events because we've already removed the child from it's existing parent and so there are no DOM mutations involved here.

TreeScope::adoptIfNeeded doesn't do reparenting (and thus fires no mutation events, ever). Document::adoptNode is what you're thinking of, but that calls removeChild explicitly. I don't think a comment is called for, or if it is, it belongs on adoptIfNeeded, not its callers.

>> Source/WebCore/dom/ContainerNode.cpp:1090
>> +        // FIXME: Attachment should be the first operation in this function, but some code
> 
> Nit: s/this function/this loop/

I think I still mean "this function": why should this happen after childrenChanged?

[WebKit Review Bot]

Comment on attachment 135658 [details]
Patch

Clearing flags on attachment: 135658

Committed r113267: <http://trac.webkit.org/changeset/113267>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Reopen the bug since the patch was rolled out.

[Adam Klein]

After some investigation with Ojan, it looks to be a basic issue with the structure of this code: the code in this patch calls attach() in a separate loop from that which appended the node to the DOM tree. For innerHTML calls with many (thousands) of top-level nodes, this can cause a 2x (or worse) slowdown, presumably due to cache misses the second time through the loop.

Still looking for a nice solution.

[Adam Klein]

Carrying out this change seems to cause an unavoidable perf regression when a DocumentFragment containing many Nodes is added, since it's now necessary to touch each Node twice, once when setting its siblings and parent pointers and then again, later, to notify it of insertion, attach a renderer, etc. Balanced against the perf regression, I don't think this refactoring is worthwhile.