Clamp RenderTextWin layout length to 10,000 code points.

Clamp RenderTextWin layout length to 10,000 code points.

Add RenderText::truncate_length_ and setter to clamp layout text length.
Avoid system limitations and performance degradation on Win/Uniscribe.
This does not impact underlying text data, only what's shown on screen.

Truncate text_ to layout_text_ in UpdateLayoutText() and append ellipsis.
Only reveal obscured characters within the layout text range.
Ensure the run and glyph limits are actually tried in their loops.
Add unit tests for reasonable truncation behavior.

Bail if layout fails, this will leave text un-rendered instead of crashing.
Clamp run length to avoid exceeding the glyph limit later on.
Clamp layout indices in conversion, fix run index bounds checks.

TODO(followup): Truncate text on Linux/Pango as needed.
TODO(followup): Increase supported text length and run counts.
TODO(followup): UMA text lengths, run counts, failures, and truncations.

BUG=235854, 236406, 248960, 131660
TEST=Paste more than 10k characters on Win to see ellipsis and no crashes. (note that omnibox specific codepaths will cause additional performance degradation in that field). No crash on Windows with Search by Image extension use (tried various sizes 16x16 -> 96x96 -> 1900x1200 -> 3840x2160). Normal text behavior with <10k code points, and no behavior change on ChromeOS/Linux_Aura.
R=asvitkine@chromium.org,oshima@chromium.org,sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209286

[msw, 2013-06-27 18:23:10]

Hey Alexei, Oshima, and Scott, please take a look; thanks!
There's room for followup work, but this should be good to merge back to M29.

