Bug 239909 – CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Devin Rousso]

an `NSRangeException` is thrown inside `-[NSConcreteMutableAttributedString addAttribute:value:range:]`

[Devin Rousso]

<rdar://problem/87885717>

[Devin Rousso]

Created attachment 458605 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 458605 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions

A subtle TextIterator fix like this seems like it strongly requires a regression test.

[Devin Rousso]

Comment on attachment 458605 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions
> 
> A subtle TextIterator fix like this seems like it strongly requires a regression test.

yeah im working on that now

i put this up to let EWS munch on it :)

[Darin Adler]

Comment on attachment 458605 [details]
Patch

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

>>> Source/WebCore/ChangeLog:3
>>> +        CrashTracer: com.apple.WebKit.WebContent at com.apple.WebKit: WebKit::TextCheckingControllerProxy::annotatedSubstringBetweenPositions
>> 
>> A subtle TextIterator fix like this seems like it strongly requires a regression test.
> 
> yeah im working on that now
> 
> i put this up to let EWS munch on it :)

I think it would be best to have some TextIterator tests for this bug that do *not* involve NSAttributedString; this underlying issue is not specific to the NSAttributedString code path, and can be tested without involving the NSAttributedString code path.

I also think the NSAttributedString code path might benefit from additional defensive range checking rather than relying on the subtle and complex TextIterator machinery to never create something with an incorrect offset.

We may want to fix this bug both ways, with:
a) defensive code in the code that deals with NSAttributedString that would prevent this kind of incorrect text offset computation from causing a crash, and
b) this TextIterator change that gives us a correct result that is valuable in and of itself.

Unfortunately the (b) change would make it a little harder to test (a) since we would no longer be able to use this bug to test the defensive checks are necessary.

[Devin Rousso]

Created attachment 459466 [details]
Patch

[Devin Rousso]

Comment on attachment 459466 [details]
Patch

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

> Source/WebCore/editing/TextIterator.cpp:638
> +            if (runStart >= runEnd && m_behaviors.contains(TextIteratorBehavior::IgnoresWhiteSpaceAtEndOfRun)) {
> +                m_textRun = nextTextRun;
> +                continue;
> +            }

I think this is actually the correct behavior that should _always_ be used, but since this is a regression I'd rather target my fix to just that and then (perhaps) remove the check later on so that if there is a problem, rollouts are less conflicting (i.e. fixes one bug only to introduce another)

[Devin Rousso]

Created attachment 459472 [details]
Patch

[Devin Rousso]

Pull request: https://github.com/WebKit/WebKit/pull/681

[EWS]

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

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