Skip to content

GitLab

T
tangled
Commit 0bd91d51 authored by yutak@chromium.org's avatar yutak@chromium.org
Browse files
Options

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

Review URL: https://codereview.chromium.org/178543013

git-svn-id: svn://svn.chromium.org/blink/trunk@168521 bbb929c8-8fbe-4397-9dbb-9b2b20218538
parent 2d75261d
Hide whitespace changes
Inline Side-by-side
Showing with 241 additions and 5 deletions
third_party/WebKit/LayoutTests/fast/dom/Range/surroundContents-crash-expected.txt 0 → 100644
View file @ 0bd91d51
Range::didSplitTextNode() should not yield an invalid Range object nor cause a crash inside surroundContents().
On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
PASS textContainer.childNodes.length is 1
PASS range.startContainer === textToBeSplit is true
PASS range.startOffset is 7
PASS range.endContainer === textContainer is true
PASS range.endOffset is 1
PASS textContainer.childNodes.length is 3
PASS range.startContainer === newTextNode is true
PASS range.startOffset is 1
PASS range.endContainer === textContainer is true
PASS range.endOffset is 3
PASS Did not crash.
PASS successfullyParsed is true
TEST COMPLETE
third_party/WebKit/LayoutTests/fast/dom/Range/surroundContents-crash.html 0 → 100644
View file @ 0bd91d51
<!DOCTYPE html>
<html>
<head>
<title>Range.surroundContents() crash</title>
<script src="../../../resources/js-test.js"></script>
</head>
<body>
<script>
description('Range::didSplitTextNode() should not yield an invalid Range object nor cause a crash inside surroundContents().');
window.jsTestIsAsync = true;
var range;
var textContainer;
var textToBeSplit;
var newTextNode;
function run()
{
textContainer = document.createElement('div');
textToBeSplit = document.createTextNode('SPLITME');
textContainer.appendChild(textToBeSplit);
document.body.appendChild(textContainer);
var surroundParent = document.createElement('div');
var textToBeRemoved = document.createTextNode('I will be removed.');
surroundParent.appendChild(textToBeRemoved);
document.body.appendChild(surroundParent);
// Range.surroundContents(newParent) removes newParent's children during its preprocess phase, thus
// the following event handler is called in the middle of surroundContents() method.
textToBeRemoved.addEventListener('DOMNodeRemoved', function (event) {
shouldEvaluateTo('textContainer.childNodes.length', 1);
shouldBeTrue('range.startContainer === textToBeSplit');
shouldEvaluateTo('range.startOffset', textToBeSplit.length);
shouldBeTrue('range.endContainer === textContainer');
shouldEvaluateTo('range.endOffset', 1);
// A bug in Range::didSplitTextNode() yielded an invalid Range object (m_start is located *after* m_end).
// This leads to a crash if this happens within surroundContents().
textToBeSplit.splitText(textToBeSplit.length - 1);
newTextNode = textToBeSplit.nextSibling;
// To reproduce a crash, there must be something in between split text nodes.
textContainer.insertBefore(document.createElement('span'), newTextNode);
shouldEvaluateTo('textContainer.childNodes.length', 3);
shouldBeTrue('range.startContainer === newTextNode');
shouldEvaluateTo('range.startOffset', newTextNode.length);
shouldBeTrue('range.endContainer === textContainer');
shouldEvaluateTo('range.endOffset', 3);
});
range = new Range();
range.setStart(textToBeSplit, textToBeSplit.length);
range.setEnd(textContainer, 1);
range.surroundContents(surroundParent);
testPassed('Did not crash.');
// Cleanup.
document.body.removeChild(textContainer);
window.finishJSTest();
}
window.setTimeout(run, 0);
</script>
</body>
</html>
third_party/WebKit/Source/core/core.gypi
View file @ 0bd91d51
......@@ -3277,6 +3277,7 @@
'dom/DocumentMarkerControllerTest.cpp',
'dom/DocumentTest.cpp',
'dom/MainThreadTaskRunnerTest.cpp',
'dom/RangeTest.cpp',
'editing/TextIteratorTest.cpp',
'fetch/CachingCorrectnessTest.cpp',
'fetch/ImageResourceTest.cpp',
......
third_party/WebKit/Source/core/dom/Range.cpp
View file @ 0bd91d51
......@@ -1457,6 +1457,8 @@ void Range::checkDeleteExtract(ExceptionState& exceptionState)
return;
}
ASSERT(boundaryPointsValid());
if (!commonAncestorContainer(exceptionState) || exceptionState.hadException())
return;
......@@ -1764,12 +1766,12 @@ void Range::didMergeTextNodes(const 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)
......@@ -1781,6 +1783,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)
......
third_party/WebKit/Source/core/dom/RangeTest.cpp 0 → 100644
View file @ 0bd91d51
// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "config.h"
#include "core/dom/Range.h"
#include "bindings/v8/ExceptionStatePlaceholder.h"
#include "core/dom/Element.h"
#include "core/dom/Text.h"
#include "core/html/HTMLBodyElement.h"
#include "core/html/HTMLDocument.h"
#include "core/html/HTMLElement.h"
#include "core/html/HTMLHtmlElement.h"
#include "wtf/Compiler.h"
#include "wtf/RefPtr.h"
#include "wtf/text/AtomicString.h"
#include <gtest/gtest.h>
using namespace WebCore;
namespace {
class RangeTest : public ::testing::Test {
protected:
virtual void SetUp() OVERRIDE;
HTMLDocument& document() const;
private:
RefPtr<HTMLDocument> m_document;
};
void RangeTest::SetUp()
{
m_document = HTMLDocument::create();
RefPtr<HTMLHtmlElement> html = HTMLHtmlElement::create(*m_document);
html->appendChild(HTMLBodyElement::create(*m_document));
m_document->appendChild(html.release());
}
HTMLDocument& RangeTest::document() const
{
return *m_document;
}
TEST_F(RangeTest, SplitTextNodeRangeWithinText)
{
document().body()->setInnerHTML("1234", ASSERT_NO_EXCEPTION);
Text* oldText = toText(document().body()->firstChild());
RefPtr<Range> range04 = Range::create(document(), oldText, 0, oldText, 4);
RefPtr<Range> range02 = Range::create(document(), oldText, 0, oldText, 2);
RefPtr<Range> range22 = Range::create(document(), oldText, 2, oldText, 2);
RefPtr<Range> range24 = Range::create(document(), oldText, 2, oldText, 4);
oldText->splitText(2, ASSERT_NO_EXCEPTION);
Text* newText = toText(oldText->nextSibling());
EXPECT_TRUE(range04->boundaryPointsValid());
EXPECT_EQ(oldText, range04->startContainer());
EXPECT_EQ(0, range04->startOffset());
EXPECT_EQ(newText, range04->endContainer());
EXPECT_EQ(2, range04->endOffset());
EXPECT_TRUE(range02->boundaryPointsValid());
EXPECT_EQ(oldText, range02->startContainer());
EXPECT_EQ(0, range02->startOffset());
EXPECT_EQ(oldText, range02->endContainer());
EXPECT_EQ(2, range02->endOffset());
// Our implementation always moves the boundary point at the separation point to the end of the original text node.
EXPECT_TRUE(range22->boundaryPointsValid());
EXPECT_EQ(oldText, range22->startContainer());
EXPECT_EQ(2, range22->startOffset());
EXPECT_EQ(oldText, range22->endContainer());
EXPECT_EQ(2, range22->endOffset());
EXPECT_TRUE(range24->boundaryPointsValid());
EXPECT_EQ(oldText, range24->startContainer());
EXPECT_EQ(2, range24->startOffset());
EXPECT_EQ(newText, range24->endContainer());
EXPECT_EQ(2, range24->endOffset());
}
TEST_F(RangeTest, SplitTextNodeRangeOutsideText)
{
document().body()->setInnerHTML("<span id=\"outer\">0<span id=\"inner-left\">1</span>SPLITME<span id=\"inner-right\">2</span>3</span>", ASSERT_NO_EXCEPTION);
Element* outer = document().getElementById(AtomicString::fromUTF8("outer"));
Element* innerLeft = document().getElementById(AtomicString::fromUTF8("inner-left"));
Element* innerRight = document().getElementById(AtomicString::fromUTF8("inner-right"));
Text* oldText = toText(outer->childNodes()->item(2));
RefPtr<Range> rangeOuterOutside = Range::create(document(), outer, 0, outer, 5);
RefPtr<Range> rangeOuterInside = Range::create(document(), outer, 1, outer, 4);
RefPtr<Range> rangeOuterSurroundingText = Range::create(document(), outer, 2, outer, 3);
RefPtr<Range> rangeInnerLeft = Range::create(document(), innerLeft, 0, innerLeft, 1);
RefPtr<Range> rangeInnerRight = Range::create(document(), innerRight, 0, innerRight, 1);
RefPtr<Range> rangeFromTextToMiddleOfElement = Range::create(document(), oldText, 6, outer, 3);
oldText->splitText(3, ASSERT_NO_EXCEPTION);
Text* newText = toText(oldText->nextSibling());
EXPECT_TRUE(rangeOuterOutside->boundaryPointsValid());
EXPECT_EQ(outer, rangeOuterOutside->startContainer());
EXPECT_EQ(0, rangeOuterOutside->startOffset());
EXPECT_EQ(outer, rangeOuterOutside->endContainer());
EXPECT_EQ(6, rangeOuterOutside->endOffset()); // Increased by 1 since a new node is inserted.
EXPECT_TRUE(rangeOuterInside->boundaryPointsValid());
EXPECT_EQ(outer, rangeOuterInside->startContainer());
EXPECT_EQ(1, rangeOuterInside->startOffset());
EXPECT_EQ(outer, rangeOuterInside->endContainer());
EXPECT_EQ(5, rangeOuterInside->endOffset());
EXPECT_TRUE(rangeOuterSurroundingText->boundaryPointsValid());
EXPECT_EQ(outer, rangeOuterSurroundingText->startContainer());
EXPECT_EQ(2, rangeOuterSurroundingText->startOffset());
EXPECT_EQ(outer, rangeOuterSurroundingText->endContainer());
EXPECT_EQ(4, rangeOuterSurroundingText->endOffset());
EXPECT_TRUE(rangeInnerLeft->boundaryPointsValid());
EXPECT_EQ(innerLeft, rangeInnerLeft->startContainer());
EXPECT_EQ(0, rangeInnerLeft->startOffset());
EXPECT_EQ(innerLeft, rangeInnerLeft->endContainer());
EXPECT_EQ(1, rangeInnerLeft->endOffset());
EXPECT_TRUE(rangeInnerRight->boundaryPointsValid());
EXPECT_EQ(innerRight, rangeInnerRight->startContainer());
EXPECT_EQ(0, rangeInnerRight->startOffset());
EXPECT_EQ(innerRight, rangeInnerRight->endContainer());
EXPECT_EQ(1, rangeInnerRight->endOffset());
EXPECT_TRUE(rangeFromTextToMiddleOfElement->boundaryPointsValid());
EXPECT_EQ(newText, rangeFromTextToMiddleOfElement->startContainer());
EXPECT_EQ(3, rangeFromTextToMiddleOfElement->startOffset());
EXPECT_EQ(outer, rangeFromTextToMiddleOfElement->endContainer());
EXPECT_EQ(4, rangeFromTextToMiddleOfElement->endOffset());
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment