Occasional crash under WebPage::sendEditorStateUpdate() when discardi…

…ng 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
WebKit · Jul 6, 2023 · b72851e · b72851e
1 parent 4660c0d
commit b72851e
Show file tree

Hide file tree

Showing 10 changed files with 49 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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<EditorClient> {
 
     // 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
@@ -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
@@ -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
@@ -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
@@ -999,7 +999,7 @@ class WebPage : public API::ObjectImpl<API::Object::Type::BundlePage>, 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
@@ -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
@@ -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
@@ -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:@"<body>Hello world</body>"];
+        [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)