Bug 103601 - Corrupted DOM tree during appendChild/insertBefore
Summary: Corrupted DOM tree during appendChild/insertBefore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-28 21:34 PST by Ojan Vafai
Modified: 2012-12-03 09:07 PST (History)
5 users (show)

See Also:


Attachments
testcase (298 bytes, text/html)
2012-11-28 21:34 PST, Ojan Vafai
no flags Details
Patch (7.06 KB, patch)
2012-11-29 17:30 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (7.22 KB, patch)
2012-12-02 23:24 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2012-11-28 21:34:26 PST
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.
Comment 1 Hajime Morrita 2012-11-28 23:15:56 PST
Wow, you beat me.
Comment 2 Hajime Morrita 2012-11-29 00:44:30 PST
Apparently we need https://bugs.webkit.org/show_bug.cgi?id=103571 before fixing this.
Otherwise this will regress speed.
Comment 3 Hajime Morrita 2012-11-29 17:30:32 PST
Created attachment 176858 [details]
Patch
Comment 4 Hajime Morrita 2012-11-29 17:31:35 PST
(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.
Comment 5 Abhishek Arya 2012-11-29 23:05:38 PST
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.
Comment 6 Hajime Morrita 2012-11-29 23:12:50 PST
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
Comment 7 Abhishek Arya 2012-11-29 23:42:58 PST
Comment on attachment 176858 [details]
Patch

It will be great if we can get some perf measurement/results on this.
Comment 8 Hajime Morrita 2012-12-02 23:24:25 PST
Created attachment 177191 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-12-02 23:27:06 PST
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
Comment 10 WebKit Review Bot 2012-12-03 09:07:32 PST
Comment on attachment 177191 [details]
Patch for landing

Clearing flags on attachment: 177191

Committed r136405: <http://trac.webkit.org/changeset/136405>
Comment 11 WebKit Review Bot 2012-12-03 09:07:36 PST
All reviewed patches have been landed.  Closing bug.