Range.insertNode should verify parent before setting end to it

Range.insertNode should verify parent before setting end to it

This is because ContainerNode::insertBefore would cause mutation in tree.
So this CL adds code block to throw exception if parent is not avaiable.
In addition, http://crbug.com/399230 will be fixed with this CL.

BUG=399230
TEST=LayoutTests/fast/dom/Range/surroundContents-for-mutation.html

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

[kangil_, 2014-08-07 00:54:54]

PTAL

fyi, trybot failures seem not to be related to this change.

[tkent, 2014-08-07 01:04:36]

-tkent, haraken
+yosin

[Yuta Kitamura, 2014-08-07 09:30:41]

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
File LayoutTests/fast/dom/Range/surroundContents-for-mutation.html (right):

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-for-mutation.html:18: var srcElement
= event.srcElement;
Does this work? You have to receive the event as the first
argument of loaded(), don't you?

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

https://codereview.chromium.org/443103002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
I wonder how newText->parentNode() becomes null.

Let me confirm my understanding to this code:
* At line 888: |container| is a text node, and it is split
  into two text nodes: |container| and |newText|.
* At line 893: |container->parentNode()| (called "P"
  hereafter) exists and is non-null. With the step above
  in mind, |container| and |newText| share the same parent
  node P.
* At line 898: newText->parentNode() should be equal to P,
  which is non-null as far as I understand.

Am I wrong? Is there any place where my assumption does not
hold? I guess insertBefore() may cause some side effect but
I'm not sure how it affects the existence of
newText->parentNode().

[kangil_, 2014-08-07 09:59:51]

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
File LayoutTests/fast/dom/Range/surroundContents-for-mutation.html (right):

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-for-mutation.html:18: var srcElement
= event.srcElement;
On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> Does this work? You have to receive the event as the first
> argument of loaded(), don't you?

Please refer to
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
. It will return the object that fired the event.

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

https://codereview.chromium.org/443103002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> I wonder how newText->parentNode() becomes null.
> 
> Let me confirm my understanding to this code:
> * At line 888: |container| is a text node, and it is split
>   into two text nodes: |container| and |newText|.
> * At line 893: |container->parentNode()| (called "P"
>   hereafter) exists and is non-null. With the step above
>   in mind, |container| and |newText| share the same parent
>   node P.
> * At line 898: newText->parentNode() should be equal to P,
>   which is non-null as far as I understand.
> 
> Am I wrong? Is there any place where my assumption does not
> hold? I guess insertBefore() may cause some side effect but
> I'm not sure how it affects the existence of
> newText->parentNode().

insertBefore() will trigger Container::insertBefore and fire event. So in some
cases, e.g. test case of this CL, Range.surroundsContent will be called while
calling same.

[kangil_, 2014-08-07 12:30:33]

PTAL

fyi, here are tree shown before/after insertBefore() in Range::insertNode.

[LayoutTests/fast/dom/Range/surroundContents-for-mutation.html]

1. range.surroundContents(testFrame) - before:
BODY	0xc70fda10010
	#text	0xc70fda1c010 "\n"
	DIV	0xc70fda101a8 ID="container"
		#text	0xc70fda1c080 "\n    "
		P	0xc70fda10230 ID="start"
			#text	0xc70fda1c0f0 ""
*			#text	0xc70fda1c400 "start"
		#text	0xc70fda1c160 "\n    "
		IFRAME	0xc70fda2c010 ID="test"
		#text	0xc70fda1c1d0 "\n    "
		P	0xc70fda10450 ID="end"
			#text	0xc70fda1c240 "end"
		#text	0xc70fda1c2b0 "\n"
	#text	0xc70fda1c320 "\n"
	SCRIPT	0xc70fda3c010
		#text	0xc70fda1c390 "\nvar index = 0;\nvar range =
document.createRange();\nvar start =
document.getElementById('start');\nrange.setStart(start.firstChild,
0);\nrange.setEnd(start.firstChild, 0);\n\nfunction loaded() {\n    var
srcElement = event.srcElement;\n   
range.surroundContents(document.getElementById('end'));\n   
console.log('[loaded] srcElement : ' + srcElement);\n    srcElement.outerHTML =
'';\n}\ndocument.addEventListener("load", loaded, true);\n\nvar testFrame =
document.getElementById('test');\nrange.surroundContents(testFrame);\n\n"

