Range.insertNode should verify parent before setting end to it

Source/core/dom/Range.cpp

 /*
  * (C) 1999 Lars Knoll (knoll@kde.org)
  * (C) 2000 Gunnstein Lye (gunnstein@netcom.no)
  * (C) 2000 Frederik Holljen (frederik.holljen@hig.no)
  * (C) 2001 Peter Kelly (pmk@post.com)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2011 Motorola Mobility. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ skipping 876 matching lines @@
         container = m_start.container();
         RefPtrWillBeRawPtr<Text> newText = toText(container)->splitText(m_start.offset(), exceptionState);
         if (exceptionState.hadException())
             return;
 
         container = m_start.container();
         container->parentNode()->insertBefore(newNode.release(), newText.get(), exceptionState);
         if (exceptionState.hadException())
             return;
 
-        if (collapsed)
+        if (collapsed) {
+            if (!newText->parentNode()) {

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

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:50]

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.

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

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:25]

load event is fired.

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

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

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

+                exceptionState.throwDOMException(HierarchyRequestError, "This operation would set range's end to parent with new offset, but there's no parent into which to continue.");
+                return;
+            }
             m_end.setToBeforeChild(*newText);
+        }
     } else {
         RefPtrWillBeRawPtr<Node> lastChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? toDocumentFragment(newNode)->lastChild() : newNode.get();
         if (lastChild && lastChild == m_start.childBefore()) {
             // The insertion will do nothing, but we need to extend the range to include
             // the inserted nodes.
             Node* firstChild = (newNodeType == Node::DOCUMENT_FRAGMENT_NODE) ? toDocumentFragment(newNode)->firstChild() : newNode.get();
             ASSERT(firstChild);
             m_start.setToBeforeChild(*firstChild);
             return;
         }
@@ skipping 793 matching lines @@
 
 void showTree(const blink::Range* range)
 {
     if (range && range->boundaryPointsValid()) {
         range->startContainer()->showTreeAndMark(range->startContainer(), "S", range->endContainer(), "E");
         fprintf(stderr, "start offset: %d, end offset: %d\n", range->startOffset(), range->endOffset());
     }
 }
 
 #endif