[Alexei Svitkine (slow), 2013-06-27 20:13:45]

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:967: static const char16 kEllipsis[] = { 0x2026, 0 };
Use UI_EXPORT extern const char kEllipsis[]; from text_elider.h instead.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:554: size_t truncate_length_;
Initialize this in the ctor.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:60: void RunMoveCursorLeftRightTest(
Add a comment.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:503: const size_t run_limit = script_items.max_size()
/ (run_size * 10);
Sorry, I don't understand why you're doing this.

I'm not sure why |run_size| is relevant here or why you're dividing
script_items.max_size() by run_size / 10.

What was wrong with the previous simpler logic?

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:758: max_glyphs = std::max(max_glyphs + 1,
std::min(max_glyphs * 2, kMaxGlyphs));
Why do you need the outer std::max() call?

Is it to prevent overflow from doubling?

If so, I don't think it should ever happen since the previous max_glyphs should
be <= 65k and doubling 65k shouldn't overflow a size_t.

[oshima, 2013-06-27 21:24:50]

i'm not familiar with this code (esp. win), so I'll leave this to alexei. Just
one nit.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:463:
RunMoveCursorLeftRightTest(render_text.get(), expected, CURSOR_RIGHT);
nit: Use SCOPED_TRACE (like SCOPED_TRAECE("CURSOR_RIGHT")) when you have
EXPECT_XXX in a function.

[msw, 2013-06-27 21:51:19]

Comments addressed, please take another look; thanks!

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.cc#new...
ui/gfx/render_text.cc:967: static const char16 kEllipsis[] = { 0x2026, 0 };
On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> Use UI_EXPORT extern const char kEllipsis[]; from text_elider.h instead.

Added similar char16 kEllipsisUTF16 there, used some there and here.
(to avoid some conversion and including yet another header here).

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text.h#newc...
ui/gfx/render_text.h:554: size_t truncate_length_;
On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> Initialize this in the ctor.

Done.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:60: void RunMoveCursorLeftRightTest(
On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_unitte...
ui/gfx/render_text_unittest.cc:463:
RunMoveCursorLeftRightTest(render_text.get(), expected, CURSOR_RIGHT);
On 2013/06/27 21:24:50, oshima wrote:
> nit: Use SCOPED_TRACE (like SCOPED_TRAECE("CURSOR_RIGHT")) when you have
> EXPECT_XXX in a function.

Done.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:503: const size_t run_limit = script_items.max_size()
/ (run_size * 10);
On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> Sorry, I don't understand why you're doing this.
> 
> I'm not sure why |run_size| is relevant here or why you're dividing
> script_items.max_size() by run_size / 10.
> 
> What was wrong with the previous simpler logic?

The supported number of runs is only limited by available memory.
This roughly caps by the possible SCRIPT_ITEM and TextRun vector sizes.
I divide by 10 to avoid using all available memory for string itemization.

The old logic imposed arbitrary limits, this is at least rationalized.
I'm open to other suggestions, but just added comments for now.

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:758: max_glyphs = std::max(max_glyphs + 1,
std::min(max_glyphs * 2, kMaxGlyphs));
On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> Why do you need the outer std::max() call?
> 
> Is it to prevent overflow from doubling?
> 
> If so, I don't think it should ever happen since the previous max_glyphs
should
> be <= 65k and doubling 65k shouldn't overflow a size_t.

No, this is a bit of consice cleverness that I added a comment to explain.
The overall idea is to double |max_glyphs|, but not exceed kMaxGlyphs.
The outer std::max call ensures the loop will exit after trying kMaxGlyphs.

Example: max_glyphs doubles from (<<kMaxGlyphs) to (>>kMaxGlyphs).
That value is clamped, so the loop actually tries the true kMaxGlyphs value.
If that fails, the loop terminates by forcing max_glyphs to exceed kMaxGlyphs.

[Alexei Svitkine (slow), 2013-06-27 22:17:46]

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:503: const size_t run_limit = script_items.max_size()
/ (run_size * 10);
On 2013/06/27 21:51:19, msw wrote:
> On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> > Sorry, I don't understand why you're doing this.
> > 
> > I'm not sure why |run_size| is relevant here or why you're dividing
> > script_items.max_size() by run_size / 10.
> > 
> > What was wrong with the previous simpler logic?
> 
> The supported number of runs is only limited by available memory.
> This roughly caps by the possible SCRIPT_ITEM and TextRun vector sizes.
> I divide by 10 to avoid using all available memory for string itemization.
> 
> The old logic imposed arbitrary limits, this is at least rationalized.
> I'm open to other suggestions, but just added comments for now.

I don't think we should do this. First, I don't think you can accurately
estimate how much memory will be used by each run. For example, each run has a
SCRIPT_CACHE which is actually just a pointer to some blob of memory that's
managed by Uniscribe and we don't really know how big that is. But even if you
could estimate that, you don't know how much temporary memory is used by
Uniscribe to get to that result or what's the CPU cost for such large inputs.
Plus, I think using 10% of available memory probably doesn't make sense either -
because then 10 of these could still just crash the process.

Perhaps we can think about this some more and come up with something better than
the hardcoded limit, but I would encourage doing that in another CL if at all,
and limiting this one to what's strictly necessarily to fix the crash bug.
Especially since you're planning to merge this to M29, and smaller changes are
less scary for TPMs to accept for merges.

[msw, 2013-06-27 22:35:44]

Please take another look; thanks!

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/17745005/diff/25001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:503: const size_t run_limit = script_items.max_size()
/ (run_size * 10);
On 2013/06/27 22:17:46, Alexei Svitkine wrote:
> On 2013/06/27 21:51:19, msw wrote:
> > On 2013/06/27 20:13:45, Alexei Svitkine wrote:
> > > Sorry, I don't understand why you're doing this.
> > > 
> > > I'm not sure why |run_size| is relevant here or why you're dividing
> > > script_items.max_size() by run_size / 10.
> > > 
> > > What was wrong with the previous simpler logic?
> > 
> > The supported number of runs is only limited by available memory.
> > This roughly caps by the possible SCRIPT_ITEM and TextRun vector sizes.
> > I divide by 10 to avoid using all available memory for string itemization.
> > 
> > The old logic imposed arbitrary limits, this is at least rationalized.
> > I'm open to other suggestions, but just added comments for now.
> 
> I don't think we should do this. First, I don't think you can accurately
> estimate how much memory will be used by each run. For example, each run has a
> SCRIPT_CACHE which is actually just a pointer to some blob of memory that's
> managed by Uniscribe and we don't really know how big that is. But even if you
> could estimate that, you don't know how much temporary memory is used by
> Uniscribe to get to that result or what's the CPU cost for such large inputs.
> Plus, I think using 10% of available memory probably doesn't make sense either
-
> because then 10 of these could still just crash the process.
> 
> Perhaps we can think about this some more and come up with something better
than
> the hardcoded limit, but I would encourage doing that in another CL if at all,
> and limiting this one to what's strictly necessarily to fix the crash bug.
> Especially since you're planning to merge this to M29, and smaller changes are
> less scary for TPMs to accept for merges.

Good points, I reverted the upper limit, but kept the new guess and lower limit.

[Alexei Svitkine (slow), 2013-06-28 15:37:02]

https://codereview.chromium.org/17745005/diff/56001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/17745005/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:36: const char16 kEllipsisUTF16 = 0x2026;
Consider making it:

const char16 kEllipsisUTF16[] = { 0x2026, 0 };

This way, it's a null-terminated char16 string consistent with the kEllipsis
which is a null-terminated char string. And I believe can explicitly be
converted to string16, so can be used directly instead of string16(1,
kEllipsisUTF16).

https://codereview.chromium.org/17745005/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:118: path += UTF8ToUTF16(kEllipsis) + kForwardSlash;
You can use kEllipisisUTF16 if you change it to a null terminated string above.

https://codereview.chromium.org/17745005/diff/56001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/17745005/diff/56001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:506: size_t run_guess = std::min(kMaxRuns,
std::max(3U, layout_text_length / 10));
The old code would start at 100 whereas your new code seems like it would start
at a much lower value (e.g. either 3 or text size / 10, which is likely also
very low).

I'm concerned this will result in a lot more cases where we have to loop through
to get the right value. And looping through calling ScriptItemize() is probably
more expensive than just pre-allocating a decent-size-ish array the first time.

Note that the run_guess is only used to resize script_items, which is a
temporary vector in this function and wouldn't result in permanent memory bloat
if we pre-allocate too many entries that don't then get used.

As such, I think the old behaviour of starting with a guess of 100 is better,
since it will likely be enough for 99% of inputs and avoids multiple (expensive)
calls to ScriptItemize to find the right size for a temporary buffer.

https://codereview.chromium.org/17745005/diff/56001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:762: max_glyphs = std::max(max_glyphs + 1,
std::min(max_glyphs * 2, kMaxGlyphs));
How about making a helper function for this construct in the anon namespace to
avoid duplicating it and commenting it twice?

