Bug 209021 - Crash in TextManipulationController::replace
Summary: Crash in TextManipulationController::replace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-12 15:02 PDT by Ryosuke Niwa
Modified: 2020-03-12 16:31 PDT (History)
5 users (show)

See Also:

Attachments
Fixes the bug (7.58 KB, patch)
2020-03-12 15:16 PDT, Ryosuke Niwa 		wenson_hsieh: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2020-03-12 15:02:16 PDT 
TextManipulationController::replace can crash when accessing TextIterator::node and other places.
Comment 1 Ryosuke Niwa 2020-03-12 15:16:30 PDT 
Created attachment 393421 [details]
Fixes the bug
Comment 2 Wenson Hsieh 2020-03-12 15:41:18 PDT 
Comment on attachment 393421 [details]
Fixes the bug

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

> Source/WebCore/editing/TextManipulationController.cpp:580
> +    Position insertionPoint = positionBeforeNode(firstContentNode.get()).parentAnchoredEquivalent();

Is it possible `firstContentNode` is null here? That would lead to a debug assertion in positionBeforeNode.


(Seems like the answer is no because we’d already bail in the early return above…?)
Comment 3 Ryosuke Niwa 2020-03-12 15:45:56 PDT 
Comment on attachment 393421 [details]
Fixes the bug

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

>> Source/WebCore/editing/TextManipulationController.cpp:580
>> +    Position insertionPoint = positionBeforeNode(firstContentNode.get()).parentAnchoredEquivalent();
> 
> Is it possible `firstContentNode` is null here? That would lead to a debug assertion in positionBeforeNode.
> 
> 
> (Seems like the answer is no because we’d already bail in the early return above…?)

I don't think so because to get here, we must have had at least one token,
which means we must have had at least one content node.
Comment 4 Ryosuke Niwa 2020-03-12 16:29:32 PDT 
Committed r258371: <https://trac.webkit.org/changeset/258371>
Comment 5 Ryosuke Niwa 2020-03-12 16:29:57 PDT 
<rdar://problem/60376855>