Attachment 350072 Details for Bug 189721 – Patch

[patch] Patch

bug-189721-20180918161631.patch (text/plain), 8.84 KB, created by Chris Dumez on 2018-09-18 16:16:32 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-18 16:16:32 PDT

Size: 8.84 KB

patch

obsolete

>Subversion Revision: 236154
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index efe33716a766bb1f017fb09dc61740fbf6196d4f..f8c7eaaea9d1968e97e9953853734f11323ccc30 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,31 @@
>+2018-09-18  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 NOBODY (OOPS!).
>+
>+        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-18  Alex Christensen  <achristensen@webkit.org>
> 
>         Make WebPageProxy always have a API::NavigationClient instead of always having a API::LoaderClient and API::PolicyClient
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>index 5d1fc5c31c9409f2e23dffdae5239421678e09b4..533290bf1e116426f83318a5b0c5a77b27a5118e 100644
>--- 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 4921109d3e4e876703249545e4c4a0ded9b5ab47..4c1292a1b0f52fed025ab67608d70f1c5bd24f7f 100644
>--- 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 e4f47d930a7bed4c1b621dff9db3198d80cf4ce8..2714215b676418cb55f8c70661ef448bf6593d32 100644
>--- 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 2767dbf827b6eb6488b3239c2403372b2b8f6b4a..595c75e77dd23b274126a3fcb789be7190d90eb5 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-09-18  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 NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2018-09-18  Jonathan Bedard  <jbedard@apple.com>
> 
>         webkitpy: Clobbering and building occurs multiple times for iOS Simulator ports
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 71b21eb492ed649e93dcbf38f2ba111d77b5e002..8fe073b83ca6e77e9eb6e161a4377083705efbf0 100644
>--- 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

Actions: View | Formatted Diff | Diff

Attachments on bug 189721: 350070 | 350072 | 350202