New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 647080 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

CharacterIterator does not work with first-letter pseudo element

Project Member Reported by ClusterFuzz, Sep 14 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5515162260078592

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  m_startPosition <= m_endPosition (#text "\n  !ABC\u202E\n \n\n\n \n  "@offsetInA
  blink::EphemeralRangeTemplate<>::EphemeralRangeTemplate
  blink::CharacterIteratorAlgorithm<>::calculateCharacterSubrange
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=404238:404340

Minimized Testcase (0.28 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95j-1vOU--EbN_IHrgiixqf_2jU5X9yH3DRpqPdAqeTBN8QeJ1mHCcSc02IqFjKsFvdaOpuoUVBAB8XAJtmQQZi4-LIsN-Vn-8UX8pidJybqYMDNkyTxDXW0vWREKZb80ZsmSywqPbO4d-Tw4LS0Wh7zCTL5A?testcase_id=5515162260078592
<style>
   body:first-letter { color: black;</style>
  <script>
function __f_0() {
    document.execCommand('findString', false, '!ABC');
}
  </script>
 <body onload="__f_2();">
  !ABC���
 </body>
<!DOCTYPE>
<html>
 <head>
  <title>
  </title>
  <script>
 run = __f_0; 
 run(); 
</script>


Issue manually filed by: mummareddy

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: koten...@yandex-team.ru
Components: Tools>Test>FindIt>NoResult
Labels: M-54 Te-Logged
Owner: yosin@chromium.org
Status: Assigned (was: Untriaged)
Through code search on file EphemeralRange.cpp, suspected CL is
https://chromium.googlesource.com/chromium/src/+/673555a5d9c7a1bc26dc67813e1110cdc2e07268

yosin@, could you please take a look and please help us to find correct owner if it is not related your changes.
Components: Blink>Editing
Cc: xiaoche...@chromium.org
Components: -Tools>Test>FindIt>NoResult
Project Member

Comment 5 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by yosin@chromium.org, Nov 28 2016

Owner: ----
Status: Available (was: Assigned)
Summary: U+202E, 'RIGHT-TO-LEFT OVERRIDE, makes character iterator to hit DCHECK (was: m_startPosition <= m_endPosition (#text "\n !ABC\u202E\n \n\n\n \n "@offsetInA)
Owner: xiaoche...@chromium.org
Status: Started (was: Available)
This issue is solely caused by first-letter, and irrelevant to U+202E.

Minimized test case:

<!doctype html>
<style>div:first-letter {color:black;}</style>
<div>   !ABC</div>
<script>
document.execCommand('findString', false, '!ABC');
</script>
Summary: CharacterIterator does not work with first-letter pseudo element (was: U+202E, 'RIGHT-TO-LEFT OVERRIDE, makes character iterator to hit DCHECK)
It's not even relevant to the '!' preceding the first letter... Further minimized:

<!doctype html>
<style>div:first-letter {color:black;}</style>
<div>  AB</div>
<script>
document.execCommand('findString', false, 'AB');
</script>
TextIterator doesn't work quite well with first-letter. We observe issue with CharacterIterator since it's just a wrapper of TextIterator.

The following two unit tests currently fail:

TEST_F(TextIteratorTest, StartAtFirstLetter) {
  setBodyContent(
    "<style>div:first-letter {color:red;}</style><div>Axyz</div>");

  Element* div = document().querySelector("div");
  Node* text = div->firstChild();
  Position start(text, 0);
  Position end(text, 4);
  TextIterator iter(start, end);
  ForwardsTextBuffer buffer;

  EXPECT_FALSE(iter.atEnd());
  EXPECT_EQ(1, iter.length());
  EXPECT_EQ(1, iter.copyTextTo(&buffer, 0)) << "Should emit 'A'.";
  EXPECT_EQ(text, iter.currentContainer());
  EXPECT_EQ(Position(text, 0), iter.startPositionInCurrentContainer());
  EXPECT_EQ(Position(text, 1), iter.endPositionInCurrentContainer());

  iter.advance();
  EXPECT_FALSE(iter.atEnd());
  EXPECT_EQ(3, iter.length());
  EXPECT_EQ(3, iter.copyTextTo(&buffer, 0)) << "Should emit 'xyz'.";
  EXPECT_EQ(text, iter.currentContainer());
  EXPECT_EQ(Position(text, 1), iter.startPositionInCurrentContainer());
  EXPECT_EQ(Position(text, 4), iter.endPositionInCurrentContainer());

  iter.advance();
  EXPECT_TRUE(iter.atEnd());

  EXPECT_EQ("Axyz", String(buffer.data()));
}

TEST_F(TextIteratorTest, StartAtRemainingText) {
  setBodyContent(
    "<style>div:first-letter {color:red;}</style><div>Axyz</div>");

  Element* div = document().querySelector("div");
  Node* text = div->firstChild();
  Position start(text, 1);
  Position end(text, 4);
  TextIterator iter(start, end);
  ForwardsTextBuffer buffer;

  EXPECT_FALSE(iter.atEnd());
  EXPECT_EQ(3, iter.length());
  EXPECT_EQ(3, iter.copyTextTo(&buffer, 0)) << "Should emit 'xyz'.";
  EXPECT_EQ(text, iter.currentContainer());
  EXPECT_EQ(Position(text, 1), iter.startPositionInCurrentContainer());
  EXPECT_EQ(Position(text, 4), iter.endPositionInCurrentContainer());

  iter.advance();
  EXPECT_TRUE(iter.atEnd());

  EXPECT_EQ("xyz", String(buffer.data()));
}

Miraculously, we currently pass layout test editing/text-iterator/first-letter-word-boundary.html, due to two buggy behaviors negating each other: TextIterator::copyTextTo emits shifted text, which is shifted back by TextIterator::start/endPositionInCurrentContainer...
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 5 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5185769b9ba7e9239d68c7dc672a27dd19b1685f

commit 5185769b9ba7e9239d68c7dc672a27dd19b1685f
Author: xiaochengh <xiaochengh@chromium.org>
Date: Mon Dec 05 10:39:01 2016

Fix TextIterator's behavior with first-letter

TextIterator's behavior with first-letter is broken:
1. If its starts in a text node with first-letter and the starting
offset is non-zero, wrong text is passed to its text state.
2. When the text state is handling the remaining text of first-letter,
it doesn't consider the length of first-letter, and hence, reports
wrong offsets of the current text.

To fix 2, this patch makes the text state remember the length of
first-letter, and use it to adjust the output of
start/endOffsetInCurrentContainer.

To fix 1, given that the current behavior is correct when we start in
a text node at offset 0 (even with first-letter), this patch performs
a reduction that:
- start iterating with offset 0 instead of the given start offset
- have extra calls of |advance()| during initialization so that we have
moved to the given start position when initialization ends
- adjust text run start and end offsets when emitting text of the starting
text node

This patch partially corrects the behavior in two layout tests:
- editing/selection/extend-by-word-002.html
- editing/text-iterator/first-letter-word-boundary.html
The tests are still failing for other reasons, see crbug.com/671104

This patch is not an ultimate fix of TextIterator's behavior with
first-letter, but more like a first-aid hack built upon the current design.
In the long term, we should rewrite TextIterator.

BUG= 647080 

Review-Url: https://codereview.chromium.org/2541163003
Cr-Commit-Position: refs/heads/master@{#436255}

[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/LayoutTests/editing/selection/modify_extend/extend_by_word_002.html
[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/LayoutTests/editing/text-iterator/first-letter-word-boundary.html
[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp
[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/Source/core/editing/iterators/TextIterator.h
[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp
[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/Source/core/editing/iterators/TextIteratorTextState.cpp
[modify] https://crrev.com/5185769b9ba7e9239d68c7dc672a27dd19b1685f/third_party/WebKit/Source/core/editing/iterators/TextIteratorTextState.h

Project Member

Comment 11 by ClusterFuzz, Dec 12 2016

ClusterFuzz has detected this issue as fixed in range 436251:436259.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5515162260078592

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  m_startPosition <= m_endPosition (#text "\n  !ABC\u202E\n \n\n\n \n  "@offsetInA
  blink::EphemeralRangeTemplate<>::EphemeralRangeTemplate
  blink::CharacterIteratorAlgorithm<>::calculateCharacterSubrange
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=404238:404340
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_debug_content_shell_drt&range=436251:436259

Minimized Testcase (0.28 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95j-1vOU--EbN_IHrgiixqf_2jU5X9yH3DRpqPdAqeTBN8QeJ1mHCcSc02IqFjKsFvdaOpuoUVBAB8XAJtmQQZi4-LIsN-Vn-8UX8pidJybqYMDNkyTxDXW0vWREKZb80ZsmSywqPbO4d-Tw4LS0Wh7zCTL5A?testcase_id=5515162260078592
<style>
   body:first-letter { color: black;</style>
  <script>
function __f_0() {
    document.execCommand('findString', false, '!ABC');
}
  </script>
 <body onload="__f_2();">
  !ABC‮
 </body>
<!DOCTYPE>
<html>
 <head>
  <title>
  </title>
  <script>
 run = __f_0; 
 run(); 
</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Status: Fixed (was: Started)

Sign in to add a comment