CANCEL: Make Range::insertNode to validate new end boundary point before using it

Another patch http://crrev.com/443103002 fixed this.

Make Range::insertNode to validate new end boundary point before using it

In |Range::insertNode()|, we use |EventQueueScope| to postpone execution of
event handler during DOM mutation. But, "load" event handler for capturing
phase is executed after |ContainerNode::insertNode| or
|ContainerNode::appendChild| call.

This patch makes |Range::insertNode()| to validate new end boundary point,
which can be modified during |ContainerNode::insertNode| call by event handler,
before using it.

BUG=353329
TEST=LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html

[yosin_UTC9, 2014-07-04 05:09:42]

Could you review this patch?
Thanks in advance.

Another option to fix the issue is calling Range::setEndBeforeChild(Node*,
ExceptionState&) to throw an exception rather than validating to avoid to update
end boundary point?

[Yuta Kitamura, 2014-07-04 07:11:33]

I think EventQueueScope should prevent this case as well...

https://codereview.chromium.org/364293004/diff/1/LayoutTests/fast/dom/Range/s...
File LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html (right):

https://codereview.chromium.org/364293004/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html:8: var
insSurroundContents = false;
"ins"?

https://codereview.chromium.org/364293004/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html:18:
document.addEventListener('load', function() {
Defining both onload and document's "load" handler
looks weird. You should use just one way of defining
event handlers consistently.

https://codereview.chromium.org/364293004/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/364293004/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:904: if (collapsed && newText->parentNode() &&
newText->ownerDocument() == m_ownerDocument)
Is "newText->parentNode()" necessary?

[yosin_UTC9, 2014-07-04 08:45:09]

On 2014/07/04 07:11:33, Yuta Kitamura wrote:
> I think EventQueueScope should prevent this case as well...
Unfortunately, EventQueueScope doesn't postpone "load" event.

[yosin_UTC9, 2014-07-04 08:45:16]

PTAL

https://codereview.chromium.org/364293004/diff/1/LayoutTests/fast/dom/Range/s...
File LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html (right):

https://codereview.chromium.org/364293004/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html:8: var
insSurroundContents = false;
On 2014/07/04 07:11:33, Yuta Kitamura wrote:
> "ins"?

Done.

https://codereview.chromium.org/364293004/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-iframe-crash.html:18:
document.addEventListener('load', function() {
On 2014/07/04 07:11:33, Yuta Kitamura wrote:
> Defining both onload and document's "load" handler
> looks weird. You should use just one way of defining
> event handlers consistently.

Done.

https://codereview.chromium.org/364293004/diff/1/Source/core/dom/Range.cpp
File Source/core/dom/Range.cpp (right):

https://codereview.chromium.org/364293004/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:904: if (collapsed && newText->parentNode() &&
newText->ownerDocument() == m_ownerDocument)
On 2014/07/04 07:11:33, Yuta Kitamura wrote:
> Is "newText->parentNode()" necessary?

Yes, the issue is caused by |newText->parentNode() == nullptr|.
Note: |Node::ownerDocument| is never null. Even if node is removed from
document.

owner document check doesn't relate to the issue. I get rid of it from this
patch.
I'll fix in another patch.

[yosin_UTC9, 2014-07-09 01:28:31]

PTAL