2. range.surroundContents(document.getElementById('end')) - before:
BODY	0xc70fda10010
	#text	0xc70fda1c010 "\n"
	DIV	0xc70fda101a8 ID="container"
		#text	0xc70fda1c080 "\n    "
		P	0xc70fda10230 ID="start"
			#text	0xc70fda1c0f0 ""
*			#text	0xc70fda1c240 ""
			IFRAME	0xc70fda2c010 ID="test"
			#text	0xc70fda1c400 "start"
		#text	0xc70fda1c160 "\n    "
		#text	0xc70fda1c1d0 "\n    "
		P	0xc70fda10450 ID="end"
		#text	0xc70fda1c2b0 "\n"
	#text	0xc70fda1c320 "\n"
	SCRIPT	0xc70fda3c010
		#text	0xc70fda1c390 "\nvar index = 0;\nvar range =
document.createRange();\nvar start =
document.getElementById('start');\nrange.setStart(start.firstChild,
0);\nrange.setEnd(start.firstChild, 0);\n\nfunction loaded() {\n    var
srcElement = event.srcElement;\n   
range.surroundContents(document.getElementById('end'));\n   
console.log('[loaded] srcElement : ' + srcElement);\n    srcElement.outerHTML =
'';\n}\ndocument.addEventListener("load", loaded, true);\n\nvar testFrame =
document.getElementById('test');\nrange.surroundContents(testFrame);\n\n"

3. range.surroundContents(document.getElementById('end')) - after:
BODY	0xc70fda10010
	#text	0xc70fda1c010 "\n"
	DIV	0xc70fda101a8 ID="container"
		#text	0xc70fda1c080 "\n    "
		P	0xc70fda10230 ID="start"
			#text	0xc70fda1c0f0 ""
			P	0xc70fda10450 ID="end"
*			#text	0xc70fda1c240 ""
			IFRAME	0xc70fda2c010 ID="test"
			#text	0xc70fda1c400 "start"
		#text	0xc70fda1c160 "\n    "
		#text	0xc70fda1c1d0 "\n    "
		#text	0xc70fda1c2b0 "\n"
	#text	0xc70fda1c320 "\n"
	SCRIPT	0xc70fda3c010
		#text	0xc70fda1c390 "\nvar index = 0;\nvar range =
document.createRange();\nvar start =
document.getElementById('start');\nrange.setStart(start.firstChild,
0);\nrange.setEnd(start.firstChild, 0);\n\nfunction loaded() {\n    var
srcElement = event.srcElement;\n   
range.surroundContents(document.getElementById('end'));\n   
console.log('[loaded] srcElement : ' + srcElement);\n    srcElement.outerHTML =
'';\n}\ndocument.addEventListener("load", loaded, true);\n\nvar testFrame =
document.getElementById('test');\nrange.surroundContents(testFrame);\n\n"

4. range.surroundContents(testFrame) - after:
*#text	0xc70fda1c400 "start"

[Yuta Kitamura, 2014-08-08 04:27:55]

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
File LayoutTests/fast/dom/Range/surroundContents-for-mutation.html (right):

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-for-mutation.html:18: var srcElement
= event.srcElement;
On 2014/08/07 09:59:50, kangil_ wrote:
> On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> > Does this work? You have to receive the event as the first
> > argument of loaded(), don't you?
> 
> Please refer to
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> . It will return the object that fired the event.

No, that's not .srcElement I'm talking about. I'm asking
about loaded() function taking no arguments. Where does
|event| come from?

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

https://codereview.chromium.org/443103002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
On 2014/08/07 09:59:50, kangil_ wrote:
> On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> > I wonder how newText->parentNode() becomes null.
> > 
> > Let me confirm my understanding to this code:
> > * At line 888: |container| is a text node, and it is split
> >   into two text nodes: |container| and |newText|.
> > * At line 893: |container->parentNode()| (called "P"
> >   hereafter) exists and is non-null. With the step above
> >   in mind, |container| and |newText| share the same parent
> >   node P.
> > * At line 898: newText->parentNode() should be equal to P,
> >   which is non-null as far as I understand.
> > 
> > Am I wrong? Is there any place where my assumption does not
> > hold? I guess insertBefore() may cause some side effect but
> > I'm not sure how it affects the existence of
> > newText->parentNode().
> 
> insertBefore() will trigger Container::insertBefore and fire event. So in some
> cases, e.g. test case of this CL, Range.surroundsContent will be called while
> calling same.

