Changeset 249690 in webkit

Timestamp:

Sep 9, 2019, 8:19:35 PM (7 years ago)

Author:

Alan Coon

Message:

Cherry-pick r249444. rdar://problem/55093558

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.

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

Location:

branches/safari-608-branch/Source/WebKit

Files:

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

branches/safari-608-branch/Source/WebKit/ChangeLog

@@ +1 @@
+2019-09-09  Kocsen Chung  <kocsen_chung@apple.com>
+
+        Cherry-pick r249444. rdar://problem/55093558
+
+    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.
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249444 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    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-05  Kocsen Chung  <kocsen_chung@apple.com>
 

branches/safari-608-branch/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] {