Changeset 248598 in webkit

Timestamp:

Aug 13, 2019 10:12:43 AM (5 years ago)

Author:

Chris Dumez

Message:

Crash under IPC::Connection::markCurrentlyDispatchedMessageAsInvalid()
​https://bugs.webkit.org/show_bug.cgi?id=200674
<rdar://problem/50692748>

Reviewed by Geoff Garen.

Source/WebKit:

When the client terminates a provisional process (e.g. via the [WKWebView _killWebContentProcessAndResetState]
SPI), the WebProcessProxy would notify its associated WebPageProxy objects that it had terminated but would fail
to notify its associated ProvisionalPageProxy objects. As a result, those objects would not get destroyed and
would still think that they were in the middle of a provisional load the next time a load started. This inconsistent
state would lead to crashes such as the one in the radar.

• UIProcess/ProvisionalPageProxy.cpp:

(WebKit::ProvisionalPageProxy::cancel):

• UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::requestTermination):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/ProvisionalPageProxy.cpp (modified) (1 diff)
• Source/WebKit/UIProcess/WebProcessProxy.cpp (modified) (2 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-08-13  Chris Dumez  <cdumez@apple.com>
+
+        Crash under IPC::Connection::markCurrentlyDispatchedMessageAsInvalid()
+        https://bugs.webkit.org/show_bug.cgi?id=200674
+        <rdar://problem/50692748>
+
+        Reviewed by Geoff Garen.
+
+        When the client terminates a provisional process (e.g. via the [WKWebView _killWebContentProcessAndResetState]
+        SPI), the WebProcessProxy would notify its associated WebPageProxy objects that it had terminated but would fail
+        to notify its associated ProvisionalPageProxy objects. As a result, those objects would not get destroyed and
+        would still think that they were in the middle of a provisional load the next time a load started. This inconsistent
+        state would lead to crashes such as the one in the radar.
+
+        * UIProcess/ProvisionalPageProxy.cpp:
+        (WebKit::ProvisionalPageProxy::cancel):
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::requestTermination):
+
 2019-08-13  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp

@@ -121 +121 @@
     if (m_provisionalLoadURL.isEmpty())
         return;
+        
+    ASSERT(m_process->state() == WebProcessProxy::State::Running);
 
     RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "cancel: Simulating a didFailProvisionalLoadForFrame for pageID = %" PRIu64, m_page.pageID().toUInt64());

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -1034 +1034 @@
         webConnection()->didClose();
 
+    auto provisionalPages = WTF::map(m_provisionalPages, [](auto* provisionalPage) { return makeWeakPtr(provisionalPage); });
     auto pages = copyToVectorOf<RefPtr<WebPageProxy>>(m_pageMap.values());
 
@@ -1040 +1041 @@
     for (auto& page : pages)
         page->processDidTerminate(reason);
+        
+    for (auto& provisionalPage : provisionalPages) {
+        if (provisionalPage)
+            provisionalPage->processDidTerminate();
+    }
 }
 

trunk/Tools/ChangeLog

@@ +1 @@
+2019-08-13  Chris Dumez  <cdumez@apple.com>
+
+        Crash under IPC::Connection::markCurrentlyDispatchedMessageAsInvalid()
+        https://bugs.webkit.org/show_bug.cgi?id=200674
+        <rdar://problem/50692748>
+
+        Reviewed by Geoff Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-08-12  Takashi Komori  <Takashi.Komori@sony.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -641 +641 @@
 }
 
+TEST(ProcessSwap, KillProvisionalWebContentProcessThenStartNewLoad)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    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()]);
+    auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:navigationDelegate.get()];
+    
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+    
+    // When the provisional load starts in the provisional process, kill the WebView's processes.
+    navigationDelegate->didStartProvisionalNavigationHandler = ^{
+        [webView _killWebContentProcessAndResetState];
+        done = true;
+    };
+    
+    // Start a new cross-site load, which should happen in a new provisional process.
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+    
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+    
+    navigationDelegate->didStartProvisionalNavigationHandler = nil;
+    
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+}
+
 TEST(ProcessSwap, NoSwappingForeTLDPlus2)
 {