InlineIterator position (unsigned int) variable can wrap around
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Mar 2014 20:15:14 +0000 (20:15 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Mar 2014 20:15:14 +0000 (20:15 +0000)
commitd24aae794b248d424059177f55fddc315a479c91
tree704d80a42a5ca4adb4607b7c319b6cfcc1ff5f48
parent4e54d7e60614e3fd990c70ea36df9da9ac5da27a
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

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@166245 268f45cc-cd09-0410-ab3c-d52691b4dbfc
LayoutTests/ChangeLog
LayoutTests/fast/text/whitespace-only-text-in-rtl-expected.txt [new file with mode: 0644]
LayoutTests/fast/text/whitespace-only-text-in-rtl.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/InlineIterator.h
Source/WebCore/rendering/RenderBlockLineLayout.cpp
Source/WebCore/rendering/line/BreakingContextInlineHeaders.h
Source/WebCore/rendering/line/TrailingObjects.cpp