Bug 228841

Summary: [macOS] Web process crashes when detaching Document with uncommitted marked text
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, ews-watchlist, megan_gardner, mifenton, rniwa, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

Description Wenson Hsieh 2021-08-05 13:12:46 PDT
rdar://79960890
Comment 1 Wenson Hsieh 2021-08-05 13:38:52 PDT
Created attachment 435020 [details]
Patch
Comment 2 Ryosuke Niwa 2021-08-05 15:18:25 PDT
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?
Comment 3 Wenson Hsieh 2021-08-05 15:33:18 PDT
(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.
Comment 4 Wenson Hsieh 2021-08-05 15:46:25 PDT
Created attachment 435033 [details]
Patch
Comment 5 Darin Adler 2021-08-05 17:44:55 PDT
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?
Comment 6 Wenson Hsieh 2021-08-05 17:46:53 PDT
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.
Comment 7 Wenson Hsieh 2021-08-05 18:23:51 PDT
Created attachment 435045 [details]
Patch
Comment 8 Wenson Hsieh 2021-08-05 21:47:34 PDT
Comment on attachment 435045 [details]
Patch

Looks like the test needs a minor adjustment to work with DumpRenderTree in legacy WebKit.
Comment 9 Wenson Hsieh 2021-08-06 08:54:05 PDT
Created attachment 435070 [details]
Patch
Comment 10 Wenson Hsieh 2021-08-07 09:40:41 PDT
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)
Comment 11 Wenson Hsieh 2021-08-07 15:12:47 PDT
Comment on attachment 435070 [details]
Patch

Thanks for the review!
Comment 12 EWS 2021-08-07 15:47:18 PDT
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].