[Mac WK2] Fix null dereference in asynchronous NSTextInputClient methods when dealloc...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jul 2017 19:57:12 +0000 (19:57 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Jul 2017 19:57:12 +0000 (19:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174751
<rdar://problem/33132405>

Reviewed by Darin Adler.

Tweaks -[WKWebView dealloc] to close the WebPageProxy at an earlier time, prior to destroying the WebViewImpl.
This fixes a NSTextInputClient crash in WKWebView when exercising the following scenario:

(1) Suppose that NSTextInputContext invokes an asynchronous text input query on WKWebView immediately before
WKWebView is deallocated, such that WebPageProxy's CallbackMap contains an NSTextInputContext callback at the
time that -[WKWebView dealloc] is called. Additionally, suppose that this callback from NSTextInputContext
invokes additional NSTextInputClient methods on the WKWebView that involve plumbing through to the WebViewImpl
(which is stored as _impl on the WKWebView).

(2) Observe that when calling [super dealloc] in [WKWebView dealloc], we will destroy the WebViewImpl as a
result of setting our unique pointer to _impl to be null. In ~WebViewImpl, we invoke WebPageProxy::close, which
in turn invokes WebPageProxy::resetState.

(3) WebPageProxy::resetState then calls m_callbacks.invalidate(error), which triggers all pending callbacks.
This invokes the block described in (1), which causes us to try and call back into WKWebView, invoking
NSTextInputClient methods. Without the fix in this patch, these methods currently assume that _impl is nonnull,
even though we've already cleared out the pointer in (2), so we segfault with a null dereference.

After this patch, we close the _page at an earlier time, such that the state is reset before the WebViewImpl
(and corresponding _impl unique_ptr in WKWebView) is torn down. This ensures that _impl will not be null for
callbacks invoked after beginning to deallocate the WKWebView.

Forcing this scenario in a custom AppKit root that triggers async NSTextInputClient methods immediately when a
WKWebView is being deallocated produces a crash with the same stack trace as what we observe in the radar, but
there are no known steps to actually reproduce this crash in shipping software.

* UIProcess/API/Cocoa/WKWebView.mm:
(-[WKWebView dealloc]):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219765 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm

index 1d2d757348f213e990eba8a6e13ca5f2ed27a6c0..b31f566ccb67be4c0d8d40847083c60099aa1e64 100644 (file)
@@ -1,3 +1,40 @@
+2017-07-22  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        [Mac WK2] Fix null dereference in asynchronous NSTextInputClient methods when deallocating a WKWebView
+        https://bugs.webkit.org/show_bug.cgi?id=174751
+        <rdar://problem/33132405>
+
+        Reviewed by Darin Adler.
+
+        Tweaks -[WKWebView dealloc] to close the WebPageProxy at an earlier time, prior to destroying the WebViewImpl.
+        This fixes a NSTextInputClient crash in WKWebView when exercising the following scenario:
+
+        (1) Suppose that NSTextInputContext invokes an asynchronous text input query on WKWebView immediately before
+        WKWebView is deallocated, such that WebPageProxy's CallbackMap contains an NSTextInputContext callback at the
+        time that -[WKWebView dealloc] is called. Additionally, suppose that this callback from NSTextInputContext
+        invokes additional NSTextInputClient methods on the WKWebView that involve plumbing through to the WebViewImpl
+        (which is stored as _impl on the WKWebView).
+
+        (2) Observe that when calling [super dealloc] in [WKWebView dealloc], we will destroy the WebViewImpl as a
+        result of setting our unique pointer to _impl to be null. In ~WebViewImpl, we invoke WebPageProxy::close, which
+        in turn invokes WebPageProxy::resetState.
+
+        (3) WebPageProxy::resetState then calls m_callbacks.invalidate(error), which triggers all pending callbacks.
+        This invokes the block described in (1), which causes us to try and call back into WKWebView, invoking
+        NSTextInputClient methods. Without the fix in this patch, these methods currently assume that _impl is nonnull,
+        even though we've already cleared out the pointer in (2), so we segfault with a null dereference.
+
+        After this patch, we close the _page at an earlier time, such that the state is reset before the WebViewImpl
+        (and corresponding _impl unique_ptr in WKWebView) is torn down. This ensures that _impl will not be null for
+        callbacks invoked after beginning to deallocate the WKWebView.
+
+        Forcing this scenario in a custom AppKit root that triggers async NSTextInputClient methods immediately when a
+        WKWebView is being deallocated produces a crash with the same stack trace as what we observe in the radar, but
+        there are no known steps to actually reproduce this crash in shipping software.
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView dealloc]):
+
 2017-07-22  Chris Dumez  <cdumez@apple.com>
 
         REGRESSION(r204565): WKObject is broken
index 7da4d60c4d649b13d1f364352dc20db30390485d..2419555a5385fe0e968f10ae7c37f93758c6ae65 100644 (file)
@@ -683,14 +683,14 @@ static uint32_t convertSystemLayoutDirection(NSUserInterfaceLayoutDirection dire
 
     if (_remoteObjectRegistry)
         _page->process().processPool().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID());
+#endif
 
     _page->close();
 
+#if PLATFORM(IOS)
     [_remoteObjectRegistry _invalidate];
     [[_configuration _contentProviderRegistry] removePage:*_page];
-#if PLATFORM(IOS)
     CFNotificationCenterRemoveObserver(CFNotificationCenterGetDarwinNotifyCenter(), (__bridge const void *)(self), nullptr, nullptr);
-#endif
     [[NSNotificationCenter defaultCenter] removeObserver:self];
     [_scrollView setInternalDelegate:nil];
 #endif