Summary: | [macOS] Web process crashes when detaching Document with uncommitted marked text | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
Component: | HTML Editing | Assignee: | 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
Wenson Hsieh
2021-08-05 13:12:46 PDT
Created attachment 435020 [details]
Patch
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? (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. Created attachment 435033 [details]
Patch
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 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. Created attachment 435045 [details]
Patch
Comment on attachment 435045 [details]
Patch
Looks like the test needs a minor adjustment to work with DumpRenderTree in legacy WebKit.
Created attachment 435070 [details]
Patch
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 on attachment 435070 [details]
Patch
Thanks for the review!
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]. |