- Revision
- 166245
- Author
- mmaxfi...@apple.com
- Date
- 2014-03-25 13:15:14 -0700 (Tue, 25 Mar 2014)
Log Message
InlineIterator position (unsigned int) variable can wrap around
https://bugs.webkit.org/show_bug.cgi?id=130540
Patch by Myles C. Maxfield <mmaxfi...@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
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (166244 => 166245)
--- trunk/LayoutTests/ChangeLog 2014-03-25 20:10:02 UTC (rev 166244)
+++ trunk/LayoutTests/ChangeLog 2014-03-25 20:15:14 UTC (rev 166245)
@@ -1,3 +1,24 @@
+2014-03-25 Myles C. Maxfield <mmaxfi...@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 <oli...@apple.com>
AST incorrectly conflates readable and writable locations
Added: trunk/LayoutTests/fast/text/whitespace-only-text-in-rtl-expected.txt (0 => 166245)
--- trunk/LayoutTests/fast/text/whitespace-only-text-in-rtl-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/text/whitespace-only-text-in-rtl-expected.txt 2014-03-25 20:15:14 UTC (rev 166245)
@@ -0,0 +1,4 @@
+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.
+a
+
+
Added: trunk/LayoutTests/fast/text/whitespace-only-text-in-rtl.html (0 => 166245)
--- trunk/LayoutTests/fast/text/whitespace-only-text-in-rtl.html (rev 0)
+++ trunk/LayoutTests/fast/text/whitespace-only-text-in-rtl.html 2014-03-25 20:15:14 UTC (rev 166245)
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<head>
+</head>
+<body>
+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.
+<div style="text-align: right;"><span>a</span> <br><br></div>
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (166244 => 166245)
--- trunk/Source/WebCore/ChangeLog 2014-03-25 20:10:02 UTC (rev 166244)
+++ trunk/Source/WebCore/ChangeLog 2014-03-25 20:15:14 UTC (rev 166245)
@@ -1,3 +1,38 @@
+2014-03-25 Myles C. Maxfield <mmaxfi...@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 <mrobin...@igalia.com>
[GTK] Remove the autotools build
Modified: trunk/Source/WebCore/rendering/InlineIterator.h (166244 => 166245)
--- trunk/Source/WebCore/rendering/InlineIterator.h 2014-03-25 20:10:02 UTC (rev 166244)
+++ trunk/Source/WebCore/rendering/InlineIterator.h 2014-03-25 20:15:14 UTC (rev 166245)
@@ -41,6 +41,7 @@
, m_renderer(0)
, m_nextBreakablePosition(-1)
, m_pos(0)
+ , m_refersToEndOfPreviousNode(false)
{
}
@@ -49,6 +50,7 @@
, m_renderer(o)
, m_nextBreakablePosition(-1)
, m_pos(p)
+ , m_refersToEndOfPreviousNode(false)
{
}
@@ -61,21 +63,24 @@
void moveTo(RenderObject* object, unsigned offset, int nextBreak = -1)
{
- m_renderer = object;
- m_pos = offset;
- m_nextBreakablePosition = nextBreak;
+ setRenderer(object);
+ setOffset(offset);
+ setNextBreakablePosition(nextBreak);
}
RenderObject* renderer() const { return m_renderer; }
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;
inline bool atTextParagraphSeparator()
@@ -100,6 +105,13 @@
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;
};
inline bool operator==(const InlineIterator& it1, const InlineIterator& it2)
@@ -321,6 +333,19 @@
m_pos++;
}
+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.
class InlineWalker {
@@ -365,6 +390,15 @@
moveTo(bidiNextSkippingEmptyInlines(*m_root, m_renderer, resolver), 0);
}
+inline void InlineIterator::fastDecrement()
+{
+ ASSERT(!refersToEndOfPreviousNode());
+ if (m_pos)
+ setOffset(m_pos - 1);
+ else
+ setRefersToEndOfPreviousNode();
+}
+
inline bool InlineIterator::atEnd() const
{
return !m_renderer;
Modified: trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp (166244 => 166245)
--- trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp 2014-03-25 20:10:02 UTC (rev 166244)
+++ trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp 2014-03-25 20:15:14 UTC (rev 166245)
@@ -105,11 +105,12 @@
if (static_cast<int>(nextMidpoint.offset() + 1) <= end) {
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));
}
Modified: trunk/Source/WebCore/rendering/line/BreakingContextInlineHeaders.h (166244 => 166245)
--- trunk/Source/WebCore/rendering/line/BreakingContextInlineHeaders.h 2014-03-25 20:10:02 UTC (rev 166244)
+++ trunk/Source/WebCore/rendering/line/BreakingContextInlineHeaders.h 2014-03-25 20:15:14 UTC (rev 166245)
@@ -904,7 +904,7 @@
// Spaces after right-aligned text and before a line-break get collapsed away completely so that the trailing
// 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.
if (m_current.offset() == renderText->textLength() - 1)
@@ -1065,7 +1065,7 @@
// We hit the line break before the start point. Shave off the start point.
lineMidpointState.decreaseNumMidpoints();
if (endpoint.renderer()->style().collapseWhiteSpace() && endpoint.renderer()->isText())
- endpoint.setOffset(endpoint.offset() - 1);
+ endpoint.fastDecrement();
}
}
}
Modified: trunk/Source/WebCore/rendering/line/TrailingObjects.cpp (166244 => 166245)
--- trunk/Source/WebCore/rendering/line/TrailingObjects.cpp 2014-03-25 20:10:02 UTC (rev 166244)
+++ trunk/Source/WebCore/rendering/line/TrailingObjects.cpp 2014-03-25 20:15:14 UTC (rev 166245)
@@ -42,7 +42,7 @@
for ( ; trailingSpaceMidpoint > 0 && lineMidpointState.midpoints()[trailingSpaceMidpoint].renderer() != m_whitespace; --trailingSpaceMidpoint) { }
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
// ignoring spaces.