That's weird. We are within a scope of EventQueueScope, which
means such events are queued and will be fired later. What events
are fired?

[kangil_, 2014-08-08 04:52:26]

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
File LayoutTests/fast/dom/Range/surroundContents-for-mutation.html (right):

https://codereview.chromium.org/443103002/diff/1/LayoutTests/fast/dom/Range/s...
LayoutTests/fast/dom/Range/surroundContents-for-mutation.html:18: var srcElement
= event.srcElement;
On 2014/08/08 04:27:55, Yuta Kitamura wrote:
> On 2014/08/07 09:59:50, kangil_ wrote:
> > On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> > > Does this work? You have to receive the event as the first
> > > argument of loaded(), don't you?
> > 
> > Please refer to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > . It will return the object that fired the event.
> 
> No, that's not .srcElement I'm talking about. I'm asking
> about loaded() function taking no arguments. Where does
> |event| come from?

I will add event argument in next patch.

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

https://codereview.chromium.org/443103002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
On 2014/08/08 04:27:55, Yuta Kitamura wrote:
> On 2014/08/07 09:59:50, kangil_ wrote:
> > On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> > > I wonder how newText->parentNode() becomes null.
> > > 
> > > Let me confirm my understanding to this code:
> > > * At line 888: |container| is a text node, and it is split
> > >   into two text nodes: |container| and |newText|.
> > > * At line 893: |container->parentNode()| (called "P"
> > >   hereafter) exists and is non-null. With the step above
> > >   in mind, |container| and |newText| share the same parent
> > >   node P.
> > > * At line 898: newText->parentNode() should be equal to P,
> > >   which is non-null as far as I understand.
> > > 
> > > Am I wrong? Is there any place where my assumption does not
> > > hold? I guess insertBefore() may cause some side effect but
> > > I'm not sure how it affects the existence of
> > > newText->parentNode().
> > 
> > insertBefore() will trigger Container::insertBefore and fire event. So in
some
> > cases, e.g. test case of this CL, Range.surroundsContent will be called
while
> > calling same.
> 
> That's weird. We are within a scope of EventQueueScope, which
> means such events are queued and will be fired later. What events
> are fired?

load event is fired.

When range.surroundContents(testFrame) is called;
Range::insertNode -> ContainerNode::insertBefore ->
ContainerNode::updateTreeAfterInsertion -> ContainerNode::notifyNodeInserted ->
.... -> EventDispatcher::dispatch

[kangil_, 2014-08-08 05:17:12]

Done. PTAL.

[Yuta Kitamura, 2014-08-08 05:25:07]

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

https://codereview.chromium.org/443103002/diff/1/Source/core/dom/Range.cpp#ne...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
On 2014/08/08 04:52:25, kangil_ wrote:
> On 2014/08/08 04:27:55, Yuta Kitamura wrote:
> > On 2014/08/07 09:59:50, kangil_ wrote:
> > > On 2014/08/07 09:30:41, Yuta Kitamura wrote:
> > > > I wonder how newText->parentNode() becomes null.
> > > > 
> > > > Let me confirm my understanding to this code:
> > > > * At line 888: |container| is a text node, and it is split
> > > >   into two text nodes: |container| and |newText|.
> > > > * At line 893: |container->parentNode()| (called "P"
> > > >   hereafter) exists and is non-null. With the step above
> > > >   in mind, |container| and |newText| share the same parent
> > > >   node P.
> > > > * At line 898: newText->parentNode() should be equal to P,
> > > >   which is non-null as far as I understand.
> > > > 
> > > > Am I wrong? Is there any place where my assumption does not
> > > > hold? I guess insertBefore() may cause some side effect but
> > > > I'm not sure how it affects the existence of
> > > > newText->parentNode().
> > > 
> > > insertBefore() will trigger Container::insertBefore and fire event. So in
> some
> > > cases, e.g. test case of this CL, Range.surroundsContent will be called
> while
> > > calling same.
> > 
> > That's weird. We are within a scope of EventQueueScope, which
> > means such events are queued and will be fired later. What events
> > are fired?
> 
> load event is fired.
> 
> When range.surroundContents(testFrame) is called;
> Range::insertNode -> ContainerNode::insertBefore ->
> ContainerNode::updateTreeAfterInsertion -> ContainerNode::notifyNodeInserted
->
> .... -> EventDispatcher::dispatch

