Crash under WebProcessProxy::suspendedPageWasDestroyed(WebKit::SuspendedPageProxy&)

https://bugs.webkit.org/show_bug.cgi?id=189721
<rdar://problem/44359788>

Reviewed by Geoffrey Garen.

Source/WebKit:

Fix crash when destroying a SuspendedPageProxy whose WebProcessProxy was already
destroyed.

* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::SuspendedPageProxy):
(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
* UIProcess/SuspendedPageProxy.h:
(WebKit::SuspendedPageProxy::process const):
Update SuspendedPageProxy::m_process to be a RefPtr<> instead of a raw pointer, similarly
to what we do in WebPageProxy. Relying on the WebProcessProxy to not get destroyed is
risky as this crash demonstrates.

* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::requestTermination):
When a WebProcessProxy is terminated (by client or WebKit due to memory / cpu usage), call
webProcessDidClose() on all SuspendedPages, similarly to what we do in case of a crash in
processDidTerminateOrFailedToLaunch(). Failing to do so means that the SuspendedPageProxy
may still have a pointer to this WebProcessProxy, even though WebProcessProxy::shutDown()
has been called (which may destroy the WebProcessProxy).

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 261b3a7..fa4531b 100644 (file)
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,31 @@
+2018-09-19  Chris Dumez  <cdumez@apple.com>
+
+        Crash under WebProcessProxy::suspendedPageWasDestroyed(WebKit::SuspendedPageProxy&)
+        https://bugs.webkit.org/show_bug.cgi?id=189721
+        <rdar://problem/44359788>
+
+        Reviewed by Geoffrey Garen.
+
+        Fix crash when destroying a SuspendedPageProxy whose WebProcessProxy was already
+        destroyed.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
+        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
+        * UIProcess/SuspendedPageProxy.h:
+        (WebKit::SuspendedPageProxy::process const):
+        Update SuspendedPageProxy::m_process to be a RefPtr<> instead of a raw pointer, similarly
+        to what we do in WebPageProxy. Relying on the WebProcessProxy to not get destroyed is
+        risky as this crash demonstrates.
+
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::requestTermination):
+        When a WebProcessProxy is terminated (by client or WebKit due to memory / cpu usage), call
+        webProcessDidClose() on all SuspendedPages, similarly to what we do in case of a crash in
+        processDidTerminateOrFailedToLaunch(). Failing to do so means that the SuspendedPageProxy
+        may still have a pointer to this WebProcessProxy, even though WebProcessProxy::shutDown()
+        has been called (which may destroy the WebProcessProxy).
+
 2018-09-19  John Wilander  <wilander@apple.com>
 
         Resource Load Statistics: Add optional cap on partitioned cache max age

diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
index 5d1fc5c..533290b 100644 (file)
--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
@@ -73,9 +73,9 @@ static const HashSet<IPC::StringReference>& messageNamesToIgnoreWhileSuspended()
 }
 #endif
 
-SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, WebProcessProxy& process, WebBackForwardListItem& item)
+SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, WebBackForwardListItem& item)
     : m_page(page)
-    , m_process(&process)
+    , m_process(WTFMove(process))
     , m_origin(SecurityOriginData::fromURL({ ParsedURLString, item.url() }))
 {
     item.setSuspendedPage(*this);
@@ -85,7 +85,7 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, WebProcessProxy& proc
 
 SuspendedPageProxy::~SuspendedPageProxy()
 {
-    if (auto process = makeRefPtr(m_process)) {
+    if (auto process = m_process) {
         process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
         process->suspendedPageWasDestroyed(*this);
         process->processPool().unregisterSuspendedPageProxy(*this);

diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
index 4921109..4c1292a 100644 (file)
--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
@@ -38,13 +38,13 @@ class WebProcessProxy;
 
 class SuspendedPageProxy : public CanMakeWeakPtr<SuspendedPageProxy> {
 public:
-    SuspendedPageProxy(WebPageProxy&, WebProcessProxy&, WebBackForwardListItem&);
+    SuspendedPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, WebBackForwardListItem&);
     ~SuspendedPageProxy();
 
     void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
 
     WebPageProxy& page() const { return m_page; }
-    WebProcessProxy* process() const { return m_process; }
+    WebProcessProxy* process() const { return m_process.get(); }
     const WebCore::SecurityOriginData& origin() const { return m_origin; }
 
     void webProcessDidClose(WebProcessProxy&);
@@ -58,7 +58,7 @@ private:
     void didFinishLoad();
 
     WebPageProxy& m_page;
-    WebProcessProxy* m_process;
+    RefPtr<WebProcessProxy> m_process;
     WebCore::SecurityOriginData m_origin;
 
 #if !LOG_DISABLED

diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
index e4f47d9..2714215 100644 (file)
--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
@@ -1008,6 +1008,11 @@ void WebProcessProxy::requestTermination(ProcessTerminationReason reason)
 
     shutDown();
 
+    for (auto* suspendedPage : copyToVectorOf<SuspendedPageProxy*>(m_suspendedPageMap.values()))
+        suspendedPage->webProcessDidClose(*this);
+
+    m_suspendedPageMap.clear();
+
     for (auto& page : pages)
         page->processDidTerminate(reason);
 }

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index 96dd810..3daefaa 100644 (file)
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,15 @@
+2018-09-19  Chris Dumez  <cdumez@apple.com>
+
+        Crash under WebProcessProxy::suspendedPageWasDestroyed(WebKit::SuspendedPageProxy&)
+        https://bugs.webkit.org/show_bug.cgi?id=189721
+        <rdar://problem/44359788>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-09-19  Thomas Denney  <tdenney@apple.com>
 
         [WHLSL] Improve test suite type safety

diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
index 71b21eb..8fe073b 100644 (file)
--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
@@ -1803,4 +1803,69 @@ TEST(ProcessSwap, APIControlledProcessSwapping)
     EXPECT_NE(pid1, pid3);
 }
 
+TEST(ProcessSwap, TerminatedSuspendedPageProcess)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    processPoolConfiguration.get().processSwapsOnNavigation = YES;
+    processPoolConfiguration.get().prewarmsProcessesAutomatically = 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()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid1 = [webView _webProcessIdentifier];
+
+    @autoreleasepool {
+        auto webViewConfiguration2 = adoptNS([[WKWebViewConfiguration alloc] init]);
+        [webViewConfiguration2 setProcessPool:processPool.get()];
+        [webViewConfiguration2 _setRelatedWebView:webView.get()]; // Make sure it uses the same process.
+        auto webView2 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration2.get()]);
+        [webView2 setNavigationDelegate:delegate.get()];
+
+        request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"about:blank"]];
+        [webView2 loadRequest:request];
+
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+
+        auto pid2 = [webView2 _webProcessIdentifier];
+        EXPECT_EQ(pid1, pid2);
+
+        request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.google.com/main2.html"]];
+        [webView loadRequest:request];
+
+        TestWebKitAPI::Util::run(&done);
+        done = false;
+
+        [webView2 _killWebContentProcessAndResetState];
+        webView2 = nullptr;
+        webViewConfiguration2 = nullptr;
+    }
+
+    auto pid3 = [webView _webProcessIdentifier];
+    EXPECT_NE(pid1, pid3);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pid4 = [webView _webProcessIdentifier];
+    EXPECT_NE(pid1, pid4);
+    EXPECT_NE(pid3, pid4);
+}
+
 #endif // WK_API_ENABLED