Bug 232053 – Negative length returned by TextUtil::midWordBreak with surrogate pair

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Gabriel Nava Marino]

In the case where the surrogate pair overlaps with the start position, the computed right location could end up being one less than the startPosition yielding a -1 length value for the returned TextUtil::MidWordBreak.

[Gabriel Nava Marino]

Created attachment 441951 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 441951 [details]
Patch

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

> Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp:181
> +    return { startPosition, std::max<unsigned>(right, startPosition) - startPosition, leftSideWidth };

Do you have any more detail about the circumstances leading to this being negative? I'm not sure this is the right fix.

[Gabriel Nava Marino]

Comment on attachment 441951 [details]
Patch

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

>> Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp:181
>> +    return { startPosition, std::max<unsigned>(right, startPosition) - startPosition, leftSideWidth };
> 
> Do you have any more detail about the circumstances leading to this being negative? I'm not sure this is the right fix.

Per this comment in the line above, "right" is computed to point to the start of the surrogate pair.
> // When the substring does not fit, the right side is supposed to be the start of the surrogate pair if applicable.

If our text is 'a\ud800\udc00a' then "right" gets value 1 per the logic in U16_SET_CP_START since this is where the surrogate starts. Since startPosition is 2, we end up computing a length = right - startPosition = 1 - 2 = -1.

[Myles C. Maxfield]

It looks like this is stale now after https://trac.webkit.org/changeset/284780/webkit

[Myles C. Maxfield]

In fact, this bug seems fixed now on ToT. I ran the 3 tests in this patch and logged the values of right and startPosition, and right is always >= startPosition.

[Gabriel Nava Marino]

(In reply to Myles C. Maxfield from comment #5)
> In fact, this bug seems fixed now on ToT. I ran the 3 tests in this patch
> and logged the values of right and startPosition, and right is always >=
> startPosition.


Thank you for the clarification! I will then use this bug to submit the new test cases only.

[Gabriel Nava Marino]

(In reply to Myles C. Maxfield from comment #5)
> In fact, this bug seems fixed now on ToT. I ran the 3 tests in this patch
> and logged the values of right and startPosition, and right is always >=
> startPosition.

Looks like this test is still crashing:
fast/text/word-break-letter-spacing-utf16-surrogates.html
It is hitting the new RELEASE_ASSERT in TextUtil::breakWord added in the above patch.
    RELEASE_ASSERT(right >= startPosition);

I will investigate.

[Gabriel Nava Marino]

Created attachment 442621 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84731205>

[zalan]

Comment on attachment 442621 [details]
Patch

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

> Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp:178
> +            if (right < startPosition)
> +                right = surrogatePairAwareIndex(middle);

Shouldn't we just simply bail out at this point? We know that our middle point (which becomes the right) falls before the start position. We surely not going to find a non-empty fitting content anymore in here.

[Gabriel Nava Marino]

Created attachment 442720 [details]
Patch

[Gabriel Nava Marino]

(In reply to zalan from comment #10)
> Comment on attachment 442621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442621&action=review
> 
> > Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp:178
> > +            if (right < startPosition)
> > +                right = surrogatePairAwareIndex(middle);
> 
> Shouldn't we just simply bail out at this point? We know that our middle
> point (which becomes the right) falls before the start position. We surely
> not going to find a non-empty fitting content anymore in here.

Yes, I have incorporated this suggestion in the latest patch.

[zalan]

Comment on attachment 442720 [details]
Patch

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

> Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp:178
> +                return { 0, 0 };

nitpick: it's really not important but you could write it as return { };

[Gabriel Nava Marino]

Created attachment 442723 [details]
Patch

[Gabriel Nava Marino]

(In reply to zalan from comment #13)
> Comment on attachment 442720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442720&action=review
> 
> > Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp:178
> > +                return { 0, 0 };
> 
> nitpick: it's really not important but you could write it as return { };

Thank you for the suggestion. Let's use { };.

[EWS]

Committed r285016 (243660@main): <https://commits.webkit.org/243660@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442723 [details].