Changeset 166245 in webkit

Timestamp:

Mar 25, 2014 1:15:14 PM (10 years ago)

Author:

mmaxfield@apple.com

Message:

InlineIterator position (unsigned int) variable can wrap around
​https://bugs.webkit.org/show_bug.cgi?id=130540

Patch by Myles C. Maxfield <​mmaxfield@apple.com> on 2014-03-25
Reviewed by Simon Fraser.

Source/WebCore:

We trigger an ASSERT that occurs when we are ignoring spaces (to collapse them
into a single whitespace mark) but then encounter a line break. Because we don't ignore
the first space (but do ignore subsequent spaces), when we hit a newline in an RTL context
we want to ignore that first space as well (so as not to push the text away from the right
edge). We do this by decrementing the InlineIterator pointing to this first space, so all
the spaces get ignored. However, if that space is the first character in a Text node, the
decrement will try to go past the beginning of the node, and trigger an ASSERT.

This design is not great. At some point we should rework it to more elegantly handle
collapsing whitespace in both RTL and LTR writing modes.

This patch adds an ASSERT earlier in this codepath to catch potential problems earlier.
It also pulls our sentinel value out into a separate boolean to make it more clear what is
going on.

Test: fast/text/whitespace-only-text-in-rtl.html

• rendering/InlineIterator.h:

(WebCore::InlineIterator::moveTo): Use the set*() calls
(WebCore::InlineIterator::setOffset): ASSERT early that our math hasn't wrapped

• rendering/RenderBlockLineLayout.cpp:

(WebCore::RenderBlockFlow::appendRunsForObject): Use new boolean value

• rendering/line/BreakingContextInlineHeaders.h:

(WebCore::BreakingContext::handleText): Guard against wraps
(WebCore::checkMidpoints): Use new boolean value

• rendering/line/TrailingObjects.cpp:

(WebCore::TrailingObjects::updateMidpointsForTrailingBoxes): Use new boolean value

LayoutTests:

This test triggers an ASSERT that occurs when we are ignoring spaces (to collapse them
into a single whitespace mark) but then encounter a line break. Because we don't ignore
the first space (but do ignore subsequent spaces), when we hit a newline in an RTL context
we want to ignore that first space as well (so as not to push the text away from the right
edge). We do this by decrementing the InlineIterator pointing to this first space, so all
the spaces get ignored. However, if that space is the first character in a Text node, the
decrement will try to go past the beginning of the node, and trigger an ASSERT.

This design is not great. At some point we should rework it to more elegantly handle
collapsing whitespace in both RTL and LTR writing modes.

• fast/text/whitespace-only-text-in-rtl-expected.txt: Added.
• fast/text/whitespace-only-text-in-rtl.html: Added.

Conflicts:

LayoutTests/ChangeLog
Source/WebCore/ChangeLog

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/text/whitespace-only-text-in-rtl-expected.txt (added)
• LayoutTests/fast/text/whitespace-only-text-in-rtl.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/InlineIterator.h (modified) (7 diffs)
• Source/WebCore/rendering/RenderBlockLineLayout.cpp (modified) (1 diff)
• Source/WebCore/rendering/line/BreakingContextInlineHeaders.h (modified) (2 diffs)
• Source/WebCore/rendering/line/TrailingObjects.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2014-03-25  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        InlineIterator position (unsigned int) variable can wrap around
+        https://bugs.webkit.org/show_bug.cgi?id=130540
+
+        Reviewed by Simon Fraser.
+
+        This test triggers an ASSERT that occurs when we are ignoring spaces (to collapse them
+        into a single whitespace mark) but then encounter a line break. Because we don't ignore
+        the first space (but do ignore subsequent spaces), when we hit a newline in an RTL context
+        we want to ignore that first space as well (so as not to push the text away from the right
+        edge). We do this by decrementing the InlineIterator pointing to this first space, so all
+        the spaces get ignored. However, if that space is the first character in a Text node, the
+        decrement will try to go past the beginning of the node, and trigger an ASSERT.
+
+        This design is not great. At some point we should rework it to more elegantly handle
+        collapsing whitespace in both RTL and LTR writing modes.
+
+        * fast/text/whitespace-only-text-in-rtl-expected.txt: Added.
+        * fast/text/whitespace-only-text-in-rtl.html: Added.
+
 2014-03-25  Oliver Hunt  <oliver@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-03-25  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        InlineIterator position (unsigned int) variable can wrap around
