Attachment 350070 Details for Bug 189721 – Patch

[patch] Patch

bug-189721-20180918155132.patch (text/plain), 4.97 KB, created by Chris Dumez on 2018-09-18 15:51:32 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-18 15:51:32 PDT

Size: 4.97 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);
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 189721: 350070 | 350072 | 350202