Changeset 249444 in webkit

Timestamp:

Sep 3, 2019 4:21:38 PM (5 years ago)

Author:

timothy_horton@apple.com

Message:

Null deref under -[WKWebView _addUpdateVisibleContentRectPreCommitHandler]'s handler block
​https://bugs.webkit.org/show_bug.cgi?id=201436
<rdar://problem/40640475>

Reviewed by Simon Fraser.

• UIProcess/API/Cocoa/WKWebView.mm:

(-[WKWebView dealloc]):
(-[WKWebView _addUpdateVisibleContentRectPreCommitHandler]):
We crash sending a message to a deallocated WKWebView inside the handler block
passed to +[CATransaction addCommitHandler:]. This seems impossible, because
we carefully retain it, but it's possible that it could be the result of
the handler block being installed under -dealloc (in which case retaining
the WKWebView wouldn't actually extend its lifetime). -[WKWebView dealloc]
is fairly sizable, and it's hard to follow all paths from it, so instead
add a RELEASE_LOG_FAULT, so we'll get simulated crash logs, and bail,
so we'll stop actually crashing (if this is the cause).

This is just a speculative fix, but a hopeful one, since intentionally calling
-_addUpdateVisibleContentRectPreCommitHandler: from dealloc yields a similar-looking
crash under the handler block.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebView.mm (modified) (3 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-09-03  Tim Horton  <timothy_horton@apple.com>
+
+        Null deref under -[WKWebView _addUpdateVisibleContentRectPreCommitHandler]'s handler block
+        https://bugs.webkit.org/show_bug.cgi?id=201436
+        <rdar://problem/40640475>
+
+        Reviewed by Simon Fraser.
+
+        * UIProcess/API/Cocoa/WKWebView.mm:
+        (-[WKWebView dealloc]):
+        (-[WKWebView _addUpdateVisibleContentRectPreCommitHandler]):
+        We crash sending a message to a deallocated WKWebView inside the handler block
+        passed to +[CATransaction addCommitHandler:]. This seems impossible, because
+        we carefully retain it, but it's possible that it could be the result of
+        the handler block being installed under -dealloc (in which case retaining
+        the WKWebView wouldn't actually extend its lifetime). -[WKWebView dealloc]
+        is fairly sizable, and it's hard to follow all paths from it, so instead
+        add a RELEASE_LOG_FAULT, so we'll get simulated crash logs, and bail,
+        so we'll stop actually crashing (if this is the cause).
+
+        This is just a speculative fix, but a hopeful one, since intentionally calling
+        -_addUpdateVisibleContentRectPreCommitHandler: from dealloc yields a similar-looking
+        crash under the handler block.
+
 2019-09-03  Jiewen Tan  <jiewen_tan@apple.com>
 

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

@@ -375 +375 @@
     NSUInteger _focusPreservationCount;
     NSUInteger _activeFocusedStateRetainCount;
+
+    BOOL _hasEnteredDealloc;
 #endif
 #if PLATFORM(MAC)
@@ -875 +877 @@
 
 #if PLATFORM(IOS_FAMILY)
+    _hasEnteredDealloc = YES;
+
     [_contentView _webViewDestroyed];
 
@@ -3012 +3016 @@
 - (void)_addUpdateVisibleContentRectPreCommitHandler
 {
+    if (_hasEnteredDealloc) {
+        RELEASE_LOG_FAULT(ViewState, "-[WKWebView %p _addUpdateVisibleContentRectPreCommitHandler]: Attempted to add pre-commit handler under -dealloc. Bailing.", self);
+        return;
+    }
+
     auto retainedSelf = retainPtr(self);
     [CATransaction addCommitHandler:[retainedSelf] {