+        https://bugs.webkit.org/show_bug.cgi?id=130540
+
+        Reviewed by Simon Fraser.
+
+        We trigger an ASSERT that occurs when we are ignoring spaces (to collapse them
+        into a single whitespace mark) but then encounter a line break. Because we don't ignore
+        the first space (but do ignore subsequent spaces), when we hit a newline in an RTL context
+        we want to ignore that first space as well (so as not to push the text away from the right
+        edge). We do this by decrementing the InlineIterator pointing to this first space, so all
+        the spaces get ignored. However, if that space is the first character in a Text node, the
+        decrement will try to go past the beginning of the node, and trigger an ASSERT.
+
+        This design is not great. At some point we should rework it to more elegantly handle
+        collapsing whitespace in both RTL and LTR writing modes.
+
+        This patch adds an ASSERT earlier in this codepath to catch potential problems earlier.
+        It also pulls our sentinel value out into a separate boolean to make it more clear what is
+        going on.
+
+        Test: fast/text/whitespace-only-text-in-rtl.html
+
+        * rendering/InlineIterator.h:
+        (WebCore::InlineIterator::moveTo): Use the set***() calls
+        (WebCore::InlineIterator::setOffset): ASSERT early that our math hasn't wrapped
+        * rendering/RenderBlockLineLayout.cpp:
+        (WebCore::RenderBlockFlow::appendRunsForObject): Use new boolean value
+        * rendering/line/BreakingContextInlineHeaders.h:
+        (WebCore::BreakingContext::handleText): Guard against wraps
+        (WebCore::checkMidpoints): Use new boolean value
+        * rendering/line/TrailingObjects.cpp:
+        (WebCore::TrailingObjects::updateMidpointsForTrailingBoxes): Use new boolean value
+
 2014-03-25  Martin Robinson  <mrobinson@igalia.com>
 

trunk/Source/WebCore/rendering/InlineIterator.h

@@ -42 +42 @@
         , m_nextBreakablePosition(-1)
         , m_pos(0)
+        , m_refersToEndOfPreviousNode(false)
     {
     }
@@ -50 +51 @@
         , m_nextBreakablePosition(-1)
         , m_pos(p)
+        , m_refersToEndOfPreviousNode(false)
     {
     }
@@ -62 +64 @@
     void moveTo(RenderObject* object, unsigned offset, int nextBreak = -1)
     {
-        m_renderer = object;
-        m_pos = offset;
-        m_nextBreakablePosition = nextBreak;
+        setRenderer(object);
+        setOffset(offset);
+        setNextBreakablePosition(nextBreak);
     }
 
@@ -70 +72 @@
     void setRenderer(RenderObject* renderer) { m_renderer = renderer; }
     unsigned offset() const { return m_pos; }
-    void setOffset(unsigned position) { m_pos = position; }
+    void setOffset(unsigned position);
     RenderElement* root() const { return m_root; }
     int nextBreakablePosition() const { return m_nextBreakablePosition; }
     void setNextBreakablePosition(int position) { m_nextBreakablePosition = position; }
+    bool refersToEndOfPreviousNode() const { return m_refersToEndOfPreviousNode; }
+    void setRefersToEndOfPreviousNode();
 
     void fastIncrementInTextNode();
     void increment(InlineBidiResolver* = 0);
+    void fastDecrement();
     bool atEnd() const;
 
@@ -101 +106 @@
     int m_nextBreakablePosition;
     unsigned m_pos;
+
+    // There are a couple places where we want to decrement an InlineIterator.
+    // Usually this take the form of decrementing m_pos; however, m_pos might be 0.
+    // However, we shouldn't ever need to decrement an InlineIterator more than
+    // once, so rather than implementing a decrement() function which traverses
+    // nodes, we can simply keep track of this state and handle it.
+    bool m_refersToEndOfPreviousNode;
 };
 
