Bug 103601 – Corrupted DOM tree during appendChild/insertBefore

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ojan Vafai]

Created attachment 176643 [details]
testcase

As noted in bug 103372, we don't properly run all the checks for invalid trees when the DOM is modified during events dispatched on removeChild. Attached is a testcase for appendChild. The current code just checks that the node being inserted has a parentNode, which is not sufficient.

[Hajime Morrita]

Wow, you beat me.

[Hajime Morrita]

Apparently we need https://bugs.webkit.org/show_bug.cgi?id=103571 before fixing this.
Otherwise this will regress speed.

[Hajime Morrita]

Created attachment 176858 [details]
Patch

[Hajime Morrita]

(In reply to comment #3)
> Created an attachment (id=176858) [details]
> Patch
There is no good benchmark to capture Bug 103571 so I'd attack it later and fix this first.

[Abhishek Arya]

Comment on attachment 176858 [details]
Patch

The patch looks good, have a question.
1. How do we have GuaranteedNodeTypes after mutation events in appendChild, insertBefore, But we don't have that guarantee in replaceChild and call checkReplaceChild again ?
2. We should probably add the test for replaceChild as well.

[Hajime Morrita]

Hi Arya, thanks for taking a look!

(In reply to comment #5)
> (From update of attachment 176858 [details])
> The patch looks good, have a question.
> 1. How do we have GuaranteedNodeTypes after mutation events in appendChild, insertBefore, But we don't have that guarantee in replaceChild and call checkReplaceChild again ?

Just for making the patch easy to merge.
I'll post follow up patch to use "guaranteed" version for replaceChild()

> 2. We should probably add the test for replaceChild as well.
We already have one.
http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/mutation-during-replace-child.html

[Abhishek Arya]

Comment on attachment 176858 [details]
Patch

It will be great if we can get some perf measurement/results on this.

[Hajime Morrita]

Created attachment 177191 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177191 [details]
Patch for landing

Rejecting attachment 177191 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

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

[WebKit Review Bot]

Comment on attachment 177191 [details]
Patch for landing

Clearing flags on attachment: 177191

Committed r136405: <http://trac.webkit.org/changeset/136405>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.