Bug 235184 – Crash in Document::updateStyleIfNeeded() when removing a node containing the drag caret

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

[Wenson Hsieh]

rdar://74845918

[Wenson Hsieh]

Created attachment 449063 [details]
Test case

[Wenson Hsieh]

Created attachment 449069 [details]
For EWS

[Darin Adler]

Comment on attachment 449069 [details]
For EWS

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

> Source/WebCore/editing/FrameSelection.cpp:543
> -    clear();
> +    if (RefPtr node = m_position.deepEquivalent().anchorNode())
> +        invalidateCaretRect(node.get(), true);
> +
> +    m_position = { };
> +    clearCaretRect();

I worry that nothing is left behind here other than the regression test to explain the subtle reason why this must not call clear().

Is there any way we can make this subtle point more obvious? Function naming? Some discipline across this entire class that more explicitly addresses repaint issues? Some kind of comment?

[Wenson Hsieh]

Comment on attachment 449069 [details]
For EWS

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

Thanks for the review!

>> Source/WebCore/editing/FrameSelection.cpp:543
>> +    clearCaretRect();
> 
> I worry that nothing is left behind here other than the regression test to explain the subtle reason why this must not call clear().
> 
> Is there any way we can make this subtle point more obvious? Function naming? Some discipline across this entire class that more explicitly addresses repaint issues? Some kind of comment?

That's a good point — perhaps I could factor this piece of logic out into helper method named something like, "clearCaretPositionWithoutUpdatingStyle()"?

[Darin Adler]

(In reply to Wenson Hsieh from comment #4)
> perhaps I could factor this piece of logic out into
> helper method named something like,
> "clearCaretPositionWithoutUpdatingStyle()"?

I love that.

My favorite type of comment is a good function name.

[Darin Adler]

Still might add a *brief* comment at the call site stating *why* it’s important to not update style in this case (and, by implication, why it’s not important in other cases).

[Wenson Hsieh]

(In reply to Darin Adler from comment #6)
> Still might add a *brief* comment at the call site stating *why* it’s
> important to not update style in this case (and, by implication, why it’s
> not important in other cases).

Sounds good! Will pull it into the helper method, and add a brief comment explaining that it's unsafe to update style here due to being in the middle of node removal.

[Wenson Hsieh]

Created attachment 449089 [details]
For landing

[EWS]

Committed r287992 (246019@main): <https://commits.webkit.org/246019@main>

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