@@ -322 +334 @@
 }
 
+inline void InlineIterator::setOffset(unsigned position)
+{
+    ASSERT(position <= UINT_MAX - 10); // Sanity check
+    m_pos = position;
+}
+
+inline void InlineIterator::setRefersToEndOfPreviousNode()
+{
+    ASSERT(!m_pos);
+    ASSERT(!m_refersToEndOfPreviousNode);
+    m_refersToEndOfPreviousNode = true;
+}
+
 // FIXME: This is used by RenderBlock for simplified layout, and has nothing to do with bidi
 // it shouldn't use functions called bidiFirst and bidiNext.
@@ -364 +389 @@
     // bidiNext can return 0, so use moveTo instead of moveToStartOf
     moveTo(bidiNextSkippingEmptyInlines(*m_root, m_renderer, resolver), 0);
+}
+
+inline void InlineIterator::fastDecrement()
+{
+    ASSERT(!refersToEndOfPreviousNode());
+    if (m_pos)
+        setOffset(m_pos - 1);
+    else
+        setRefersToEndOfPreviousNode();
 }
 

trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

@@ -106 +106 @@
             lineMidpointState.setBetweenMidpoints(true);
             lineMidpointState.incrementCurrentMidpoint();
-            if (nextMidpoint.offset() != UINT_MAX) { // UINT_MAX means stop at the object and don't include any of it.
-                if (static_cast<int>(nextMidpoint.offset() + 1) > start)
-                    runs.addRun(createRun(start, nextMidpoint.offset() + 1, obj, resolver));
-                return appendRunsForObject(runs, nextMidpoint.offset() + 1, end, obj, resolver);
-            }
+            // The end of the line is before the object we're inspecting. Skip everything and return
+            if (nextMidpoint.refersToEndOfPreviousNode())
+                return;
+            if (static_cast<int>(nextMidpoint.offset() + 1) > start)
+                runs.addRun(createRun(start, nextMidpoint.offset() + 1, obj, resolver));
+            appendRunsForObject(runs, nextMidpoint.offset() + 1, end, obj, resolver);
         } else
            runs.addRun(createRun(start, end, obj, resolver));

trunk/Source/WebCore/rendering/line/BreakingContextInlineHeaders.h

@@ -905 +905 @@
             // space doesn't seem to push the text out from the right-hand edge.
             // FIXME: Do this regardless of the container's alignment - will require rebaselining a lot of test results.
-            if (m_nextObject && m_nextObject->isBR() && (m_blockStyle.textAlign() == RIGHT || m_blockStyle.textAlign() == WEBKIT_RIGHT)) {
+            if (m_nextObject && m_startOfIgnoredSpaces.offset() && m_nextObject->isBR() && (m_blockStyle.textAlign() == RIGHT || m_blockStyle.textAlign() == WEBKIT_RIGHT)) {
                 m_startOfIgnoredSpaces.setOffset(m_startOfIgnoredSpaces.offset() - 1);
                 // If there's just a single trailing space start ignoring it now so it collapses away.
@@ -1066 +1066 @@
             lineMidpointState.decreaseNumMidpoints();
             if (endpoint.renderer()->style().collapseWhiteSpace() && endpoint.renderer()->isText())
-                endpoint.setOffset(endpoint.offset() - 1);
+                endpoint.fastDecrement();
         }
     }

trunk/Source/WebCore/rendering/line/TrailingObjects.cpp

@@ -43 +43 @@
         ASSERT(trailingSpaceMidpoint >= 0);
         if (collapseFirstSpace == CollapseFirstSpace)
-            lineMidpointState.midpoints()[trailingSpaceMidpoint].setOffset(lineMidpointState.midpoints()[trailingSpaceMidpoint].offset() -1);
+            lineMidpointState.midpoints()[trailingSpaceMidpoint].fastDecrement();
 
         // Now make sure every single trailingPositionedBox following the trailingSpaceMidpoint properly stops and starts