CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions

[dcrousso]

0cc930a

CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions
https://bugs.webkit.org/show_bug.cgi?id=239909
<rdar://problem/87885717 >

Reviewed by Wenson Hsieh.

This exception happens when trying to add attributes for text that contains collapsed whitespace and
has also been wrapped due to the size of its parent container. The exception specifically is about
trying to add attributes beyond the current length of a `NSAttributedString`.

* Source/WebCore/editing/TextIteratorBehavior.h:
* Source/WebCore/editing/TextIterator.cpp:
(WebCore::TextIterator::handleTextRun):
In the case that `m_lastTextNodeEndedWithCollapsedSpace`, we only want to add the remaining text if
we're still within the desired portion of the `m_textRun`. Otherwise, we'll iterate over too much of
the text and result in a string that's longer than what would be the case if one manually calculated
it from the given `offset` and `offsetEnd`. Add a new `TextIteratorBehavior::IgnoresWhiteSpaceAtEndOfRun`
to not include the trailing whitespace.

* Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:
(WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions):
Use the new `WebCore::TextIteratorBehavior::IgnoresWhiteSpaceAtEndOfRun` to not include the trailing
whitespace.
Also add some defensive checks just in case.

* Source/WebCore/testing/Internals.idl:
* Source/WebCore/testing/Internals.h:
* Source/WebCore/testing/Internals.cpp:
(WebCore::toTextIteratorBehaviors): Added.
(WebCore::Internals::locationFromRange):
(WebCore::Internals::lengthFromRange):
(WebCore::Internals::statesOfTextIterator): Added.
Add a way to provide `TextIteratorBehaviors` to methods that use `TextIterator`.
Add a method that gets the `text` and `range` of a `TextIterator` after every `advance`.

* LayoutTests/editing/text-iterator/sequential-collapsed-ranges.html: Added.
* LayoutTests/editing/text-iterator/sequential-collapsed-ranges-expected.txt: Added.
* LayoutTests/editing/text-iterator/subrange-with-trailing-collapsed-whitespace.html: Added.
* LayoutTests/editing/text-iterator/subrange-with-trailing-collapsed-whitespace-expected.txt: Added.

Canonical link: https://commits.webkit.org/251061@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294955 268f45cc-cd09-0410-ab3c-d52691b4dbfc

[rniwa]

This exception happens when trying to add attributes for text that contains collapsed whitespace and has also been wrapped due to the size of its parent container. The exception specifically is about trying to add attributes beyond the current length of a NSAttributedString.

In the case that m_lastTextNodeEndedWithCollapsedSpace, we only want to add the remaining text if we're still within the desired portion of the m_textRun. Otherwise, we'll iterate over too much of the text and result in a string that's longer than what would be the case if one manually calculated it from the given offset and offsetEnd. Add a new TextIteratorBehavior::IgnoresWhiteSpaceAtEndOfRun to not include the trailing whitespace.

Please hard wrap these long lines.

[rniwa]

What if the next text run was also entirely consisting of whitespace?

[dcrousso]

I'm not sure I understand your question 😅

The idea behind this code is to do similar to what's done on :688 in that if we've reached the end of the desired range (i.e. runStart >= runEnd) then we should stop processing things and instead just keep iterating until we run out of text runs to iterate.

When I first read this code it just struck me as odd that we didn't check runStart < runEnd for the collapsed whitespace case. My gut is telling me that this should always be the behavior in all cases (i.e. we shouldn't be reading text beyond the end of the range we want), but I created TextIteratorBehavior::IgnoresWhiteSpaceAtEndOfRun to minimize any risk of other issues (since this is a fix for a crash, i wouldn't want the change to have to get rolled out because it caused some other problem).

[dcrousso]

i added some tests for this (assuming i understood your point 😅)

AFAICT TextIteratorBehavior::IgnoresWhiteSpaceAtEndOfRun does not affect multiple whitespace text runs in a row

[whsieh]

For new tests, we generally prefer using js-test.js instead of the pre/post scripts.

[whsieh]

(Ditto re: pre/post.js)

[webkit-early-warning-system]

Committed r294955 (251061@main): https://commits.webkit.org/251061@main

Reviewed commits have been landed. Closing PR #681 and removing active labels.

[dcrousso]

the above revision/link is wrong

it's actually r294959 https://commits.webkit.org/251062@main