From b72851e83e5aec900b8d6a49d6be0614e3e6f586 Mon Sep 17 00:00:00 2001 From: Wenson Hsieh Date: Thu, 6 Jul 2023 13:30:40 -0700 Subject: [PATCH] Occasional crash under WebPage::sendEditorStateUpdate() when discarding IME composition https://bugs.webkit.org/show_bug.cgi?id=258942 rdar://111258989 Reviewed by Tim Horton. Address a web content process crash, wherein we attempt to access `Page::m_focusController` once it has already been cleared out. This can occur when a page in the page cache is destroyed, while it still contains an active IME composition (i.e. `Editor::m_compositionNode` is non-null). Mitigate this by only computing and sending an editor state update in this case, if the `Document` containing the marked text range has a living render tree. To do this, we refactor the `discardedComposition` `EditorClient` hook to pass a `const Document&` instead of a `Frame*` (the latter of which ends up being null at this point during page destruction), and use it to return early in `WebPage::discardedComposition` to avoid the crash. Test: KeyboardInputTests.NoCrashWhenDiscardingMarkedText * Source/WebCore/editing/Editor.cpp: (WebCore::Editor::clear): (WebCore::Editor::confirmOrCancelCompositionAndNotifyClient): * Source/WebCore/loader/EmptyClients.cpp: * Source/WebCore/page/EditorClient.h: * Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp: (WebKit::WebEditorClient::discardedComposition): * Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h: * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::didChangeSelectionOrOverflowScrollPosition): (WebKit::WebPage::discardedComposition): * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h: * Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm: (WebEditorClient::discardedComposition): * Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm: (TestWebKitAPI::TEST): Canonical link: https://commits.webkit.org/265821@main --- Source/WebCore/editing/Editor.cpp | 4 +-- Source/WebCore/loader/EmptyClients.cpp | 2 +- Source/WebCore/page/EditorClient.h | 3 +- .../WebCoreSupport/WebEditorClient.cpp | 4 +-- .../WebCoreSupport/WebEditorClient.h | 2 +- Source/WebKit/WebProcess/WebPage/WebPage.cpp | 8 +++-- Source/WebKit/WebProcess/WebPage/WebPage.h | 2 +- .../mac/WebCoreSupport/WebEditorClient.h | 2 +- .../mac/WebCoreSupport/WebEditorClient.mm | 2 +- .../Tests/ios/KeyboardInputTestsIOS.mm | 32 +++++++++++++++++++ 10 files changed, 49 insertions(+), 12 deletions(-) diff --git a/Source/WebCore/editing/Editor.cpp b/Source/WebCore/editing/Editor.cpp index cfbe98a5f00e..5a46dc58e2d3 100644 --- a/Source/WebCore/editing/Editor.cpp +++ b/Source/WebCore/editing/Editor.cpp @@ -1340,7 +1340,7 @@ void Editor::clear() if (m_compositionNode) { m_compositionNode = nullptr; if (EditorClient* client = this->client()) - client->discardedComposition(m_document.frame()); + client->discardedComposition(m_document); } m_customCompositionUnderlines.clear(); m_customCompositionHighlights.clear(); @@ -2119,7 +2119,7 @@ void Editor::confirmOrCancelCompositionAndNotifyClient() if (auto editorClient = client()) { editorClient->respondToChangedSelection(frame.get()); - editorClient->discardedComposition(frame.get()); + editorClient->discardedComposition(m_document); } } diff --git a/Source/WebCore/loader/EmptyClients.cpp b/Source/WebCore/loader/EmptyClients.cpp index 1233aaffa34c..bf3363e7e26b 100644 --- a/Source/WebCore/loader/EmptyClients.cpp +++ b/Source/WebCore/loader/EmptyClients.cpp @@ -277,7 +277,7 @@ class EmptyEditorClient final : public EditorClient { void respondToChangedContents() final { } void respondToChangedSelection(LocalFrame*) final { } void updateEditorStateAfterLayoutIfEditabilityChanged() final { } - void discardedComposition(LocalFrame*) final { } + void discardedComposition(const Document&) final { } void canceledComposition() final { } void didUpdateComposition() final { } void didEndEditing() final { } diff --git a/Source/WebCore/page/EditorClient.h b/Source/WebCore/page/EditorClient.h index 2c1e51ca933e..986565614ac5 100644 --- a/Source/WebCore/page/EditorClient.h +++ b/Source/WebCore/page/EditorClient.h @@ -41,6 +41,7 @@ enum class DOMPasteAccessCategory : uint8_t; enum class DOMPasteAccessResponse : uint8_t; class SharedBuffer; +class Document; class DocumentFragment; class Element; class KeyboardEvent; @@ -108,7 +109,7 @@ class EditorClient : public CanMakeWeakPtr { // Notify an input method that a composition was voluntarily discarded by WebCore, so that it could clean up too. // This function is not called when a composition is closed per a request from an input method. - virtual void discardedComposition(LocalFrame*) = 0; + virtual void discardedComposition(const Document&) = 0; virtual void canceledComposition() = 0; virtual void didUpdateComposition() = 0; diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp index 1635d4232449..057530fb6879 100644 --- a/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp +++ b/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.cpp @@ -240,9 +240,9 @@ void WebEditorClient::didUpdateComposition() m_page->didUpdateComposition(); } -void WebEditorClient::discardedComposition(LocalFrame*) +void WebEditorClient::discardedComposition(const Document& document) { - m_page->discardedComposition(); + m_page->discardedComposition(document); } void WebEditorClient::canceledComposition() diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h b/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h index 61d0ce235bf3..d45cbfd19693 100644 --- a/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h +++ b/Source/WebKit/WebProcess/WebCoreSupport/WebEditorClient.h @@ -82,7 +82,7 @@ class WebEditorClient final : public WebCore::EditorClient, public WebCore::Text void respondToChangedSelection(WebCore::LocalFrame*) final; void didEndUserTriggeredSelectionChanges() final; void updateEditorStateAfterLayoutIfEditabilityChanged() final; - void discardedComposition(WebCore::LocalFrame*) final; + void discardedComposition(const WebCore::Document&) final; void canceledComposition() final; void didUpdateComposition() final; void didEndEditing() final; diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp index 801eb5cee2bc..5d902e2455d2 100644 --- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp @@ -6734,7 +6734,8 @@ void WebPage::didChangeSelectionOrOverflowScrollPosition() // FIXME: We can't cancel composition when selection changes to NoSelection, but we probably should. if (frame->editor().hasComposition() && !frame->editor().ignoreSelectionChanges() && !frame->selection().isNone()) { frame->editor().cancelComposition(); - discardedComposition(); + if (RefPtr document = frame->document()) + discardedComposition(*document); return; } #endif // HAVE(TOUCH_BAR) @@ -6878,9 +6879,12 @@ void WebPage::didEndUserTriggeredSelectionChanges() sendEditorStateUpdate(); } -void WebPage::discardedComposition() +void WebPage::discardedComposition(const Document& document) { send(Messages::WebPageProxy::CompositionWasCanceled()); + if (!document.hasLivingRenderTree()) + return; + sendEditorStateUpdate(); } diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h index ce67c1731365..1cb8e6d089c1 100644 --- a/Source/WebKit/WebProcess/WebPage/WebPage.h +++ b/Source/WebKit/WebProcess/WebPage/WebPage.h @@ -999,7 +999,7 @@ class WebPage : public API::ObjectImpl, public IP void didChangeSelection(WebCore::LocalFrame&); void didChangeOverflowScrollPosition(); void didChangeContents(); - void discardedComposition(); + void discardedComposition(const WebCore::Document&); void canceledComposition(); void didUpdateComposition(); void didEndUserTriggeredSelectionChanges(); diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h b/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h index 5989b7646294..b63afdf35187 100644 --- a/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h +++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.h @@ -113,7 +113,7 @@ class WebEditorClient final : public WebCore::EditorClient, public WebCore::Text void respondToChangedSelection(WebCore::LocalFrame*) final; void didEndUserTriggeredSelectionChanges() final { } void updateEditorStateAfterLayoutIfEditabilityChanged() final; - void discardedComposition(WebCore::LocalFrame*) final; + void discardedComposition(const WebCore::Document&) final; void canceledComposition() final; void didUpdateComposition() final { } diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm b/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm index 991d8c8d8b2c..b3e2f9d4d196 100644 --- a/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm +++ b/Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm @@ -384,7 +384,7 @@ static void updateFontPanel(WebView *webView) #endif // ENABLE(TEXT_CARET) } -void WebEditorClient::discardedComposition(LocalFrame*) +void WebEditorClient::discardedComposition(const Document&) { // The effects of this function are currently achieved via -[WebHTMLView _updateSelectionForInputManager]. } diff --git a/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm b/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm index 472c7376b3c9..53be1b375154 100644 --- a/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm +++ b/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm @@ -1017,6 +1017,38 @@ static BOOL shouldSimulateKeyboardInputOnTextInsertionOverride(id, SEL) EXPECT_TRUE(didStartInputSession); } +TEST(KeyboardInputTests, NoCrashWhenDiscardingMarkedText) +{ + auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]); + [processPoolConfiguration setProcessSwapsOnNavigation:YES]; + + auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]); + auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]); + [configuration setProcessPool:processPool.get()]; + + auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]); + auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:configuration.get()]); + [webView setNavigationDelegate:navigationDelegate.get()]; + [webView _setEditable:YES]; + + auto navigateAndSetMarkedText = [&](const String& urlString) { + auto request = [NSURLRequest requestWithURL:[NSURL URLWithString:(NSString *)urlString]]; + [webView loadSimulatedRequest:request responseHTMLString:@"Hello world"]; + [navigationDelegate waitForDidFinishNavigation]; + [webView selectAll:nil]; + [[webView textInputContentView] setMarkedText:@"Hello" selectedRange:NSMakeRange(0, 5)]; + [webView waitForNextPresentationUpdate]; + }; + + navigateAndSetMarkedText("https://foo.com"_s); + navigateAndSetMarkedText("https://bar.com"_s); + navigateAndSetMarkedText("https://baz.com"_s); + navigateAndSetMarkedText("https://foo.com"_s); + [webView _close]; + + Util::runFor(100_ms); +} + } // namespace TestWebKitAPI #endif // PLATFORM(IOS_FAMILY)