- Revision
- 286479
- Author
- cdu...@apple.com
- Date
- 2021-12-02 20:37:35 -0800 (Thu, 02 Dec 2021)
Log Message
Regression(r283179) Google Drive freezes after downloading a folder
https://bugs.webkit.org/show_bug.cgi?id=233783
<rdar://85918531>
Reviewed by Darin Adler.
When process-swapping on a navigation response due to COOP, we create a new ProvisionalPageProxy
to trigger a provisional load in a new WebProcess. In the common case, the ProvisionalPageProxy
gets committed, we process-swap and everything works fine. However, if the client decides to
convert the navigation into a download (like in the Google Drive case), then the
ProvisionalPageProxy gets destroyed without being committed. The issue is that the committed
process still thinks at this point that it is in the middle of a navigation and its layer
tree is thus frozen with reason=PageTransition. This is what makes Google Drive look frozen
after the download. This is not an issue with PSON because the process-swap happens in
decidePolicyForNavigationAction() and we tell the previous process to ignore the navigation
when we process-swap.
To address the issue, we now tell the committed process to cancel its navigation if the
ProvisionalPageProxy ends up getting destroyed without being committed. This lets the
committed process know there is no point in waiting for this navigation to happen and allows
it to unfreeze its layer tree.
* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
* UIProcess/ProvisionalPageProxy.h:
(WebKit::ProvisionalPageProxy::isProcessSwappingOnNavigationResponse const):
(WebKit::ProvisionalPageProxy::shouldClosePreviousPageAfterCommit const): Deleted.
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::commitProvisionalPage):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (286478 => 286479)
--- trunk/Source/WebKit/ChangeLog 2021-12-03 04:24:41 UTC (rev 286478)
+++ trunk/Source/WebKit/ChangeLog 2021-12-03 04:37:35 UTC (rev 286479)
@@ -1,3 +1,37 @@
+2021-12-02 Chris Dumez <cdu...@apple.com>
+
+ Regression(r283179) Google Drive freezes after downloading a folder
+ https://bugs.webkit.org/show_bug.cgi?id=233783
+ <rdar://85918531>
+
+ Reviewed by Darin Adler.
+
+ When process-swapping on a navigation response due to COOP, we create a new ProvisionalPageProxy
+ to trigger a provisional load in a new WebProcess. In the common case, the ProvisionalPageProxy
+ gets committed, we process-swap and everything works fine. However, if the client decides to
+ convert the navigation into a download (like in the Google Drive case), then the
+ ProvisionalPageProxy gets destroyed without being committed. The issue is that the committed
+ process still thinks at this point that it is in the middle of a navigation and its layer
+ tree is thus frozen with reason=PageTransition. This is what makes Google Drive look frozen
+ after the download. This is not an issue with PSON because the process-swap happens in
+ decidePolicyForNavigationAction() and we tell the previous process to ignore the navigation
+ when we process-swap.
+
+ To address the issue, we now tell the committed process to cancel its navigation if the
+ ProvisionalPageProxy ends up getting destroyed without being committed. This lets the
+ committed process know there is no point in waiting for this navigation to happen and allows
+ it to unfreeze its layer tree.
+
+ * UIProcess/ProvisionalPageProxy.cpp:
+ (WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
+ (WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
+ * UIProcess/ProvisionalPageProxy.h:
+ (WebKit::ProvisionalPageProxy::isProcessSwappingOnNavigationResponse const):
+ (WebKit::ProvisionalPageProxy::shouldClosePreviousPageAfterCommit const): Deleted.
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::commitProvisionalPage):
+ (WebKit::WebPageProxy::continueNavigationInNewProcess):
+
2021-12-02 Rachel Ginsberg <rginsb...@apple.com>
Make enum values consisten in ApplicationManifest::Icon::Purpose
Modified: trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp (286478 => 286479)
--- trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp 2021-12-03 04:24:41 UTC (rev 286478)
+++ trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp 2021-12-03 04:37:35 UTC (rev 286479)
@@ -58,7 +58,7 @@
#define PROVISIONALPAGEPROXY_RELEASE_LOG(channel, fmt, ...) RELEASE_LOG(channel, "%p - [pageProxyID=%" PRIu64 ", webPageID=%" PRIu64 ", PID=%i, navigationID=%" PRIu64 "] ProvisionalPageProxy::" fmt, this, m_page.identifier().toUInt64(), m_webPageID.toUInt64(), m_process->processIdentifier(), m_navigationID, ##__VA_ARGS__)
#define PROVISIONALPAGEPROXY_RELEASE_LOG_ERROR(channel, fmt, ...) RELEASE_LOG_ERROR(channel, "%p - [pageProxyID=%" PRIu64 ", webPageID=%" PRIu64 ", PID=%i, navigationID=%" PRIu64 "] ProvisionalPageProxy::" fmt, this, m_page.identifier().toUInt64(), m_webPageID.toUInt64(), m_process->processIdentifier(), m_navigationID, ##__VA_ARGS__)
-ProvisionalPageProxy::ProvisionalPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, std::unique_ptr<SuspendedPageProxy> suspendedPage, uint64_t navigationID, bool isServerRedirect, const WebCore::ResourceRequest& request, ProcessSwapRequestedByClient processSwapRequestedByClient, bool shouldClosePreviousPageAfterCommit, API::WebsitePolicies* websitePolicies)
+ProvisionalPageProxy::ProvisionalPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, std::unique_ptr<SuspendedPageProxy> suspendedPage, uint64_t navigationID, bool isServerRedirect, const WebCore::ResourceRequest& request, ProcessSwapRequestedByClient processSwapRequestedByClient, bool isProcessSwappingOnNavigationResponse, API::WebsitePolicies* websitePolicies)
: m_page(page)
, m_webPageID(suspendedPage ? suspendedPage->webPageID() : PageIdentifier::generate())
, m_process(WTFMove(process))
@@ -66,7 +66,7 @@
, m_isServerRedirect(isServerRedirect)
, m_request(request)
, m_processSwapRequestedByClient(processSwapRequestedByClient)
- , m_shouldClosePreviousPageAfterCommit(shouldClosePreviousPageAfterCommit)
+ , m_isProcessSwappingOnNavigationResponse(isProcessSwappingOnNavigationResponse)
#if PLATFORM(IOS_FAMILY)
, m_provisionalLoadActivity(m_process->throttler().foregroundActivity("Provisional Load"_s))
#endif
@@ -117,6 +117,12 @@
m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
send(Messages::WebPage::Close());
m_process->removeVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.identifier());
+
+ // If we were process-swapping on navigation response then there is still a provisional load going on in the previous process
+ // and its layer tree is frozen. Since we didn't end up committing the provisional process, we need to stop the load in the
+ // previous process so that it cancels its navigation and unfreezes its layer tree.
+ if (isProcessSwappingOnNavigationResponse())
+ m_page.send(Messages::WebPage::StopLoading());
}
m_process->removeProvisionalPageProxy(*this);
Modified: trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h (286478 => 286479)
--- trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h 2021-12-03 04:24:41 UTC (rev 286478)
+++ trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h 2021-12-03 04:37:35 UTC (rev 286479)
@@ -73,7 +73,7 @@
class ProvisionalPageProxy : public IPC::MessageReceiver, public IPC::MessageSender {
WTF_MAKE_FAST_ALLOCATED;
public:
- ProvisionalPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, std::unique_ptr<SuspendedPageProxy>, uint64_t navigationID, bool isServerRedirect, const WebCore::ResourceRequest&, ProcessSwapRequestedByClient, bool shouldClosePreviousPageAfterCommit, API::WebsitePolicies*);
+ ProvisionalPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, std::unique_ptr<SuspendedPageProxy>, uint64_t navigationID, bool isServerRedirect, const WebCore::ResourceRequest&, ProcessSwapRequestedByClient, bool isProcessSwappingOnNavigationResponse, API::WebsitePolicies*);
~ProvisionalPageProxy();
WebPageProxy& page() const { return m_page; }
@@ -84,7 +84,7 @@
uint64_t navigationID() const { return m_navigationID; }
const URL& provisionalURL() const { return m_provisionalLoadURL; }
- bool shouldClosePreviousPageAfterCommit() const { return m_shouldClosePreviousPageAfterCommit; }
+ bool isProcessSwappingOnNavigationResponse() const { return m_isProcessSwappingOnNavigationResponse; }
DrawingAreaProxy* drawingArea() const { return m_drawingArea.get(); }
std::unique_ptr<DrawingAreaProxy> takeDrawingArea();
@@ -171,7 +171,7 @@
WebCore::ResourceRequest m_request;
ProcessSwapRequestedByClient m_processSwapRequestedByClient;
bool m_wasCommitted { false };
- bool m_shouldClosePreviousPageAfterCommit { false };
+ bool m_isProcessSwappingOnNavigationResponse { false };
URL m_provisionalLoadURL;
#if PLATFORM(COCOA)
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (286478 => 286479)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2021-12-03 04:24:41 UTC (rev 286478)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2021-12-03 04:37:35 UTC (rev 286479)
@@ -3572,7 +3572,7 @@
removeAllMessageReceivers();
auto* navigation = navigationState().navigation(m_provisionalPage->navigationID());
- bool didSuspendPreviousPage = navigation && !m_provisionalPage->shouldClosePreviousPageAfterCommit() ? suspendCurrentPageIfPossible(*navigation, mainFrameIDInPreviousProcess, m_provisionalPage->processSwapRequestedByClient(), shouldDelayClosingUntilFirstLayerFlush) : false;
+ bool didSuspendPreviousPage = navigation && !m_provisionalPage->isProcessSwappingOnNavigationResponse() ? suspendCurrentPageIfPossible(*navigation, mainFrameIDInPreviousProcess, m_provisionalPage->processSwapRequestedByClient(), shouldDelayClosingUntilFirstLayerFlush) : false;
m_process->removeWebPage(*this, m_websiteDataStore.ptr() == &m_provisionalPage->process().websiteDataStore() ? WebProcessProxy::EndsUsingDataStore::No : WebProcessProxy::EndsUsingDataStore::Yes);
// There is no way we'll be able to return to the page in the previous page so close it.
@@ -3602,8 +3602,8 @@
}
bool isServerSideRedirect = shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::YesAfterNavigationPolicyDecision && navigation.currentRequestIsRedirect();
- bool shouldClosePreviousPageAfterCommit = shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::YesAfterProvisionalLoadStarted;
- m_provisionalPage = makeUnique<ProvisionalPageProxy>(*this, WTFMove(newProcess), WTFMove(suspendedPage), navigation.navigationID(), isServerSideRedirect, navigation.currentRequest(), processSwapRequestedByClient, shouldClosePreviousPageAfterCommit, websitePolicies.get());
+ bool isProcessSwappingOnNavigationResponse = shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::YesAfterProvisionalLoadStarted;
+ m_provisionalPage = makeUnique<ProvisionalPageProxy>(*this, WTFMove(newProcess), WTFMove(suspendedPage), navigation.navigationID(), isServerSideRedirect, navigation.currentRequest(), processSwapRequestedByClient, isProcessSwappingOnNavigationResponse, websitePolicies.get());
auto continuation = [this, protectedThis = Ref { *this }, navigation = Ref { navigation }, shouldTreatAsContinuingLoad, websitePolicies = WTFMove(websitePolicies), existingNetworkResourceLoadIdentifierToResume]() mutable {
if (auto* item = navigation->targetItem()) {
LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
