Attachment 349468 Details for Bug 189517 – Patch

[patch] Patch

bug-189517-20180911150313.patch (text/plain), 10.30 KB, created by Chris Dumez on 2018-09-11 15:03:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-11 15:03:15 PDT

Size: 10.30 KB

patch

obsolete

>Subversion Revision: 235914
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index e4b2e5a0298319731cb4687d3183e017cfcee0b9..91b64963b12281d90e1aae395e7404e8aa4b1bf4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,35 @@
>+2018-09-11  Chris Dumez  <cdumez@apple.com>
>+
>+        Clean up SuspendedPageProxy
>+        https://bugs.webkit.org/show_bug.cgi?id=189517
>+
>+        Reviewed by Alex Christensen.
>+
>+        Clean up SuspendedPageProxy:
>+        1. SuspendedPageProxy does not need to be RefCounted. It is even dangerous given that WebPageProxy
>+           owns the SuspendedPageProxy and SuspendedPageProxy has a WebPageProxy& data member. We definitely
>+           do not want it to outlive its WebPageProxy.
>+        2. The SuspendedPageProxy destructor does not need to be virtual.
>+        3. Have WebBackForwardListItem keep a WeakPtr<SuspendedPageProxy> instead of a SuspendedPageProxy*.
>+           This is safer and avoid having to explicitly clear the pointer.
>+        4. m_finishedSuspending data member does not need a getter and is only needed if !LOG_DISABLED.
>+
>+        * Shared/WebBackForwardListItem.cpp:
>+        (WebKit::WebBackForwardListItem::setSuspendedPage):
>+        * Shared/WebBackForwardListItem.h:
>+        (WebKit::WebBackForwardListItem::suspendedPage const):
>+        * UIProcess/SuspendedPageProxy.cpp:
>+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
>+        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
>+        (WebKit::SuspendedPageProxy::webProcessDidClose):
>+        (WebKit::SuspendedPageProxy::didFinishLoad):
>+        * UIProcess/SuspendedPageProxy.h:
>+        (WebKit::SuspendedPageProxy::process const):
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::maybeCreateSuspendedPage):
>+        (WebKit::WebPageProxy::reattachToWebProcess):
>+        * UIProcess/WebPageProxy.h:
>+
> 2018-09-11  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [macOS] [WK2] Support changing foreground colors via color panel
>diff --git a/Source/WebKit/Shared/WebBackForwardListItem.cpp b/Source/WebKit/Shared/WebBackForwardListItem.cpp
>index 32e7c780f67418328ae8b615d70598c96f7c62d7..3be1cb08b5ebe992e335c01a8a10240408cf82ac 100644
>--- a/Source/WebKit/Shared/WebBackForwardListItem.cpp
>+++ b/Source/WebKit/Shared/WebBackForwardListItem.cpp
>@@ -26,6 +26,7 @@
> #include "config.h"
> #include "WebBackForwardListItem.h"
> 
>+#include "SuspendedPageProxy.h"
> #include <WebCore/URL.h>
> #include <wtf/DebugUtilities.h>
> 
>@@ -112,10 +113,10 @@ bool WebBackForwardListItem::itemIsInSameDocument(const WebBackForwardListItem&
>     return documentTreesAreEqual(mainFrameState, otherMainFrameState);
> }
> 
>-void WebBackForwardListItem::setSuspendedPage(SuspendedPageProxy* page)
>+void WebBackForwardListItem::setSuspendedPage(SuspendedPageProxy& page)
> {
>-    ASSERT(!m_suspendedPage || page == nullptr);
>-    m_suspendedPage = page;
>+    ASSERT(!m_suspendedPage);
>+    m_suspendedPage = makeWeakPtr(page);
> }
> 
> #if !LOG_DISABLED
>diff --git a/Source/WebKit/Shared/WebBackForwardListItem.h b/Source/WebKit/Shared/WebBackForwardListItem.h
>index ba746bee953bb24e92a076315c94625f2af5f107..4376648396c4efeb8b642380bd403312891f3844 100644
>--- a/Source/WebKit/Shared/WebBackForwardListItem.h
>+++ b/Source/WebKit/Shared/WebBackForwardListItem.h
>@@ -28,6 +28,7 @@
> #include "APIObject.h"
> #include "SessionState.h"
> #include <wtf/Ref.h>
>+#include <wtf/WeakPtr.h>
> #include <wtf/text/WTFString.h>
> 
> namespace API {
>@@ -68,8 +69,8 @@ public:
>     ViewSnapshot* snapshot() const { return m_itemState.snapshot.get(); }
>     void setSnapshot(RefPtr<ViewSnapshot>&& snapshot) { m_itemState.snapshot = WTFMove(snapshot); }
> #endif
>-    void setSuspendedPage(SuspendedPageProxy*);
>-    SuspendedPageProxy* suspendedPage() const { return m_suspendedPage; }
>+    void setSuspendedPage(SuspendedPageProxy&);
>+    SuspendedPageProxy* suspendedPage() const { return m_suspendedPage.get(); }
> 
> #if !LOG_DISABLED
>     const char* loggingString();
>@@ -80,7 +81,7 @@ private:
> 
>     BackForwardListItemState m_itemState;
>     uint64_t m_pageID;
>-    SuspendedPageProxy* m_suspendedPage { nullptr };
>+    WeakPtr<SuspendedPageProxy> m_suspendedPage;
> };
> 
> typedef Vector<Ref<WebBackForwardListItem>> BackForwardListItemVector;
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>index 807189258f6d084321c523f146fbf1d3c5a5a083..5d1fc5c31c9409f2e23dffdae5239421678e09b4 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>@@ -76,10 +76,9 @@ static const HashSet<IPC::StringReference>& messageNamesToIgnoreWhileSuspended()
> SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, WebProcessProxy& process, WebBackForwardListItem& item)
>     : m_page(page)
>     , m_process(&process)
>-    , m_backForwardListItem(item)
>     , m_origin(SecurityOriginData::fromURL({ ParsedURLString, item.url() }))
> {
>-    m_backForwardListItem->setSuspendedPage(this);
>+    item.setSuspendedPage(*this);
>     m_process->processPool().registerSuspendedPageProxy(*this);
>     m_process->send(Messages::WebPage::SetIsSuspended(true), m_page.pageID());
> }
>@@ -91,20 +90,17 @@ SuspendedPageProxy::~SuspendedPageProxy()
>         process->suspendedPageWasDestroyed(*this);
>         process->processPool().unregisterSuspendedPageProxy(*this);
>     }
>-
>-    m_backForwardListItem->setSuspendedPage(nullptr);
> }
> 
> void SuspendedPageProxy::webProcessDidClose(WebProcessProxy& process)
> {
>     ASSERT_UNUSED(process, &process == m_process);
> 
>-    auto protectedThis = makeRef(*this);
>     m_process->processPool().unregisterSuspendedPageProxy(*this);
>     m_process = nullptr;
> 
>+    // This will destroy |this|.
>     m_page.suspendedPageClosed(*this);
>-    m_backForwardListItem->setSuspendedPage(nullptr);
> }
> 
> void SuspendedPageProxy::destroyWebPageInWebProcess()
>@@ -118,7 +114,9 @@ void SuspendedPageProxy::didFinishLoad()
>     ASSERT(m_process);
>     LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
> 
>+#if !LOG_DISABLED
>     m_finishedSuspending = true;
>+#endif
> 
>     m_process->send(Messages::WebProcess::UpdateActivePages(), 0);
> }
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>index 5c84b2282406739b4bebb3c572faebccfb266484..4921109d3e4e876703249545e4c4a0ded9b5ab47 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>@@ -29,30 +29,24 @@
> #include "WebBackForwardListItem.h"
> #include <WebCore/SecurityOriginData.h>
> #include <wtf/RefCounted.h>
>+#include <wtf/WeakPtr.h>
> 
> namespace WebKit {
> 
> class WebPageProxy;
> class WebProcessProxy;
> 
>-class SuspendedPageProxy : public RefCounted<SuspendedPageProxy> {
>+class SuspendedPageProxy : public CanMakeWeakPtr<SuspendedPageProxy> {
> public:
>-    static Ref<SuspendedPageProxy> create(WebPageProxy& page, WebProcessProxy& process, WebBackForwardListItem& item)
>-    {
>-        return adoptRef(*new SuspendedPageProxy(page, process, item));
>-    }
>-
>-    virtual ~SuspendedPageProxy();
>+    SuspendedPageProxy(WebPageProxy&, WebProcessProxy&, WebBackForwardListItem&);
>+    ~SuspendedPageProxy();
> 
>     void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
> 
>     WebPageProxy& page() const { return m_page; }
>     WebProcessProxy* process() const { return m_process; }
>-    WebBackForwardListItem& item() const { return m_backForwardListItem; }
>     const WebCore::SecurityOriginData& origin() const { return m_origin; }
> 
>-    bool finishedSuspending() const { return m_finishedSuspending; }
>-
>     void webProcessDidClose(WebProcessProxy&);
>     void destroyWebPageInWebProcess();
> 
>@@ -61,16 +55,15 @@ public:
> #endif
> 
> private:
>-    SuspendedPageProxy(WebPageProxy&, WebProcessProxy&, WebBackForwardListItem&);
>-
>     void didFinishLoad();
> 
>     WebPageProxy& m_page;
>     WebProcessProxy* m_process;
>-    Ref<WebBackForwardListItem> m_backForwardListItem;
>     WebCore::SecurityOriginData m_origin;
> 
>+#if !LOG_DISABLED
>     bool m_finishedSuspending { false };
>+#endif
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 403b01293a4cb01ee35cc430630ee4e722f98dba..79dcc5980039dca239869d77f11287b7ae1a1ad5 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -718,7 +718,7 @@ SuspendedPageProxy* WebPageProxy::maybeCreateSuspendedPage(WebProcessProxy& proc
>         return nullptr;
>     }
> 
>-    m_suspendedPage = SuspendedPageProxy::create(*this, process, *currentItem);
>+    m_suspendedPage = std::make_unique<SuspendedPageProxy>(*this, process, *currentItem);
> 
>     LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), m_suspendedPage->loggingString(), process.processIdentifier(), currentItem->itemID().logString());
> 
>@@ -740,12 +740,12 @@ void WebPageProxy::reattachToWebProcess(Ref<WebProcessProxy>&& process, API::Nav
> 
>     // If the process we're attaching to is kept alive solely by our current suspended page,
>     // we need to maintain that by temporarily keeping the suspended page alive.
>-    RefPtr<SuspendedPageProxy> currentSuspendedPage;
>+    std::unique_ptr<SuspendedPageProxy> currentSuspendedPage;
>     if (!navigation) {
>         m_process->removeWebPage(*this, m_pageID);
>         m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
>     } else {
>-        currentSuspendedPage = m_suspendedPage;
>+        currentSuspendedPage = WTFMove(m_suspendedPage);
>         m_process->suspendWebPageProxy(*this, *navigation);
>     }
> 
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index b7845bfcebd2fed9f32287c157a40d0efeecd858..3d3372086281f0749cff235bf36a458f862dc5c3 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -2234,7 +2234,7 @@ private:
>     // FIXME: Support more than one suspended page per WebPageProxy,
>     // and have a global collection of them per process pool
>     // (e.g. for that process pool's page cache)
>-    RefPtr<SuspendedPageProxy> m_suspendedPage;
>+    std::unique_ptr<SuspendedPageProxy> m_suspendedPage;
> 
>     RunLoop::Timer<WebPageProxy> m_resetRecentCrashCountTimer;
>     unsigned m_recentCrashCount { 0 };

Actions: View | Formatted Diff | Diff

Attachments on bug 189517: 349424 | 349468