Fix crash when trying to iterate over words in an inline text box of length zero.

Fix crash when trying to iterate over words in an inline text box of length zero.

BUG=373885
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181649

[dmazzoni, 2014-09-04 07:57:50]

dmazzoni@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2014-09-04 16:39:49]

https://codereview.chromium.org/542553002/diff/1/Source/core/rendering/Abstra...
File Source/core/rendering/AbstractInlineTextBox.cpp (right):

https://codereview.chromium.org/542553002/diff/1/Source/core/rendering/Abstra...
Source/core/rendering/AbstractInlineTextBox.cpp:134: if (!len)
Should this be moved to after wordBreakIterator if (iterator) instead?

[dmazzoni, 2014-09-04 16:46:57]

https://codereview.chromium.org/542553002/diff/1/Source/core/rendering/Abstra...
File Source/core/rendering/AbstractInlineTextBox.cpp (right):

https://codereview.chromium.org/542553002/diff/1/Source/core/rendering/Abstra...
Source/core/rendering/AbstractInlineTextBox.cpp:134: if (!len)
On 2014/09/04 16:39:49, eseidel wrote:
> Should this be moved to after wordBreakIterator if (iterator) instead?

Sure. I was torn between catching the actual crash (null iterator) or the
underlying reason behind it (len == 0). I switched it to check the iterator but
added a comment so we know there's a reason why it might happen.

[dmazzoni, 2014-09-04 16:48:58]

BTW, do you happen to know why the :first-letter text-transform is resulting in
an empty inline text box? It doesn't really matter, but I am curious. I tried
hundreds of web pages and never encountered an inline text box with len 0 until
this crash happened in the wild.

[eseidel, 2014-09-04 18:05:51]

eseidel@chromium.org changed reviewers:
+ leviw@chromium.org

[eseidel, 2014-09-04 18:05:51]

I recall seeing 0len inline text boxes before.  But I don't remember why.  Levi
might.

[eseidel, 2014-09-04 18:06:32]

Oh.  I remember why we used to see them.  It was bugs in updateFirstLetter.  I
wonder if we just have more bugs in updateFirstLetter again. :(

[eseidel, 2014-09-04 18:07:16]

It looks like we have a bunch of crashes in updateFirstLetter:
https://code.google.com/p/chromium/issues/list?can=2&q=updateFirstLetter

That's likely to be the actual cause of your issue and this is just a symptom.

[dmazzoni, 2014-09-04 18:11:08]

On 2014/09/04 18:07:16, eseidel wrote:
> It looks like we have a bunch of crashes in updateFirstLetter:
> https://code.google.com/p/chromium/issues/list?can=2&q=updateFirstLetter
> 
> That's likely to be the actual cause of your issue and this is just a symptom.

Got it.

This change still okay? It seems like we'd want to avoid the crash if this were
to happen in the future.

[eseidel, 2014-09-04 18:16:41]

If you want to make this change, please add an ASSERT(iterator) before it, and
add a comment that this is likely caused by a bug in updateFirstLetter and link
to either that search or one of the bugs.

[dmazzoni, 2014-09-04 22:53:28]

dmazzoni@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dmazzoni, 2014-09-04 22:53:29]

Adding an assert would mean I'd have to disable the layout test, right?

@dsinclair, it looks like you've been looking at first-letter issues. Could you
check out the layout test I made for this change and see if there's a better
fix, or if we should land this fix in the meantime?

[dsinclair, 2014-09-05 13:39:01]

On 2014/09/04 22:53:29, dmazzoni wrote:
> Adding an assert would mean I'd have to disable the layout test, right?
> 
> @dsinclair, it looks like you've been looking at first-letter issues. Could
you
> check out the layout test I made for this change and see if there's a better
> fix, or if we should land this fix in the meantime?

Nothing jumps out at me with the layout test. What about leaving this as an if
(!iterator) and putting in a fixme to making this an ASSERT with a crbug link to
figure out the cause of the zero length box?

[dmazzoni, 2014-09-08 23:16:43]

On 2014/09/05 13:39:01, dsinclair wrote:
> Nothing jumps out at me with the layout test. What about leaving this as an if
> (!iterator) and putting in a fixme to making this an ASSERT with a crbug link
to
> figure out the cause of the zero length box?

Done, PTAL.

[dsinclair, 2014-09-09 13:12:53]

lgtm

[dmazzoni, 2014-09-09 13:57:31]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-09 13:58:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/542553002/40001

[commit-bot: I haz the power, 2014-09-09 14:44:04]

Committed patchset #3 (id:40001) as 181649