Character iterators should not advance if they are at end

https://bugs.webkit.org/show_bug.cgi?id=90560

Reviewed by Ryosuke Niwa.

Source/WebCore:

CharacterIterator and BackwardsCharacterIterator try to advance their
internal TextIterator without checking if they already are at end.
This can cause crashes in TextIterator::advance.

Test: platform/chromium/editing/surrounding-text/surrounding-text.html

* editing/SurroundingText.cpp:
(WebCore::SurroundingText::SurroundingText):
* editing/TextIterator.cpp:
(WebCore::CharacterIterator::advance):
(WebCore::BackwardsCharacterIterator::advance):

LayoutTests:

Add a new test case where character iterators are already at end when
trying to advance. This was caught by Chromium's address sanitizer
here: http://code.google.com/p/chromium/issues/detail?id=135705

* platform/chromium/editing/surrounding-text/surrounding-text-expected.txt:
* platform/chromium/editing/surrounding-text/surrounding-text.html:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@121921 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 41e569f..275f45e 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,17 @@
+2012-07-05  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        Character iterators should not advance if they are at end
+        https://bugs.webkit.org/show_bug.cgi?id=90560
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a new test case where character iterators are already at end when
+        trying to advance. This was caught by Chromium's address sanitizer
+        here: http://code.google.com/p/chromium/issues/detail?id=135705
+
+        * platform/chromium/editing/surrounding-text/surrounding-text-expected.txt:
+        * platform/chromium/editing/surrounding-text/surrounding-text.html:
+
 2012-07-05  Alexey Proskuryakov  <ap@apple.com>
 
         [Mac][WK2] Enable HTTPS tests

diff --git a/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text-expected.txt b/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text-expected.txt
index 912c1c5..a0842d0 100644 (file)
--- a/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text-expected.txt
+++ b/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text-expected.txt
@@ -15,6 +15,7 @@ PASS surroundingText('<button>.</button><div id="here">This is <!-- comment --!>
 PASS surroundingText('<button>.</button><div id="here">012345678901234567890123456789</div><button>.</button>', 15, 12) is "901234567890"
 PASS surroundingText('<option>.</option>12345<button id="here">test</button><option>.</option>', 0, 100) is ""
 PASS surroundingText('<option>.</option>12345<button>te<span id="here">st</span></button><option>.</option>', 0, 100) is ""
+PASS surroundingText('<p id="here">.', 0, 2) is "."
 PASS successfullyParsed is true
 
 TEST COMPLETE

diff --git a/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html b/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html
index 6b7f478..7c4b2ae 100644 (file)
--- a/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html
+++ b/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html
@@ -40,6 +40,7 @@ function run() {
     shouldBeEqualToString('surroundingText(\'<button>.</button><div id="here">012345678901234567890123456789</div><button>.</button>\', 15, 12)', '901234567890');
     shouldBeEqualToString('surroundingText(\'<option>.</option>12345<button id="here">test</button><option>.</option>\', 0, 100)', '');
     shouldBeEqualToString('surroundingText(\'<option>.</option>12345<button>te<span id="here">st</span></button><option>.</option>\', 0, 100)', '');
+    shouldBeEqualToString('surroundingText(\'<p id="here">.\', 0, 2)', '.');
 
     document.body.removeChild(document.getElementById('test'));
     finishJSTest();

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index d883e04..3e2ecfa 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2012-07-05  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        Character iterators should not advance if they are at end
+        https://bugs.webkit.org/show_bug.cgi?id=90560
+
+        Reviewed by Ryosuke Niwa.
+
+        CharacterIterator and BackwardsCharacterIterator try to advance their
+        internal TextIterator without checking if they already are at end.
+        This can cause crashes in TextIterator::advance.
+
+        Test: platform/chromium/editing/surrounding-text/surrounding-text.html
+
+        * editing/SurroundingText.cpp:
+        (WebCore::SurroundingText::SurroundingText):
+        * editing/TextIterator.cpp:
+        (WebCore::CharacterIterator::advance):
+        (WebCore::BackwardsCharacterIterator::advance):
+
 2012-07-05  John Mellor  <johnme@chromium.org>
 
         Text Autosizing: Add basic framework

diff --git a/Source/WebCore/editing/SurroundingText.cpp b/Source/WebCore/editing/SurroundingText.cpp
index df9aa7e..cfaedda 100644 (file)
--- a/Source/WebCore/editing/SurroundingText.cpp
+++ b/Source/WebCore/editing/SurroundingText.cpp
@@ -45,7 +45,8 @@ SurroundingText::SurroundingText(const VisiblePosition& visiblePosition, unsigne
 {
     const unsigned halfMaxLength = maxLength / 2;
     CharacterIterator forwardIterator(makeRange(visiblePosition, endOfDocument(visiblePosition)).get(), TextIteratorStopsOnFormControls);
-    forwardIterator.advance(maxLength - halfMaxLength);
+    if (!forwardIterator.atEnd())
+        forwardIterator.advance(maxLength - halfMaxLength);
 
     Position position = visiblePosition.deepEquivalent().parentAnchoredEquivalent();
     Document* document = position.document();
@@ -53,7 +54,8 @@ SurroundingText::SurroundingText(const VisiblePosition& visiblePosition, unsigne
         return;
 
     BackwardsCharacterIterator backwardsIterator(makeRange(startOfDocument(visiblePosition), visiblePosition).get(), TextIteratorStopsOnFormControls);
-    backwardsIterator.advance(halfMaxLength);
+    if (!backwardsIterator.atEnd())
+        backwardsIterator.advance(halfMaxLength);
 
     m_positionOffsetInContent = Range::create(document, backwardsIterator.range()->endPosition(), position)->text().length();
     m_contentRange = Range::create(document, backwardsIterator.range()->endPosition(), forwardIterator.range()->startPosition());

diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp
index 7ca3dbe..d7020a7 100644 (file)
--- a/Source/WebCore/editing/TextIterator.cpp
+++ b/Source/WebCore/editing/TextIterator.cpp
@@ -1406,6 +1406,8 @@ PassRefPtr<Range> CharacterIterator::range() const
 
 void CharacterIterator::advance(int count)
 {
+    ASSERT(!atEnd());
+
     if (count <= 0) {
         ASSERT(count == 0);
         return;
@@ -1514,6 +1516,8 @@ PassRefPtr<Range> BackwardsCharacterIterator::range() const
 
 void BackwardsCharacterIterator::advance(int count)
 {
+    ASSERT(!atEnd());
+
     if (count <= 0) {
         ASSERT(!count);
         return;