parserInsertBefore and parserRemoveChild should check newChild for a parent.

parserInsertBefore and parserRemoveChild should check newChild for a parent.

parserRemoveChild can run script in obscure cases involving the adoption
agency changing the children of a script element, this script can then
move the element the parser is trying to insert back into the page so that
parserInsertBefore would then insert the newChild in the tree again. This
means the child ends up being inserted twice which can result in a use
after free if one is removed and a GC happens.

To fix this we run the removal in a loop inside the insert methods until
the child really is removed. We probably want to file a spec bug about this
too.

BUG=478745
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194835

[esprehn, 2015-05-01 01:13:46]

esprehn@chromium.org changed reviewers:
+ adamk@chromium.org, inferno@chromium.org

[esprehn, 2015-05-01 01:13:46]

This is not the most beautiful fix, but it works.

[inferno, 2015-05-01 01:28:21]

lgtm, thanks for the fix.

[esprehn, 2015-05-01 01:33:35]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-01 01:33:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117973003/1

[esprehn, 2015-05-01 02:49:03]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2015-05-01 03:23:57]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[esprehn, 2015-05-01 03:23:58]

The patchset sent to the CQ was uploaded after l-g-t-m from inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/1117973003/#ps20001
(title: "Remove assert.")

[commit-bot: I haz the power, 2015-05-01 03:24:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117973003/20001

[commit-bot: I haz the power, 2015-05-01 05:33:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-01 05:33:43]

Dry run: This issue passed the CQ dry run.

[esprehn, 2015-05-01 05:36:58]

https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (left):

https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:755: ASSERT(!newChild->parentNode()); // Use
appendChild if you need to handle reparenting (and want DOM mutation events).
Funny enough this ASSERT wasn't in parserInsertBefore, only parserAppendChild...

[inferno, 2015-05-01 05:46:55]

On 2015/05/01 05:36:58, esprehn wrote:
>
https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
> File Source/core/dom/ContainerNode.cpp (left):
> 
>
https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
> Source/core/dom/ContainerNode.cpp:755: ASSERT(!newChild->parentNode()); // Use
> appendChild if you need to handle reparenting (and want DOM mutation events).
> Funny enough this ASSERT wasn't in parserInsertBefore, only
parserAppendChild...

If this ASSERT in parserInsertBefore, then we would have caught this in fuzzing.
These days ASSERT bugs are filed by Anantha's team, but i dont think people pay
enough attention to important asserts.

[adamk, 2015-05-01 16:46:54]

https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (left):

https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:755: ASSERT(!newChild->parentNode()); // Use
appendChild if you need to handle reparenting (and want DOM mutation events).
On 2015/05/01 05:36:58, esprehn wrote:
> Funny enough this ASSERT wasn't in parserInsertBefore, only
parserAppendChild...

It's in insertBeforeCommon.

https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/1117973003/diff/20001/Source/core/dom/Contain...
Source/core/dom/ContainerNode.cpp:292: void
ContainerNode::parserInsertBefore(PassRefPtrWillBeRawPtr<Node> newChild, Node&
nextChild)
Please update the comment in ContainerNode.h, as it claims that this and
parserAppendChild don't handle reparenting.

[esprehn, 2015-05-01 18:05:12]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2015-05-01 18:05:12]

The patchset sent to the CQ was uploaded after l-g-t-m from inferno@chromium.org
Link to the patchset: https://codereview.chromium.org/1117973003/#ps40001
(title: "Update comment.")

[commit-bot: I haz the power, 2015-05-01 18:05:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1117973003/40001

[commit-bot: I haz the power, 2015-05-02 01:30:15]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194835