Bug 228841 - [macOS] Web process crashes when detaching Document with uncommitted marked text
Summary: [macOS] Web process crashes when detaching Document with uncommitted marked text
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: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-05 13:12 PDT by Wenson Hsieh
Modified: 2021-08-07 15:47 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.53 KB, patch)
2021-08-05 13:38 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (6.61 KB, patch)
2021-08-05 15:46 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2021-08-05 18:23 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (8.42 KB, patch)
2021-08-06 08:54 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].