I'm lost somewhere between ContainerNode::notifyNodeInserted and
EventDispatcher::dispatch. Could you elaborate more?

[kangil_, 2014-08-08 05:31:09]

On 2014/08/08 05:25:07, Yuta Kitamura wrote:
> > load event is fired.
> > 
> > When range.surroundContents(testFrame) is called;
> > Range::insertNode -> ContainerNode::insertBefore ->
> > ContainerNode::updateTreeAfterInsertion -> ContainerNode::notifyNodeInserted
> ->
> > .... -> EventDispatcher::dispatch
> 
> I'm lost somewhere between ContainerNode::notifyNodeInserted and
> EventDispatcher::dispatch. Could you elaborate more?

I will attach full call stack soon. Few minutes to come.

[kangil_, 2014-08-08 05:38:10]

#1 0x000002452cf1 blink::EventDispatcher::dispatch()
#2 0x000002451f70 blink::EventDispatchMediator::dispatchEvent()
#3 0x000002452303 blink::EventDispatcher::dispatchEvent()
#4 0x0000023b6e44 blink::Node::dispatchEvent()
#5 0x000002636c56 blink::HTMLFrameOwnerElement::dispatchLoad()
#6 0x000002636c8c blink::HTMLFrameOwnerElement::dispatchLoad()
#7 0x000002aea444 blink::LocalDOMWindow::dispatchLoadEvent()
#8 0x000002aea239 blink::LocalDOMWindow::dispatchWindowLoadEvent()
#9 0x000002aea579 blink::LocalDOMWindow::documentWasClosed()
#10 0x00000230ef6c blink::Document::implicitClose()
#11 0x000002bf6aed blink::FrameLoader::checkCompleted()
#12 0x000002bf558d blink::FrameLoader::finishedParsing()
#13 0x00000231a169 blink::Document::finishedParsing()
#14 0x000002805f41 blink::HTMLConstructionSite::finishedParsing()
#15 0x000002788529 blink::HTMLTreeBuilder::finished()
#16 0x00000274913a blink::HTMLDocumentParser::end()
#17 0x000002744670
blink::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
#18 0x0000027443ac blink::HTMLDocumentParser::prepareToStopParsing()
#19 0x00000274917f blink::HTMLDocumentParser::attemptToEnd()
#20 0x00000274938a blink::HTMLDocumentParser::finish()
#21 0x000002bf0c0c blink::DocumentWriter::end()
#22 0x000002be6e6a blink::DocumentLoader::endWriting()
#23 0x000002be6925 blink::DocumentLoader::finishedLoading()
#24 0x000002be95d6 blink::DocumentLoader::maybeLoadEmpty()
#25 0x000002be9749 blink::DocumentLoader::startLoadingMainResource()
#26 0x000002bf94e6 blink::FrameLoader::loadWithNavigationAction()
#27 0x000002bf8703 blink::FrameLoader::load()
#28 0x00000223539c blink::WebLocalFrameImpl::createChildFrame()
#29 0x0000022949fb blink::FrameLoaderClientImpl::createFrame()
#30 0x00000263737a blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
#31 0x0000063c14b0 blink::HTMLFrameElementBase::openURL()
#32 0x0000063c19ea blink::HTMLFrameElementBase::setNameAndOpenURL()
#33 0x0000063c1a8a
blink::HTMLFrameElementBase::didNotifySubtreeInsertionsToDocument()
#34 0x0000022ed71d blink::ContainerNode::notifyNodeInserted()
#35 0x0000022ed0db blink::ContainerNode::updateTreeAfterInsertion()
#36 0x0000022ec548 blink::ContainerNode::insertBefore()
#37 0x0000023df449 blink::Range::insertNode()
#38 0x0000023e15da blink::Range::surroundContents()
#39 0x00000334ccc4 blink::RangeV8Internal::surroundContentsMethod()
#40 0x00000334a004 blink::RangeV8Internal::surroundContentsMethodCallback()

