Somewhat bizarrely, I'm no longer able to reproduce this issue on Linux, even if I use what I believe is the same revision I originally tested on. I re-ran the trybots on https://codereview.chromium.org/2723663002 and indeed Linux is now passing but Windows and Mac are still failing. It's not yet clear to me if there's actually a platform-specific component to this or if the issue is just sporadic for some reason.

This bug seems purely a renderer bug, so I think it's better to minimize it inside the renderer. Browser tests may use platform-dependent APIs and have too many unrelated dependencies.

How about minimizing it as a TextFinderTest (in WebKit/Source/web/tests/)?

@xiaochengh, that sounds like a good idea; I haven't quite figured out how to minimize the test case yet though.

This must be some issue coming from the lines being laid out differently on the different bots. When I run the test locally, the marker gets broken up at follows:

#text "My mother has " from 10 to 14
#text "light blue" from 0 to 10
#text " eyes and my father has " from 0 to 9
#text " eyes and my father has " from 9 to 10
#text " eyes and my father has " from 10 to 24

I think the difference must be coming from how the lines are being broken up. Locally I have:

LayoutBlockFlow 0x834bd41c3a0          	P style="font-size: 5em;"
  RootInlineBox 0x834bd4640d0          	LayoutBlockFlow 0x834bd41c3a0 {pos=0,1 size=1124.33,88} baseline=72/45
    InlineTextBox 0x834bd454088        	LayoutText 0x834bd42c0d8                                                  (0,14) "My mother has "
    InlineTextBox 0x834bd454100        	LayoutText 0x834bd42c268                                                  (0,10) "light blue"
*   InlineTextBox 0x834bd454178        	LayoutText 0x834bd42c330                                                  (0,9) " eyes and"
  RootInlineBox 0x834bd464190          	LayoutBlockFlow 0x834bd41c3a0 {pos=0,92 size=1011.59,88} baseline=72/45
*   InlineTextBox 0x834bd4541f0        	LayoutText 0x834bd42c330                                                  (10,24) "my father has "
    InlineTextBox 0x834bd454268        	LayoutText 0x834bd42c4c0                                                  (0,10) "dark green"
    InlineTextBox 0x834bd4542e0        	LayoutText 0x834bd42c588                                                  (0,6) " eyes."


I don't have a log output of this from the trybots yet, but if I had to guess, the line is longer there for some reason (since " eyes and my father has " is not split), and the space at the beginning of " eyes and my father has " is getting some special handling as whitespace that's not working properly.

That's interesting. Have you tried testing with different viewport sizes?

Well, the viewport size seems to be controlled by the test infra; but I managed to reproduce the problem on my Retina MacBook Pro by increasing the screen resolution. So I should be able to debug this now.

I haven't tried changing viewport size in layout tests, but at least in unit tests it can be changed.

Anyway, I just realized that changing viewport size is an overkill. Changing the width of the container element does the same job. And since you can repro, please try to get a platform-independent repro.

Ok here's what's going on:

To get this to repro, the text needs to wrap after "My mother has light blue". One reliable way of doing this is to make the text really big in the test case and put non-breaking spaces everywhere we don't want a wrap.

TextIterator works off of the layout tree, not the DOM tree. It has some logic that takes the line break that was inserted by the layout algorithm and converts it back to a space:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp?gsn=LineLayoutItem&q=textiterator.cpp&l=724

TextIterator reports this as if it's outputting the space at the beginning of the line box containing " eyes and my father", but really that space is being skipped over and it's coming from the line break. Then the next time TextIterator::advance() is called, restoreCollapsedLeadingSpace() is called, which backs up to before the space in the line box and repeats it.

This also means if you e.g. call plainText() on an EphemeralRange containing the <p> element triggering the bug, the space after "blue" is repeated. So I'm confident this is a bug in TextIterator, not in how we're using it in DocumentMarkerController.

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

commit 477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3
Author: rlanday <rlanday@chromium.org>
Date: Tue Mar 21 10:30:33 2017

Fix bug causing TextIterator to duplicate leading spaces

I ran into a TextIterator bug causing spaces at the beginning of a line occuring
immediately after another element to be duplicated. If you're iterating over
ranges, you get a range containing just the space, and then a range containing
the space together with the rest of the line, and if you call plainText() on a
range to get the text, the space occurs twice in the output.

The bug occurs because the line break gets output as one space, and then the
actual space at the beginning of the line is supposed to be skipped over. The
method restoreCollapsedLeadingSpace() is called, which incorrectly causes
TextIterator to output the real space in addition to the space from the line
break.

It appears this method was originally added to fix a text copying bug. Text
copying does use the plainText() function, but does not exhibit the bug because
it sets the emitsImageAltText behavior, and this method immediately returns if
that flag is set.

It appears that the correct fix is to remove this method. It may have been
necessary at some point but now it just appears to be introducing this bug. I
took out this method and re-ran all the webkit_unit_tests and blink layout
tests, and the only test that failed was one with a TODO saying to fix the
behavior. So my change actually fixes another bug there.

Additionally, I think the change in https://codereview.chromium.org/2723913002
is just a workaround for this bug and is no longer necessary, so I'm reverting
it. I don't understand it super well though so I could be wrong.

BUG= 697654 

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

[modify] https://crrev.com/477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3/third_party/WebKit/Source/core/editing/FrameSelection.cpp
[modify] https://crrev.com/477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3/third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp
[modify] https://crrev.com/477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3/third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp
[modify] https://crrev.com/477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3/third_party/WebKit/Source/core/editing/iterators/TextIterator.h
[modify] https://crrev.com/477e319330d3ad4eccfb32fbd5c7d28a75c6d7e3/third_party/WebKit/Source/core/editing/iterators/TextIteratorTest.cpp