Prevent infinite loop about itemizing text in RenderTextHarfBuzz.

Prevent infinite loop about itemizing text in RenderTextHarfBuzz.

BUG=403892
TEST=the new test case covers

[Daniel Erat, 2015-03-17 23:25:59]

derat@chromium.org changed reviewers:
+ ckocagil@chromium.org, derat@chromium.org, msw@chromium.org

[Daniel Erat, 2015-03-17 23:26:00]

lgtm, but adding others more familiar with this code

https://codereview.chromium.org/993153003/diff/1/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/993153003/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:2580: TEST_F(RenderTextTest,
HarfBuzz_InfiniteLoop) {
nit: mind adding a comment pointing back at the bug?

[Jun Mukai, 2015-03-18 01:32:05]

https://codereview.chromium.org/993153003/diff/1/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/993153003/diff/1/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:2580: TEST_F(RenderTextTest,
HarfBuzz_InfiniteLoop) {
On 2015/03/17 23:26:00, Daniel Erat (OOO to Thursday) wrote:
> nit: mind adding a comment pointing back at the bug?

added

[msw, 2015-03-18 16:21:36]

https://codereview.chromium.org/993153003/diff/20001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/993153003/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1187: if (run_break <= run->range.start())
Why does this ever happen? Can we fix the actual defect, presumably with line
1178: TextIndexToGivenTextIndex(text, style.GetRange().end())?

[Jun Mukai, 2015-03-23 23:14:06]

https://codereview.chromium.org/993153003/diff/20001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/993153003/diff/20001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1187: if (run_break <= run->range.start())
On 2015/03/18 16:21:36, msw wrote:
> Why does this ever happen? Can we fix the actual defect, presumably with line
> 1178: TextIndexToGivenTextIndex(text, style.GetRange().end())?

It's still unclear why it happens on the reported environment.
For the new test case I've added, this could happen because |text| is different
from |this->text()|. In the test case, render_text is obscured. Therefore,
|text| is like "**" while this->text() holds 3 code points (1 surrogate pair + 1
character), and TextIndexToGivenTextIndex("**", 2) returns to 1.

This happens because of a confusion; the style ranges are usually specified to
this->text(), but the style's max is capped by text.length(), which is shorter
than this->text().

That said, I can fix this by changing the max of style, uploaded the patchset 3.


However, I don't think this is the case we're seeing in the bug reports, this
would happen only on obscured render text. I can't think of other cases which
run_break cannot proceed, but the crash indicates sometimes it goes into such
case.

[msw, 2015-03-24 01:56:30]

msw@chromium.org changed reviewers:
+ oshima@chromium.org

[msw, 2015-03-24 01:56:31]

+oshima for new RenderText layout/display text use.

https://codereview.chromium.org/993153003/diff/40001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/993153003/diff/40001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1158: empty_colors.SetMax(this->text().length());
Just use colors().max() and remove this comment.

https://codereview.chromium.org/993153003/diff/40001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1191:
style.UpdatePosition(DisplayIndexToTextIndex(run_break));
ItemizeTextToRuns is called with the layout text and the display text as the
|text| argument at different times. The use of DisplayIndexToTextIndex will be
wrong in the case of layout text... Perhaps that's [part of] the defect?