Changeset 219765 in webkit


Ignore:
Timestamp:
Jul 22, 2017 12:57:12 PM (7 years ago)
Author:
Wenson Hsieh
Message:

[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]):

Location:
trunk/Source/WebKit
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r219764 r219765  
     12017-07-22  Wenson Hsieh  <wenson_hsieh@apple.com>
     2
     3        [Mac WK2] Fix null dereference in asynchronous NSTextInputClient methods when deallocating a WKWebView
     4        https://bugs.webkit.org/show_bug.cgi?id=174751
     5        <rdar://problem/33132405>
     6
     7        Reviewed by Darin Adler.
     8
     9        Tweaks -[WKWebView dealloc] to close the WebPageProxy at an earlier time, prior to destroying the WebViewImpl.
     10        This fixes a NSTextInputClient crash in WKWebView when exercising the following scenario:
     11
     12        (1) Suppose that NSTextInputContext invokes an asynchronous text input query on WKWebView immediately before
     13        WKWebView is deallocated, such that WebPageProxy's CallbackMap contains an NSTextInputContext callback at the
     14        time that -[WKWebView dealloc] is called. Additionally, suppose that this callback from NSTextInputContext
     15        invokes additional NSTextInputClient methods on the WKWebView that involve plumbing through to the WebViewImpl
     16        (which is stored as _impl on the WKWebView).
     17
     18        (2) Observe that when calling [super dealloc] in [WKWebView dealloc], we will destroy the WebViewImpl as a
     19        result of setting our unique pointer to _impl to be null. In ~WebViewImpl, we invoke WebPageProxy::close, which
     20        in turn invokes WebPageProxy::resetState.
     21
     22        (3) WebPageProxy::resetState then calls m_callbacks.invalidate(error), which triggers all pending callbacks.
     23        This invokes the block described in (1), which causes us to try and call back into WKWebView, invoking
     24        NSTextInputClient methods. Without the fix in this patch, these methods currently assume that _impl is nonnull,
     25        even though we've already cleared out the pointer in (2), so we segfault with a null dereference.
     26
     27        After this patch, we close the _page at an earlier time, such that the state is reset before the WebViewImpl
     28        (and corresponding _impl unique_ptr in WKWebView) is torn down. This ensures that _impl will not be null for
     29        callbacks invoked after beginning to deallocate the WKWebView.
     30
     31        Forcing this scenario in a custom AppKit root that triggers async NSTextInputClient methods immediately when a
     32        WKWebView is being deallocated produces a crash with the same stack trace as what we observe in the radar, but
     33        there are no known steps to actually reproduce this crash in shipping software.
     34
     35        * UIProcess/API/Cocoa/WKWebView.mm:
     36        (-[WKWebView dealloc]):
     37
    1382017-07-22  Chris Dumez  <cdumez@apple.com>
    239
  • trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm

    r219594 r219765  
    684684    if (_remoteObjectRegistry)
    685685        _page->process().processPool().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID());
     686#endif
    686687
    687688    _page->close();
    688689
     690#if PLATFORM(IOS)
    689691    [_remoteObjectRegistry _invalidate];
    690692    [[_configuration _contentProviderRegistry] removePage:*_page];
    691 #if PLATFORM(IOS)
    692693    CFNotificationCenterRemoveObserver(CFNotificationCenterGetDarwinNotifyCenter(), (__bridge const void *)(self), nullptr, nullptr);
    693 #endif
    694694    [[NSNotificationCenter defaultCenter] removeObserver:self];
    695695    [_scrollView setInternalDelegate:nil];
Note: See TracChangeset for help on using the changeset viewer.