Changeset 122139 in webkit

Timestamp:

Jul 9, 2012 12:36:57 PM (12 years ago)

Author:

leandrogracia@chromium.org

Message:

SurroundingText should not advance character iterators 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):
(WebCore::SurroundingText::rangeFromContentOffsets):

Source/WebKit/chromium:

Moving the check for null visible positions to WebCore as it makes no
sense to be in a platform-specific code.

• src/WebSurroundingText.cpp:

(WebKit::WebSurroundingText::initialize):

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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text-expected.txt (modified) (1 diff)
• LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/editing/SurroundingText.cpp (modified) (2 diffs)
• Source/WebKit/chromium/ChangeLog (modified) (1 diff)
• Source/WebKit/chromium/src/WebSurroundingText.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2012-07-09  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        SurroundingText should not advance character iterators 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-09  Rafael Weinstein  <rafaelw@chromium.org>
 

trunk/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text-expected.txt

@@ -16 +16 @@
 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">.</p>', 0, 2) is "."
 PASS successfullyParsed is true
 

trunk/LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html

@@ -41 +41 @@
     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">.</p>\', 0, 2)', '.');
 
     document.body.removeChild(document.getElementById('test'));

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-07-09  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        SurroundingText should not advance character iterators 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):
+        (WebCore::SurroundingText::rangeFromContentOffsets):
+
 2012-07-09  Sudarsana Nagineni  <sudarsana.nagineni@linux.intel.com>
 

trunk/Source/WebCore/editing/SurroundingText.cpp

@@ -44 +44 @@
     : m_positionOffsetInContent(0)
 {
+    if (visiblePosition.isNull())
+        return;
+
     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();
-    if (!Range::create(document, position, forwardIterator.range()->startPosition())->text().length())
+    RefPtr<Range> forwardRange = forwardIterator.range();
+    if (!forwardRange || !Range::create(document, position, forwardRange->startPosition())->text().length()) {
+        ASSERT(forwardRange);
         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());
+    RefPtr<Range> backwardsRange = backwardsIterator.range();
+    if (!backwardsRange) {
+        ASSERT(backwardsRange);
+        return;
+    }
+
+    m_positionOffsetInContent = Range::create(document, backwardsRange->endPosition(), position)->text().length();
+    m_contentRange = Range::create(document, backwardsRange->endPosition(), forwardRange->startPosition());
+    ASSERT(m_contentRange);
 }
 
@@ -66 +81 @@
 
     CharacterIterator iterator(m_contentRange.get());
+
+    ASSERT(!iterator.atEnd());
     iterator.advance(startOffsetInContent);
 
+    ASSERT(iterator.range());
     Position start = iterator.range()->startPosition();
+
+    ASSERT(!iterator.atEnd());
     iterator.advance(endOffsetInContent - startOffsetInContent);
+
+    ASSERT(iterator.range());
     Position end = iterator.range()->startPosition();
+
     return Range::create(start.document(), start, end);
 }

trunk/Source/WebKit/chromium/ChangeLog

@@ +1 @@
+2012-07-09  Leandro Gracia Gil  <leandrogracia@chromium.org>
+
+        SurroundingText should not advance character iterators if they are at end.
+        https://bugs.webkit.org/show_bug.cgi?id=90560
+
+        Reviewed by Ryosuke Niwa.
+
+        Moving the check for null visible positions to WebCore as it makes no
+        sense to be in a platform-specific code.
+
+        * src/WebSurroundingText.cpp:
+        (WebKit::WebSurroundingText::initialize):
+
 2012-07-09  Dana Jansens  <danakj@chromium.org>
 

trunk/Source/WebKit/chromium/src/WebSurroundingText.cpp

@@ -47 +47 @@
         return;
 
-    VisiblePosition visiblePosition(node->renderer()->positionForPoint(static_cast<IntPoint>(hitTestInfo.localPoint())));
-    if (visiblePosition.isNull())
-        return;
-
-    m_private.reset(new SurroundingText(visiblePosition, maxLength));
+    m_private.reset(new SurroundingText(VisiblePosition(node->renderer()->positionForPoint(static_cast<IntPoint>(hitTestInfo.localPoint()))), maxLength));
 }