Attachment 357238 Details for Bug 192668 – WIP Patch

[patch] WIP Patch

192668_PSON_page_suspension_wip.patch (text/plain), 16.31 KB, created by Chris Dumez on 2018-12-13 10:54:45 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-12-13 10:54:45 PST

Size: 16.31 KB

patch

obsolete

>diff --git a/Source/WebCore/history/PageCache.cpp b/Source/WebCore/history/PageCache.cpp
>index 3cf836ced2e..f13e5a8e8b2 100644
>--- a/Source/WebCore/history/PageCache.cpp
>+++ b/Source/WebCore/history/PageCache.cpp
>@@ -425,13 +425,13 @@ static void firePageHideEventRecursively(Frame& frame)
>         firePageHideEventRecursively(*child);
> }
> 
>-void PageCache::addIfCacheable(HistoryItem& item, Page* page)
>+bool PageCache::addIfCacheable(HistoryItem& item, Page* page)
> {
>     if (item.isInPageCache())
>-        return;
>+        return false;
> 
>     if (!page || !canCache(*page))
>-        return;
>+        return false;
> 
>     ASSERT_WITH_MESSAGE(!page->isUtilityPage(), "Utility pages such as SVGImage pages should never go into PageCache");
> 
>@@ -449,7 +449,7 @@ void PageCache::addIfCacheable(HistoryItem& item, Page* page)
>     // could have altered the page in a way that could prevent caching.
>     if (!canCache(*page)) {
>         setPageCacheState(*page, Document::NotInPageCache);
>-        return;
>+        return false;
>     }
> 
>     destroyRenderTree(page->mainFrame());
>@@ -465,6 +465,7 @@ void PageCache::addIfCacheable(HistoryItem& item, Page* page)
>         m_items.add(&item);
>     }
>     prune(PruningReason::ReachedMaxSize);
>+    return true;
> }
> 
> std::unique_ptr<CachedPage> PageCache::take(HistoryItem& item, Page* page)
>diff --git a/Source/WebCore/history/PageCache.h b/Source/WebCore/history/PageCache.h
>index a62df9ac857..e211cc459fe 100644
>--- a/Source/WebCore/history/PageCache.h
>+++ b/Source/WebCore/history/PageCache.h
>@@ -51,7 +51,7 @@ public:
>     WEBCORE_EXPORT void setMaxSize(unsigned); // number of pages to cache.
>     unsigned maxSize() const { return m_maxSize; }
> 
>-    void addIfCacheable(HistoryItem&, Page*); // Prunes if maxSize() is exceeded.
>+    WEBCORE_EXPORT bool addIfCacheable(HistoryItem&, Page*); // Prunes if maxSize() is exceeded.
>     WEBCORE_EXPORT void remove(HistoryItem&);
>     CachedPage* get(HistoryItem&, Page*);
>     std::unique_ptr<CachedPage> take(HistoryItem&, Page*);
>diff --git a/Source/WebCore/loader/DocumentLoader.cpp b/Source/WebCore/loader/DocumentLoader.cpp
>index f6ffb13ff59..6d99146f9a8 100644
>--- a/Source/WebCore/loader/DocumentLoader.cpp
>+++ b/Source/WebCore/loader/DocumentLoader.cpp
>@@ -628,16 +628,12 @@ void DocumentLoader::willSendRequest(ResourceRequest&& newRequest, const Resourc
> 
>     setRequest(newRequest);
> 
>-    if (!didReceiveRedirectResponse && shouldContinue != ShouldContinue::ForSuspension)
>+    if (!didReceiveRedirectResponse)
>         return completionHandler(WTFMove(newRequest));
> 
>     auto navigationPolicyCompletionHandler = [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request, WeakPtr<FormState>&&, ShouldContinue shouldContinue) mutable {
>         m_waitingForNavigationPolicy = false;
>         switch (shouldContinue) {
>-        case ShouldContinue::ForSuspension:
>-            // We handle suspension by navigating forward to about:blank, which leaves us setup to navigate back to resume.
>-            request = { WTF::blankURL() };
>-            break;
>         case ShouldContinue::No:
>             stopLoadingForPolicyChange();
>             break;
>@@ -651,11 +647,6 @@ void DocumentLoader::willSendRequest(ResourceRequest&& newRequest, const Resourc
>     ASSERT(!m_waitingForNavigationPolicy);
>     m_waitingForNavigationPolicy = true;
> 
>-    if (shouldContinue == ShouldContinue::ForSuspension) {
>-        navigationPolicyCompletionHandler(WTFMove(newRequest), nullptr, shouldContinue);
>-        return;
>-    }
>-
>     frameLoader()->policyChecker().checkNavigationPolicy(WTFMove(newRequest), redirectResponse, WTFMove(navigationPolicyCompletionHandler));
> }
> 
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index 8336522a610..a333904b2f1 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -1772,7 +1772,9 @@ void FrameLoader::reload(OptionSet<ReloadOption> options)
> 
> void FrameLoader::stopAllLoaders(ClearProvisionalItemPolicy clearProvisionalItemPolicy)
> {
>-    ASSERT(!m_frame.document() || m_frame.document()->pageCacheState() != Document::InPageCache);
>+    if (m_frame.document() && m_frame.document()->pageCacheState() == Document::InPageCache)
>+        return;
>+
>     if (!isStopLoadingAllowed())
>         return;
> 
>@@ -1866,6 +1868,9 @@ void FrameLoader::setDocumentLoader(DocumentLoader* loader)
> {
>     if (!loader && !m_documentLoader)
>         return;
>+
>+    if (loader == m_documentLoader)
>+        return;
>     
>     ASSERT(loader != m_documentLoader);
>     ASSERT(!loader || loader->frameLoader() == this);
>@@ -1953,7 +1958,7 @@ void FrameLoader::commitProvisionalLoad()
> 
>     willTransitionToCommitted();
> 
>-    if (!m_frame.tree().parent() && history().currentItem()) {
>+    if (!m_frame.tree().parent() && history().currentItem() && history().currentItem() != history().provisionalItem()) {
>         // Check to see if we need to cache the page we are navigating away from into the back/forward cache.
>         // We are doing this here because we know for sure that a new page is about to be loaded.
>         PageCache::singleton().addIfCacheable(*history().currentItem(), m_frame.page());
>@@ -3369,15 +3374,6 @@ void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& reque
>             
>             m_loadingFromCachedPage = false;
> 
>-            // We handle suspension by navigating forward to about:blank, which leaves us setup to navigate back to resume.
>-            if (shouldContinue == ShouldContinue::ForSuspension) {
>-                // Make sure we do a standard load to about:blank instead of reusing whatever the load type was on process swap.
>-                // For example, using a Back/Forward load to load about blank would cause the current HistoryItem's url to be
>-                // overwritten with 'about:blank', which we do not want.
>-                m_loadType = FrameLoadType::Standard;
>-                m_provisionalDocumentLoader->willContinueMainResourceLoadAfterRedirect({ WTF::blankURL() });
>-            }
>-
>             m_provisionalDocumentLoader->startLoadingMainResource(shouldContinue);
>         });
>     };
>@@ -3393,7 +3389,6 @@ void FrameLoader::continueLoadAfterNavigationPolicy(const ResourceRequest& reque
> void FrameLoader::continueLoadAfterNewWindowPolicy(const ResourceRequest& request,
>     FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue, AllowNavigationToInvalidURL allowNavigationToInvalidURL, NewFrameOpenerPolicy openerPolicy)
> {
>-    ASSERT(shouldContinue != ShouldContinue::ForSuspension);
>     if (shouldContinue != ShouldContinue::Yes)
>         return;
> 
>diff --git a/Source/WebCore/loader/FrameLoaderTypes.h b/Source/WebCore/loader/FrameLoaderTypes.h
>index a5a1f7febe7..a3d0f242caf 100644
>--- a/Source/WebCore/loader/FrameLoaderTypes.h
>+++ b/Source/WebCore/loader/FrameLoaderTypes.h
>@@ -44,7 +44,7 @@ enum class PolicyAction : uint8_t {
>     Use,
>     Download,
>     Ignore,
>-    Suspend,
>+    Suspend, // FIXME: This is only used by WebKit2 so we shouldn't need this in the WebCore enum.
> };
> 
> enum class ReloadOption : uint8_t {
>diff --git a/Source/WebCore/loader/PolicyChecker.cpp b/Source/WebCore/loader/PolicyChecker.cpp
>index b9f2c53993e..3b6d10a3f8d 100644
>--- a/Source/WebCore/loader/PolicyChecker.cpp
>+++ b/Source/WebCore/loader/PolicyChecker.cpp
>@@ -183,7 +183,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, const Resou
>         case PolicyAction::Ignore:
>             return function({ }, nullptr, ShouldContinue::No);
>         case PolicyAction::Suspend:
>-            return function({ WTF::blankURL() }, nullptr, ShouldContinue::ForSuspension);
>+            RELEASE_ASSERT_NOT_REACHED();
>         case PolicyAction::Use:
>             if (!m_frame.loader().client().canHandleRequest(request)) {
>                 handleUnimplementablePolicy(m_frame.loader().client().cannotShowURLError(request));
>diff --git a/Source/WebCore/loader/PolicyChecker.h b/Source/WebCore/loader/PolicyChecker.h
>index 2071ffab174..d7fd7b3fb17 100644
>--- a/Source/WebCore/loader/PolicyChecker.h
>+++ b/Source/WebCore/loader/PolicyChecker.h
>@@ -54,8 +54,7 @@ class ResourceResponse;
> 
> enum class ShouldContinue {
>     Yes,
>-    No,
>-    ForSuspension
>+    No
> };
> 
> enum class PolicyDecisionMode { Synchronous, Asynchronous };
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>index ca5aeef40b6..7ed22380405 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>@@ -130,14 +130,12 @@ void SuspendedPageProxy::unsuspend()
>     m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
> }
> 
>-void SuspendedPageProxy::didFinishLoad()
>+void SuspendedPageProxy::didSuspend()
> {
>     LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
> 
>     m_finishedSuspending = true;
> 
>-    m_process->send(Messages::WebProcess::UpdateActivePages(), 0);
>-
> #if PLATFORM(IOS_FAMILY)
>     m_suspensionToken = nullptr;
> #endif
>@@ -159,8 +157,8 @@ void SuspendedPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder& decod
> {
>     ASSERT(decoder.messageReceiverName() == Messages::WebPageProxy::messageReceiverName());
> 
>-    if (decoder.messageName() == Messages::WebPageProxy::DidFinishLoadForFrame::name()) {
>-        didFinishLoad();
>+    if (decoder.messageName() == Messages::WebPageProxy::DidSuspendAfterProcessSwap::name()) {
>+        didSuspend();
>         return;
>     }
> 
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>index 113bd51e74b..b661f52a86c 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>@@ -55,7 +55,7 @@ public:
> #endif
> 
> private:
>-    void didFinishLoad();
>+    void didSuspend();
>     void didFailToSuspend();
> 
>     // IPC::MessageReceiver
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index fbbc875e369..584fcbac558 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -2765,6 +2765,12 @@ NO_RETURN_DUE_TO_ASSERT void WebPageProxy::didFailToSuspendAfterProcessSwap()
>     ASSERT_NOT_REACHED();
> }
> 
>+NO_RETURN_DUE_TO_ASSERT void WebPageProxy::didSuspendAfterProcessSwap()
>+{
>+    // Only the SuspendedPageProxy should be getting this call.
>+    ASSERT_NOT_REACHED();
>+}
>+
> void WebPageProxy::setUserAgent(String&& userAgent)
> {
>     if (m_userAgent == userAgent)
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index a32e831f59b..b1299b77d51 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -1567,6 +1567,7 @@ private:
>     void reattachToWebProcess();
>     void swapToWebProcess(Ref<WebProcessProxy>&&, std::unique_ptr<SuspendedPageProxy>&&, ShouldDelayAttachingDrawingArea);
>     void didFailToSuspendAfterProcessSwap();
>+    void didSuspendAfterProcessSwap();
> 
>     void finishAttachingToWebProcess(ShouldDelayAttachingDrawingArea = ShouldDelayAttachingDrawingArea::No);
> 
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.messages.in b/Source/WebKit/UIProcess/WebPageProxy.messages.in
>index f7b43adebe5..e6e94fe9a38 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.messages.in
>+++ b/Source/WebKit/UIProcess/WebPageProxy.messages.in
>@@ -505,6 +505,7 @@ messages -> WebPageProxy {
> #endif
> 
>     DidFailToSuspendAfterProcessSwap()
>+    DidSuspendAfterProcessSwap()
> 
>     ImageOrMediaDocumentSizeChanged(WebCore::IntSize newSize)
> 
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>index 3e6f31a59ec..3fdf1e4f5b0 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>@@ -735,12 +735,6 @@ void WebFrameLoaderClient::dispatchDecidePolicyForResponse(const ResourceRespons
>         return;
>     }
> 
>-    // For suspension loads to about:blank, no need to ask the SuspendedPageProxy.
>-    if (request.url() == WTF::blankURL() && webPage->isSuspended()) {
>-        function(PolicyAction::Use);
>-        return;
>-    }
>-
>     RefPtr<API::Object> userData;
> 
>     // Notify the bundle client.
>diff --git a/Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp b/Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp
>index 40722c862fe..706b378d513 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp
>@@ -50,7 +50,7 @@ void WebDocumentLoader::detachFromFrame()
> void WebDocumentLoader::setNavigationID(uint64_t navigationID)
> {
>     ASSERT(navigationID);
>-    ASSERT(!m_navigationID || m_navigationID == navigationID);
>+    //ASSERT(!m_navigationID || m_navigationID == navigationID);
> 
>     m_navigationID = navigationID;
> }
>diff --git a/Source/WebKit/WebProcess/WebPage/WebFrame.cpp b/Source/WebKit/WebProcess/WebPage/WebFrame.cpp
>index 9ae378b288c..39575351056 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebFrame.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebFrame.cpp
>@@ -275,7 +275,16 @@ void WebFrame::didReceivePolicyDecision(uint64_t listenerID, PolicyAction action
>             documentLoader->setNavigationID(navigationID);
>     }
> 
>+    bool shouldSuspend = false;
>+    if (action == PolicyAction::Suspend) {
>+        shouldSuspend = true;
>+        action = PolicyAction::Ignore;
>+    }
>+
>     function(action);
>+
>+    if (shouldSuspend)
>+        page()->suspendForProcessSwap();
> }
> 
> void WebFrame::startDownload(const WebCore::ResourceRequest& request, const String& suggestedName)
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 99a069c55c7..5c1405b6ffb 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -184,6 +184,7 @@
> #include <WebCore/MouseEvent.h>
> #include <WebCore/NotImplemented.h>
> #include <WebCore/Page.h>
>+#include <WebCore/PageCache.h>
> #include <WebCore/PageConfiguration.h>
> #include <WebCore/PingLoader.h>
> #include <WebCore/PlatformKeyboardEvent.h>
>@@ -1311,6 +1312,27 @@ void WebPage::sendClose()
>     send(Messages::WebPageProxy::ClosePage(false));
> }
> 
>+void WebPage::suspendForProcessSwap()
>+{
>+    auto failedToSuspend = [this, protectedThis = makeRef(*this)] {
>+        close();
>+        send(Messages::WebPageProxy::DidFailToSuspendAfterProcessSwap());
>+    };
>+
>+    auto* currentHistoryItem = m_mainFrame->coreFrame()->loader().history().currentItem();
>+    if (!currentHistoryItem) {
>+        failedToSuspend();
>+        return;
>+    }
>+
>+    if (!PageCache::singleton().addIfCacheable(*currentHistoryItem, corePage())) {
>+        failedToSuspend();
>+        return;
>+    }
>+
>+     send(Messages::WebPageProxy::DidSuspendAfterProcessSwap());
>+}
>+
> void WebPage::loadURLInFrame(URL&& url, uint64_t frameID)
> {
>     WebFrame* frame = WebProcess::singleton().webFrame(frameID);
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
>index d4a9b7d49cd..6eea3b0c4b0 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.h
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.h
>@@ -450,6 +450,8 @@ public:
>     void clearMainFrameName();
>     void sendClose();
> 
>+    void suspendForProcessSwap();
>+
>     void sendSetWindowFrame(const WebCore::FloatRect&);
> 
>     double textZoomFactor() const;
>diff --git a/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm b/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
>index bdae746d5d4..2f2496bf4ab 100644
>--- a/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
>+++ b/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
>@@ -460,14 +460,6 @@ static NSURL *origin(WebPage& page)
>         return nil;
> 
>     URL mainFrameURL = { URL(), mainFrame->url() };
>-    if (page.isSuspended()) {
>-        // Suspended page are navigated to about:blank upon suspension so we really want to report the previous URL.
>-        if (auto* coreFrame = mainFrame->coreFrame()) {
>-            if (auto* backHistoryItem = coreFrame->loader().history().previousItem())
>-                mainFrameURL = { URL(), backHistoryItem->urlString() };
>-        }
>-    }
>-
>     Ref<SecurityOrigin> mainFrameOrigin = SecurityOrigin::create(mainFrameURL);
>     String mainFrameOriginString;
>     if (!mainFrameOrigin->isUnique())

Actions: View | Formatted Diff | Diff

Attachments on bug 192668: 357238 | 357244 | 357251