Attachment 349589 Details for Bug 189562 – Patch

[patch] Patch

bug-189562-20180912153016.patch (text/plain), 10.20 KB, created by Chris Dumez on 2018-09-12 15:30:17 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-12 15:30:17 PDT

Size: 10.20 KB

patch

obsolete

>Subversion Revision: 235953
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 52386e188b32c7995a7a811233d5d48e4560ed44..2b339a9705800eb45da4830af7fceb83d506db2f 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-09-12  Chris Dumez  <cdumez@apple.com>
>+
>+        Add release logging to help debug PSON issues
>+        https://bugs.webkit.org/show_bug.cgi?id=189562
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add release logging to help debug issues related to process swap on navigation.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::processForNavigation):
>+        (WebKit::WebProcessPool::processForNavigationInternal):
>+        * UIProcess/WebProcessPool.h:
>+
> 2018-09-11  Simon Fraser  <simon.fraser@apple.com>
> 
>         Make GraphicsLayers ref-counted, so their tree can persist when disconnected from RenderLayerBackings
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 79dcc5980039dca239869d77f11287b7ae1a1ad5..7eca8fdfb6776c3055352c820061b7e8c1d9a068 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -4040,15 +4040,19 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const WebCo
>         }
> 
>         if (policyAction == PolicyAction::Use && frame->isMainFrame()) {
>-            auto proposedProcess = process().processPool().processForNavigation(*this, *navigation, processSwapRequestedByClient, policyAction);
>+            String reason;
>+            auto proposedProcess = process().processPool().processForNavigation(*this, *navigation, processSwapRequestedByClient, policyAction, reason);
>+            ASSERT(!reason.isNull());
>             
>             if (proposedProcess.ptr() != &process()) {
>+                RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, swapping process %i with process %i for navigation, reason: %{public}s", this, processIdentifier(), proposedProcess->processIdentifier(), reason.utf8().data());
>                 LOG(ProcessSwapping, "(ProcessSwapping) Switching from process %i to new process (%i) for navigation %" PRIu64 " '%s'", processIdentifier(), proposedProcess->processIdentifier(), navigation->navigationID(), navigation->loggingString());
>                 
>                 RunLoop::main().dispatch([this, protectedThis = WTFMove(protectedThis), navigation = makeRef(*navigation), proposedProcess = WTFMove(proposedProcess)]() mutable {
>                     continueNavigationInNewProcess(navigation.get(), WTFMove(proposedProcess));
>                 });
>-            }
>+            } else
>+                RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "%p - WebPageProxy::decidePolicyForNavigationAction, keep using process %i for navigation, reason: %{public}s", this, processIdentifier(), reason.utf8().data());
>         }
> 
>         receivedPolicyDecision(policyAction, navigation.get(), WTFMove(data), WTFMove(sender));
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 8f645dd4d9a3efad7d055d495298d0c8714486b8..8f86f3cb53b51963c13c16e46a736d11d24ad51f 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -2102,9 +2102,9 @@ void WebProcessPool::removeProcessFromOriginCacheSet(WebProcessProxy& process)
>         m_swappedProcessesPerRegistrableDomain.remove(registrableDomain);
> }
> 
>-Ref<WebProcessProxy> WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, PolicyAction& action)
>+Ref<WebProcessProxy> WebProcessPool::processForNavigation(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, PolicyAction& action, String& reason)
> {
>-    auto process = processForNavigationInternal(page, navigation, processSwapRequestedByClient, action);
>+    auto process = processForNavigationInternal(page, navigation, processSwapRequestedByClient, action, reason);
> 
>     if (m_configuration->alwaysKeepAndReuseSwappedProcesses() && process.ptr() != &page.process()) {
>         static std::once_flag onceFlag;
>@@ -2120,60 +2120,84 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigation(WebPageProxy& page, co
>     return process;
> }
> 
>-Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, PolicyAction& action)
>+Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API::Navigation& navigation, ProcessSwapRequestedByClient processSwapRequestedByClient, PolicyAction& action, String& reason)
> {
>-    if (!m_configuration->processSwapsOnNavigation() && processSwapRequestedByClient == ProcessSwapRequestedByClient::No)
>+    if (!m_configuration->processSwapsOnNavigation() && processSwapRequestedByClient == ProcessSwapRequestedByClient::No) {
>+        reason = "Feature is disabled"_s;
>         return page.process();
>+    }
> 
>-    if (page.inspectorFrontendCount() > 0)
>+    if (page.inspectorFrontendCount() > 0) {
>+        reason = "A Web Inspector frontend is connected"_s;
>         return page.process();
>+    }
> 
>-    if (m_automationSession)
>+    if (m_automationSession) {
>+        reason = "An automation session is active"_s;
>         return page.process();
>+    }
> 
>-    if (!page.process().hasCommittedAnyProvisionalLoads())
>+    if (!page.process().hasCommittedAnyProvisionalLoads()) {
>+        reason = "Process has not yet committed any provisional loads"_s;
>         return page.process();
>+    }
> 
>     if (navigation.isCrossOriginWindowOpenNavigation()) {
>-        if (navigation.opener() && !m_configuration->processSwapsOnWindowOpenWithOpener())
>+        if (navigation.opener() && !m_configuration->processSwapsOnWindowOpenWithOpener()) {
>+            reason = "Browsing context has an opener"_s;
>             return page.process();
>+        }
> 
>+        reason = "Initial navigation is cross-site in a newly opened window"_s;
>         action = PolicyAction::Ignore;
>         return createNewWebProcess(page.websiteDataStore());
>     }
> 
>     // FIXME: We should support process swap when a window has an opener.
>-    if (navigation.opener())
>+    if (navigation.opener()) {
>+        reason = "Browsing context has an opener"_s;
>         return page.process();
>+    }
> 
>     // FIXME: We should support process swap when a window has opened other windows via window.open.
>-    if (navigation.hasOpenedFrames())
>+    if (navigation.hasOpenedFrames()) {
>+        reason = "Browsing context has opened other windows"_s;
>         return page.process();
>+    }
> 
>     if (auto* backForwardListItem = navigation.targetItem()) {
>         if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
>             ASSERT(suspendedPage->process());
>             action = PolicyAction::Suspend;
>+            reason = "Using target back/forward item's process"_s;
>             return *suspendedPage->process();
>         }
> 
>         // If the target back/forward item and the current back/forward item originated
>         // in the same WebProcess then we should reuse the current WebProcess.
>         if (auto* fromItem = navigation.fromItem()) {
>-            if (fromItem->itemID().processIdentifier == backForwardListItem->itemID().processIdentifier)
>+            if (fromItem->itemID().processIdentifier == backForwardListItem->itemID().processIdentifier) {
>+                reason = "Source and target back/forward item originated in the same process"_s;
>                 return page.process();
>+            }
>         }
>     }
> 
>     auto targetURL = navigation.currentRequest().url();
>     if (processSwapRequestedByClient == ProcessSwapRequestedByClient::No) {
>-        if (navigation.treatAsSameOriginNavigation())
>+        if (navigation.treatAsSameOriginNavigation()) {
>+            reason = "The treatAsSameOriginNavigation flag is set"_s;
>             return page.process();
>+        }
> 
>         auto url = URL { ParsedURLString, page.pageLoadState().url() };
>-        if (!url.isValid() || !targetURL.isValid() || url.isEmpty() || url.isBlankURL() || registrableDomainsAreEqual(url, targetURL))
>+        if (!url.isValid() || !targetURL.isValid() || url.isEmpty() || url.isBlankURL() || registrableDomainsAreEqual(url, targetURL)) {
>+            reason = "Navigation is same-site"_s;
>             return page.process();
>-    }
>+        }
>+        reason = "Navigation is cross-site"_s;
>+    } else
>+        reason = "Process swap was requested by the client"_s;
>     
>     if (m_configuration->alwaysKeepAndReuseSwappedProcesses()) {
>         auto registrableDomain = toRegistrableDomain(targetURL);
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.h b/Source/WebKit/UIProcess/WebProcessPool.h
>index a93998ea1fa088884c0a341dde4b591733293cf5..39351848f5f86a4ae366dd9627356a69de201f95 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.h
>+++ b/Source/WebKit/UIProcess/WebProcessPool.h
>@@ -447,7 +447,7 @@ public:
>     BackgroundWebProcessToken backgroundWebProcessToken() const { return BackgroundWebProcessToken(m_backgroundWebProcessCounter.count()); }
> #endif
> 
>-    Ref<WebProcessProxy> processForNavigation(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&);
>+    Ref<WebProcessProxy> processForNavigation(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&, String& reason);
>     void registerSuspendedPageProxy(SuspendedPageProxy&);
>     void unregisterSuspendedPageProxy(SuspendedPageProxy&);
>     void didReachGoodTimeToPrewarm();
>@@ -467,7 +467,7 @@ private:
>     void platformInitializeWebProcess(WebProcessCreationParameters&);
>     void platformInvalidateContext();
> 
>-    Ref<WebProcessProxy> processForNavigationInternal(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&);
>+    Ref<WebProcessProxy> processForNavigationInternal(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&, String& reason);
> 
>     RefPtr<WebProcessProxy> tryTakePrewarmedProcess(WebsiteDataStore&);
>

Actions: View | Formatted Diff | Diff

Attachments on bug 189562: 349589 | 349607 | 349636 | 349671