[Yuta Kitamura, 2014-08-08 06:20:58]

https://codereview.chromium.org/443103002/diff/20001/LayoutTests/fast/dom/Ran...
File LayoutTests/fast/dom/Range/surroundContents-for-mutation.html (right):

https://codereview.chromium.org/443103002/diff/20001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/surroundContents-for-mutation.html:17: function
loaded(ev) {
Urm okay, you were trying to use window.event. I didn't
know it.

As far as I know window.event is non-standard (I think
Firefox does not implement it), so I advise using
the passed event (|ev| in your code) instead of
window.event.

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

https://codereview.chromium.org/443103002/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
OK, I understand that load event must be fired synchronously.

Could you add some comment about this, since it's tricky to
understand why newText->parentNode() can become null:
* The load event is fired regardless of EventQueueScope; and
* The event handler may mutate the tree so newText->parentNode()
  may become null.

[kangil_, 2014-08-08 06:32:50]

PTAL

https://codereview.chromium.org/443103002/diff/20001/LayoutTests/fast/dom/Ran...
File LayoutTests/fast/dom/Range/surroundContents-for-mutation.html (right):

https://codereview.chromium.org/443103002/diff/20001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/surroundContents-for-mutation.html:17: function
loaded(ev) {
On 2014/08/08 06:20:58, Yuta Kitamura wrote:
> Urm okay, you were trying to use window.event. I didn't
> know it.
> 
> As far as I know window.event is non-standard (I think
> Firefox does not implement it), so I advise using
> the passed event (|ev| in your code) instead of
> window.event.

Done.

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

https://codereview.chromium.org/443103002/diff/20001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:898: if (!newText->parentNode()) {
On 2014/08/08 06:20:58, Yuta Kitamura wrote:
> OK, I understand that load event must be fired synchronously.
> 
> Could you add some comment about this, since it's tricky to
> understand why newText->parentNode() can become null:
> * The load event is fired regardless of EventQueueScope; and
> * The event handler may mutate the tree so newText->parentNode()
>   may become null.

Done.

[yosin_UTC9, 2014-08-08 06:49:30]

Sorry for later response. I tried to fix this one but it is suspended by another
task, https://codereview.chromium.org/364293004/

Thanks for fixing.

Issue 35329 is as same as this issue 399230.

[kangil_, 2014-08-08 07:16:34]

On 2014/08/08 06:49:30, Yosi_UTC9 wrote:
> Sorry for later response. I tried to fix this one but it is suspended by
another
> task, https://codereview.chromium.org/364293004/
> 
> Thanks for fixing.
> 
> Issue 35329 is as same as this issue 399230.

Thanks for the information.
I will add you as well as yutak for my every contribution to Range. :)

[Yuta Kitamura, 2014-08-08 09:17:34]

lgtm after minor revision

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

https://codereview.chromium.org/443103002/diff/40001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:898: // The event would be fired regardless of
EventQueueScope;
Please be clear that this is for the load event.

Other events will be delayed by EventQueueScope via
dispatchScopedEvent().

[kangil_, 2014-08-08 10:47:13]

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

https://codereview.chromium.org/443103002/diff/40001/Source/core/dom/Range.cp...
Source/core/dom/Range.cpp:898: // The event would be fired regardless of
EventQueueScope;
On 2014/08/08 09:17:34, Yuta Kitamura wrote:
> Please be clear that this is for the load event.
> 
> Other events will be delayed by EventQueueScope via
> dispatchScopedEvent().

Done.

[kangil_, 2014-08-08 10:47:43]

Thanks for reviewing this CL! :)

[kangil_, 2014-08-08 10:47:50]

The CQ bit was checked by kangil.han@samsung.com

[commit-bot: I haz the power, 2014-08-08 10:48:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kangil.han@samsung.com/443103002/60001

[commit-bot: I haz the power, 2014-08-08 12:14:02]

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

[commit-bot: I haz the power, 2014-08-08 12:52:45]

Change committed as 179828