Fix a bug in Range::didSplitTextNode() that may yield an invalid Range object.

Fix a bug in Range::didSplitTextNode() that may yield an invalid Range object.

Range::didSplitTextNode() fails to update RangeBoundaryPoint's
m_childBeforeBoundary correctly, if either boundary point is located
immediately after the split text node. This change fixes this bug and adds
a couple of unit tests that make sure text splits are handled correctly.

This is a bug I found while I was investigating on a ClusterFuzz crash. The bug
above was the root cause of the crash. The crash happens in the following way:
  1. Range::surroundContents() removes some nodes during its operation,
     which causes DOMNodeRemoved event to fire *before* surroundContents()
     completes.
  2. A user-supplied event handler does something causing text to split.
  3. Due to the bug above, Range's boundary points may get into an inconsistent
     state; i.e. m_start may be located *after* m_end.
  4. If certain conditions are met, an invalid Range object created during
     Range::surroundContents() causes a crash within checkDeleteExtract().
  5. Sad face.

This change adds a new layout test that reproduces this crash.

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

[Yuta Kitamura, 2014-03-04 06:20:31]

This might be the most ridiculous ClusterFuzz bug
I've ever seen... FWIW, ClusterFuzz's original repro case
was gigantic and I'm kinda proud of being able to reduce
the test into something smaller and more understandable.

Anyway, PTAL.

[yosin_UTC9, 2014-03-04 09:26:29]

ACK

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

https://codereview.chromium.org/178543013/diff/20001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/surroundContents-crash.html:32:
textToBeRemoved.addEventListener('DOMNodeRemoved', function (event) {
nit: Can we use DOM mutation observer instead of infamous DOM mutation event?

BTW, if this bug is caused by DOM mutation event, we should consider to postpone
dispatching  DOM mutation event at end of Range::surroundContents() like
Document::execCommand().

[yosin_UTC9, 2014-03-05 01:09:07]

LGTM

Sorry, I should put LGTM instead of ACK.

[Yuta Kitamura, 2014-03-05 01:15:52]

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

https://codereview.chromium.org/178543013/diff/20001/LayoutTests/fast/dom/Ran...
LayoutTests/fast/dom/Range/surroundContents-crash.html:32:
textToBeRemoved.addEventListener('DOMNodeRemoved', function (event) {
On 2014/03/04 09:26:29, Yoshi wrote:
> nit: Can we use DOM mutation observer instead of infamous DOM mutation event?
> 
> BTW, if this bug is caused by DOM mutation event, we should consider to
postpone
> dispatching  DOM mutation event at end of Range::surroundContents() like
> Document::execCommand().

This crash needs synchronous call within surroundContents(),
so mutation observers cannot be used.

And postponing the events sounds like a good direction;
I'd like to follow up with that in a later patch.

[Yuta Kitamura, 2014-03-05 01:16:09]

Kent-san, can you take a look as an OWNER?

[tkent, 2014-03-05 03:05:26]

On 2014/03/05 01:16:09, Yuta Kitamura wrote:
> Kent-san, can you take a look as an OWNER?

You don't need lgtm from another owner because you are an owner.

[Yuta Kitamura, 2014-03-05 03:11:33]

On 2014/03/05 03:05:26, tkent wrote:
> On 2014/03/05 01:16:09, Yuta Kitamura wrote:
> > Kent-san, can you take a look as an OWNER?
> 
> You don't need lgtm from another owner because you are an owner.

Oh I didn't know that. I was thinking like I was on WebKit.
Anyway, thanks!

[Yuta Kitamura, 2014-03-05 03:11:36]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2014-03-05 03:12:11]

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

[commit-bot: I haz the power, 2014-03-05 03:12:13]

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

[commit-bot: I haz the power, 2014-03-05 03:12:14]

Failed to apply patch for Source/core/dom/Range.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/dom/Range.cpp
  Hunk #2 FAILED at 1767.
  Hunk #3 succeeded at 1783 with fuzz 1 (offset -2 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/dom/Range.cpp.rej

Patch:       Source/core/dom/Range.cpp
Index: Source/core/dom/Range.cpp
diff --git a/Source/core/dom/Range.cpp b/Source/core/dom/Range.cpp
index
53a081d175f13e58a51dee60b31a953d1ef101cc..690e034061c641f35c78bfa457fbb2579cfcbda6
100644
--- a/Source/core/dom/Range.cpp
+++ b/Source/core/dom/Range.cpp
@@ -1457,6 +1457,8 @@ void Range::checkDeleteExtract(ExceptionState&
exceptionState)
         return;
     }
 
+    ASSERT(boundaryPointsValid());
+
     if (!commonAncestorContainer(exceptionState) ||
exceptionState.hadException())
         return;
 
@@ -1765,12 +1767,12 @@ void Range::didMergeTextNodes(NodeWithIndex& oldNode,
unsigned offset)
 
 static inline void boundaryTextNodeSplit(RangeBoundaryPoint& boundary, Text*
oldNode)
 {
-    if (boundary.container() != oldNode)
-        return;
+    Node* boundaryContainer = boundary.container();
     unsigned boundaryOffset = boundary.offset();
-    if (boundaryOffset <= oldNode->length())
-        return;
-    boundary.set(oldNode->nextSibling(), boundaryOffset - oldNode->length(),
0);
+    if (boundary.childBefore() == oldNode)
+        boundary.set(boundaryContainer, boundaryOffset + 1,
oldNode->nextSibling());
+    else if (boundary.container() == oldNode && boundaryOffset >
oldNode->length())
+        boundary.set(oldNode->nextSibling(), boundaryOffset -
oldNode->length(), 0);
 }
 
 void Range::didSplitTextNode(Text* oldNode)
@@ -1783,6 +1785,7 @@ void Range::didSplitTextNode(Text* oldNode)
     ASSERT(oldNode->nextSibling()->isTextNode());
     boundaryTextNodeSplit(m_start, oldNode);
     boundaryTextNodeSplit(m_end, oldNode);
+    ASSERT(boundaryPointsValid());
 }
 
 void Range::expand(const String& unit, ExceptionState& exceptionState)

[Yuta Kitamura, 2014-03-05 03:38:26]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2014-03-05 03:38:46]

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

[commit-bot: I haz the power, 2014-03-05 19:39:44]

Change committed as 168521