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.
Created attachment 441951 [details] Patch
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.
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.
It looks like this is stale now after https://trac.webkit.org/changeset/284780/webkit
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.
(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.
(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.
Created attachment 442621 [details] Patch
<rdar://problem/84731205>
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.
Created attachment 442720 [details] Patch
(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.
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 { };
Created attachment 442723 [details] Patch
(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 { };.
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].