[msw, 2013-06-28 16:31:23]

Comments addressed. Please take another look and check the CQ bit if this CL
meets your approval. I'm on vacation through monday and will be afk most of the
day, but getting this in soon would ease the merge to M-29. I will try to take a
look later this afternoon/evening.

https://codereview.chromium.org/17745005/diff/56001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

https://codereview.chromium.org/17745005/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:36: const char16 kEllipsisUTF16 = 0x2026;
On 2013/06/28 15:37:02, Alexei Svitkine wrote:
> Consider making it:
> 
> const char16 kEllipsisUTF16[] = { 0x2026, 0 };
> 
> This way, it's a null-terminated char16 string consistent with the kEllipsis
> which is a null-terminated char string. And I believe can explicitly be
> converted to string16, so can be used directly instead of string16(1,
> kEllipsisUTF16).

Done.

https://codereview.chromium.org/17745005/diff/56001/ui/base/text/text_elider....
ui/base/text/text_elider.cc:118: path += UTF8ToUTF16(kEllipsis) + kForwardSlash;
On 2013/06/28 15:37:02, Alexei Svitkine wrote:
> You can use kEllipisisUTF16 if you change it to a null terminated string
above.

I'll pass; I hit test failures trying to change more than I already have.

https://codereview.chromium.org/17745005/diff/56001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/17745005/diff/56001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:506: size_t run_guess = std::min(kMaxRuns,
std::max(3U, layout_text_length / 10));
On 2013/06/28 15:37:02, Alexei Svitkine wrote:
> The old code would start at 100 whereas your new code seems like it would
start
> at a much lower value (e.g. either 3 or text size / 10, which is likely also
> very low).
> 
> I'm concerned this will result in a lot more cases where we have to loop
through
> to get the right value. And looping through calling ScriptItemize() is
probably
> more expensive than just pre-allocating a decent-size-ish array the first
time.
> 
> Note that the run_guess is only used to resize script_items, which is a
> temporary vector in this function and wouldn't result in permanent memory
bloat
> if we pre-allocate too many entries that don't then get used.
> 
> As such, I think the old behaviour of starting with a guess of 100 is better,
> since it will likely be enough for 99% of inputs and avoids multiple
(expensive)
> calls to ScriptItemize to find the right size for a temporary buffer.

Done.

https://codereview.chromium.org/17745005/diff/56001/ui/gfx/render_text_win.cc...
ui/gfx/render_text_win.cc:762: max_glyphs = std::max(max_glyphs + 1,
std::min(max_glyphs * 2, kMaxGlyphs));
On 2013/06/28 15:37:02, Alexei Svitkine wrote:
> How about making a helper function for this construct in the anon namespace to
> avoid duplicating it and commenting it twice?

I'll pass; 3 uses or real complexity constitutes a helper, but this seems ok.

[Alexei Svitkine (slow), 2013-06-28 16:42:18]

lgtm

[sky, 2013-06-28 17:07:59]

LGTM

[commit-bot: I haz the power, 2013-06-28 17:38:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/17745005/62001

[commit-bot: I haz the power, 2013-06-29 06:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/17745005/62001

[commit-bot: I haz the power, 2013-06-29 13:02:51]

Change committed as 209286