Call insertedInto or removedFrom before childrenChanged

Call insertedInto or removedFrom before childrenChanged

We must notify nodes that they were removed before calling childrenChanged
because childrenChanged could run script. If don't then the script can remove
the parent and then Element::removedFrom doesn't think the parent is
inTreeScope or inDocument so it'll fail to clean up the TreeScope hash maps
like the id map.

I tried this once before for a different reason in:
https://src.chromium.org/viewvc/blink?revision=175732&view=revision
but that got rolled out because it caused crashes http://crbug.com/382160

By looking at the code it appears that the reason for this is that
m_element in the WebPluginContainerImpl is a raw ptr, and so nothing is
keeping the element alive inside the UpdateSuspendScope. In this patch
I didn't remove the NodeVector, a future patch will attempt to do that.

Unfortunately even when I did remove the NodeVector I couldn't reproduce
the crashes mentioned in the bug, but by code inspection and the crash
stacks it appears to be the situation I described.

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

[adamk, 2014-07-24 21:35:59]

https://codereview.chromium.org/418133003/diff/1/LayoutTests/fast/dom/script-...
File LayoutTests/fast/dom/script-remove-child-id-map.html (right):

https://codereview.chromium.org/418133003/diff/1/LayoutTests/fast/dom/script-...
LayoutTests/fast/dom/script-remove-child-id-map.html:5: function gc()
Please include ../../resources/gc.js instead of rolling your own. Or myou could
just use js-test.js and avoid having to do this and the dumpAsText call below.

https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:756: RefPtrWillBeRawPtr<Node> protect(this);
What's this about?

https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:786: // ShadowRoots are not real children, we
don't need to tell host that it's
Why are you changing this behavior in this patch? It seems unrelated...

[esprehn, 2014-07-24 22:50:19]

https://codereview.chromium.org/418133003/diff/1/LayoutTests/fast/dom/script-...
File LayoutTests/fast/dom/script-remove-child-id-map.html (right):

https://codereview.chromium.org/418133003/diff/1/LayoutTests/fast/dom/script-...
LayoutTests/fast/dom/script-remove-child-id-map.html:5: function gc()
done.

https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
File Source/core/dom/ContainerNode.cpp (right):

https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:756: RefPtrWillBeRawPtr<Node> protect(this);
Nothing is protecting |this| in these parser methods like in the non-parser ones
even though they run script. I added the protects just like in my original patch
(which you also reviewed). I guess I can leave that out?

https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
Source/core/dom/ContainerNode.cpp:786: // ShadowRoots are not real children, we
don't need to tell host that it's
I'm not changing any behavior, I moved childrenChanged into notifyNodeInserted
which means we'd call childrenChanged on ShadowRoot's since
ElementShadow::addShadowRoot calls notifyNodeInserted(), but calling
childrenChanged on a ShadowRoot right when it's created is not correct and would
change behavior.

[esprehn, 2014-07-24 22:51:13]

On 2014/07/24 at 22:50:19, esprehn wrote:
> ..
> 
>
https://codereview.chromium.org/418133003/diff/1/Source/core/dom/ContainerNod...
> Source/core/dom/ContainerNode.cpp:786: // ShadowRoots are not real children,
we don't need to tell host that it's
> I'm not changing any behavior, I moved childrenChanged into notifyNodeInserted
which means we'd call childrenChanged on ShadowRoot's since
ElementShadow::addShadowRoot calls notifyNodeInserted(), but calling
childrenChanged on a ShadowRoot right when it's created is not correct and would
change behavior.

It also crashes IIRC.

[adamk, 2014-07-24 23:45:14]

lgtm

Thanks for the explanation of the ShadowRoot code, I didn't realize it was
getting called from elsewhere.

[esprehn, 2014-07-24 23:49:54]

On 2014/07/24 at 23:45:14, adamk wrote:
> lgtm
> 
> Thanks for the explanation of the ShadowRoot code, I didn't realize it was
getting called from elsewhere.

Yeah I have plans to fix that, that's what the FIXME is about.

[esprehn, 2014-07-25 00:56:10]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-07-25 00:56:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/esprehn@chromium.org/418133003/20001

[commit-bot: I haz the power, 2014-07-25 02:27:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[esprehn, 2014-07-25 03:32:12]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2014-07-25 03:32:57]

On 2014/07/25 at 03:32:12, esprehn wrote:
> The CQ bit was unchecked by esprehn@chromium.org

Sigh, looks like things have changed since the last time I tried this, now I
fail an assert in HTMLSelectElement:

STDERR: ASSERTION FAILED: items == m_listItems
STDERR: ../../third_party/WebKit/Source/core/html/HTMLSelectElement.cpp(749) :
const WTF::Vector<WTF::RawPtr<HTMLElement> >
&blink::HTMLSelectElement::listItems() const

in all the tests. It does seem to pass the tests though. :/

[esprehn, 2014-07-25 22:07:50]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-07-25 22:08:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/esprehn@chromium.org/418133003/40001

[commit-bot: I haz the power, 2014-07-25 22:55:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/17168)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/19027)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-25 23:03:37]

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

[commit-bot: I haz the power, 2014-07-25 23:03:38]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[esprehn, 2014-07-26 00:35:39]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-07-26 00:36:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/esprehn@chromium.org/418133003/40001

[commit-bot: I haz the power, 2014-07-26 05:11:47]

Change committed as 178976