[webkit-reviews] review granted: [Bug 236378] Check bidiLevels are valid before reordering : [Attachment 451397] Patch

[bugzilla-daemon at webkit.org]

zalan <zalan at apple.com> has granted Brandon <brandonstewart at apple.com>'s
request for review:
Bug 236378: Check bidiLevels are valid before reordering
https://bugs.webkit.org/show_bug.cgi?id=236378

Attachment 451397: Patch

https://bugs.webkit.org/attachment.cgi?id=451397&action=review




--- Comment #2 from zalan <zalan at apple.com> ---
Comment on attachment 451397
  --> https://bugs.webkit.org/attachment.cgi?id=451397
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451397&action=review

> Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp:350
> +
> +	       // bidiLevels are required to be less than the MAX + 1,
otherwise
> +	       // ubidi_reorderVisual will silently fail.
> +	       if (lineRuns[i].bidiLevel() > UBIDI_MAX_EXPLICIT_LEVEL + 1)
> +		   continue;
> +

Great patch! What happens here is that 
1. an empty DOM text node (length = 0) generates an InlineTextItem with zero
length
2. this InlineTextItem gets ignored at
InlineItemsBuilder::breakAndComputeBidiLevels() -this is ok, it's hard to
assign a bidi level for a 0 length content.
3. this zero length InlineTextItem enters the bidi reordering with the default
UBIDI_DEFAULT_LTR value (254, see InlineItem's c'tor) which is greater than
UBIDI_MAX_EXPLICIT_LEVEL (125)
I'd slightly change this patch by adding the following asserts 
ASSERT(lineRuns[I].bidiLevel() == UBIDI_DEFAULT_LTR);
ASSERT(!downcast<InlineTextItem>(lineRuns[I]).length());
as we do not expect any other type of content with such bidi level.