Bug 228841 – [macOS] Web process crashes when detaching Document with uncommitted marked text

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

rdar://79960890

[Wenson Hsieh]

Created attachment 435020 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 435020 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:1241
> -        if (EditorClient* client = this->client())
> +        if (auto client = this->client(); client && m_document.hasLivingRenderTree())
>              client->discardedComposition(m_document.frame());

This seems like a risky change given WK1 delegate callbacks will be affected by it.
Also, there are other reasons the document may not have a living render tree like going into the page cache.
Can we exit early in WK2 client code instead?

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 435020 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435020&action=review
> 
> > Source/WebCore/editing/Editor.cpp:1241
> > -        if (EditorClient* client = this->client())
> > +        if (auto client = this->client(); client && m_document.hasLivingRenderTree())
> >              client->discardedComposition(m_document.frame());
> 
> This seems like a risky change given WK1 delegate callbacks will be affected
> by it.
> Also, there are other reasons the document may not have a living render tree
> like going into the page cache.
> Can we exit early in WK2 client code instead?

Good point — I'll move the check there.

[Wenson Hsieh]

Created attachment 435033 [details]
Patch

[Darin Adler]

Comment on attachment 435033 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp:260
> -void WebEditorClient::discardedComposition(Frame*)
> +void WebEditorClient::discardedComposition(Frame* frame)
>  {
> +    if (!frame)
> +        return;
> +
> +    if (!frame->document())
> +        return;
> +
> +    if (!frame->document()->hasLivingRenderTree())
> +        return;
> +
>      m_page->discardedComposition();
>  }

This doesn’t feel quite right. The solution seems too far from the problem, an indirect way of side stepping the bad thing.

Could these checks be in sendEditorStateUpdate instead?

[Wenson Hsieh]

Comment on attachment 435033 [details]
Patch

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

>> Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp:260
>>  }
> 
> This doesn’t feel quite right. The solution seems too far from the problem, an indirect way of side stepping the bad thing.
> 
> Could these checks be in sendEditorStateUpdate instead?

They could, but that would be a larger behavior change. That said, it's probably a good idea nonetheless, since we'd *probably* just crash otherwise when computing editor state. I'll give this a try.

[Wenson Hsieh]

Created attachment 435045 [details]
Patch

[Wenson Hsieh]

Comment on attachment 435045 [details]
Patch

Looks like the test needs a minor adjustment to work with DumpRenderTree in legacy WebKit.

[Wenson Hsieh]

Created attachment 435070 [details]
Patch

[Wenson Hsieh]

Comment on attachment 435070 [details]
Patch

> Found 2 new test failures: fast/events/dropzone-005.html, fast/forms/search/search-zoom-computed-style-height.html

The Windows test failures are unrelated (they're also showing up in other EWS runs in https://ews-build.webkit.org/#/builders/10)

[Wenson Hsieh]

Comment on attachment 435070 [details]
Patch

Thanks for the review!

[EWS]

Committed r280762 (240347@main): <https://commits.webkit.org/240347@main>

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