ASSERTION FAILED: m_messageReceivers.contains(...) under ViewGestureController remove...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 18 Nov 2018 00:25:20 +0000 (00:25 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 18 Nov 2018 00:25:20 +0000 (00:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191734
<rdar://problem/46151497>

Reviewed by Ryosuke Niwa.

Source/WebKit:

When a WebProcess crashes, we destroy the ViewGestureController and reconstruct it later
after we've relaunched a new WebProcess. The ViewGestureController controller takes care
of adding itself as an IPC message receiver to the WebProcessProxy, and the destructor
takes care of removing itself as an IPC message receiver.

However, when process-swapping on navigation, we do not destroy the ViewGestureController
because doing so would take down the swipe gesture snapshot on cross-site swipe navigation.
This led to hitting this assertion later on because the ViewGestureController is still
registered as an IPC message receiver with the old process after process swapping.

To address the issue, we now make sure the ViewGestureController unregisters itself from
the old process and registers itself with the new process on process-swap.

* UIProcess/Cocoa/ViewGestureController.cpp:
(WebKit::ViewGestureController::ViewGestureController):
(WebKit::ViewGestureController::~ViewGestureController):
(WebKit::ViewGestureController::disconnectFromProcess):
(WebKit::ViewGestureController::connectToProcess):
* UIProcess/Cocoa/ViewGestureController.h:
* UIProcess/Cocoa/WebViewImpl.mm:
(WebKit::WebViewImpl::processWillSwap):
(WebKit::WebViewImpl::didRelaunchProcess):

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/Cocoa/ViewGestureController.cpp
Source/WebKit/UIProcess/Cocoa/ViewGestureController.h
Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index a214c3a..eede130 100644 (file)
@@ -1,5 +1,36 @@
 2018-11-17  Chris Dumez  <cdumez@apple.com>
 
+        ASSERTION FAILED: m_messageReceivers.contains(...) under ViewGestureController removeMessageReceiver
+        https://bugs.webkit.org/show_bug.cgi?id=191734
+        <rdar://problem/46151497>
+
+        Reviewed by Ryosuke Niwa.
+
+        When a WebProcess crashes, we destroy the ViewGestureController and reconstruct it later
+        after we've relaunched a new WebProcess. The ViewGestureController controller takes care
+        of adding itself as an IPC message receiver to the WebProcessProxy, and the destructor
+        takes care of removing itself as an IPC message receiver.
+
+        However, when process-swapping on navigation, we do not destroy the ViewGestureController
+        because doing so would take down the swipe gesture snapshot on cross-site swipe navigation.
+        This led to hitting this assertion later on because the ViewGestureController is still
+        registered as an IPC message receiver with the old process after process swapping.
+
+        To address the issue, we now make sure the ViewGestureController unregisters itself from
+        the old process and registers itself with the new process on process-swap.
+
+        * UIProcess/Cocoa/ViewGestureController.cpp:
+        (WebKit::ViewGestureController::ViewGestureController):
+        (WebKit::ViewGestureController::~ViewGestureController):
+        (WebKit::ViewGestureController::disconnectFromProcess):
+        (WebKit::ViewGestureController::connectToProcess):
+        * UIProcess/Cocoa/ViewGestureController.h:
+        * UIProcess/Cocoa/WebViewImpl.mm:
+        (WebKit::WebViewImpl::processWillSwap):
+        (WebKit::WebViewImpl::didRelaunchProcess):
+
+2018-11-17  Chris Dumez  <cdumez@apple.com>
+
         [PSON] ASSERTION FAILED: m_uncommittedState.state == State::Committed
         https://bugs.webkit.org/show_bug.cgi?id=191781
 
index f14898c..8a70694 100644 (file)
@@ -63,7 +63,7 @@ ViewGestureController::ViewGestureController(WebPageProxy& webPageProxy)
     , m_pendingSwipeTracker(webPageProxy, *this)
 #endif
 {
-    m_webPageProxy.process().addMessageReceiver(Messages::ViewGestureController::messageReceiverName(), m_webPageProxy.pageID(), *this);
+    connectToProcess();
 
     viewGestureControllersForAllPages().add(webPageProxy.pageID(), this);
 }
@@ -74,7 +74,25 @@ ViewGestureController::~ViewGestureController()
 
     viewGestureControllersForAllPages().remove(m_webPageProxy.pageID());
 
+    disconnectFromProcess();
+}
+
+void ViewGestureController::disconnectFromProcess()
+{
+    if (!m_isConnectedToProcess)
+        return;
+
     m_webPageProxy.process().removeMessageReceiver(Messages::ViewGestureController::messageReceiverName(), m_webPageProxy.pageID());
+    m_isConnectedToProcess = false;
+}
+
+void ViewGestureController::connectToProcess()
+{
+    if (m_isConnectedToProcess)
+        return;
+
+    m_webPageProxy.process().addMessageReceiver(Messages::ViewGestureController::messageReceiverName(), m_webPageProxy.pageID(), *this);
+    m_isConnectedToProcess = true;
 }
 
 ViewGestureController* ViewGestureController::controllerForGesture(uint64_t pageID, ViewGestureController::GestureID gestureID)
index 52d21bc..a77d345 100644 (file)
@@ -68,6 +68,9 @@ public:
     ViewGestureController(WebPageProxy&);
     ~ViewGestureController();
     void platformTeardown();
+
+    void disconnectFromProcess();
+    void connectToProcess();
     
     enum class ViewGestureType {
         None,
@@ -306,6 +309,7 @@ private:
     RetainPtr<_UIViewControllerOneToOneTransitionContext> m_swipeTransitionContext;
     uint64_t m_snapshotRemovalTargetRenderTreeSize { 0 };
 #endif
+    bool m_isConnectedToProcess { false };
 
     SnapshotRemovalTracker m_snapshotRemovalTracker;
 };
index 2dc6a51..79ee0aa 100644 (file)
@@ -1477,6 +1477,8 @@ void WebViewImpl::handleProcessSwapOrExit()
 void WebViewImpl::processWillSwap()
 {
     handleProcessSwapOrExit();
+    if (m_gestureController)
+        m_gestureController->disconnectFromProcess();
 }
 
 void WebViewImpl::processDidExit()
@@ -1492,6 +1494,9 @@ void WebViewImpl::pageClosed()
 
 void WebViewImpl::didRelaunchProcess()
 {
+    if (m_gestureController)
+        m_gestureController->connectToProcess();
+
     accessibilityRegisterUIProcessTokens();
 }
 
index 63c608f..425ecdc 100644 (file)
@@ -1,3 +1,15 @@
+2018-11-17  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILED: m_messageReceivers.contains(...) under ViewGestureController removeMessageReceiver
+        https://bugs.webkit.org/show_bug.cgi?id=191734
+        <rdar://problem/46151497>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-11-17  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][IFC] InlineFormattingState::addDetachingRule should accumulate rules.
index ff3fd08..915e96f 100644 (file)
@@ -27,6 +27,7 @@
 
 #import "PlatformUtilities.h"
 #import "Test.h"
+#import "TestNavigationDelegate.h"
 #import <WebKit/WKNavigationDelegatePrivate.h>
 #import <WebKit/WKNavigationPrivate.h>
 #import <WebKit/WKPreferencesPrivate.h>
@@ -2727,6 +2728,60 @@ TEST(ProcessSwap, UsePrewarmedProcessAfterTerminatingNetworkProcess)
 
 #if PLATFORM(MAC)
 
+TEST(ProcessSwap, TerminateProcessAfterProcessSwap)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    [webView setAllowsBackForwardNavigationGestures:YES];
+
+    auto navigationDelegate = adoptNS([[TestNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+    __block bool webProcessTerminated = false;
+    [navigationDelegate setWebContentProcessDidTerminate:^(WKWebView *) {
+        webProcessTerminated = true;
+    }];
+    [navigationDelegate setDidFinishNavigation:^(WKWebView *, WKNavigation *) {
+        done = true;
+    }];
+
+    // Make sure there is a gesture controller.
+    [webView _setCustomSwipeViewsTopContentInset:2.];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+    [webView loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+
+    [webView loadRequest:request];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_NE(webkitPID, [webView _webProcessIdentifier]);
+
+    webProcessTerminated = false;
+    kill([webView _webProcessIdentifier], 9);
+
+    TestWebKitAPI::Util::run(&webProcessTerminated);
+
+    TestWebKitAPI::Util::spinRunLoop(1);
+
+    [webView reload];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+}
+
 TEST(ProcessSwap, GoBackToSuspendedPageWithMainFrameIDThatIsNotOne)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);