Title: [238353] trunk/Source/WebKit
Revision
238353
Author
cdu...@apple.com
Date
2018-11-17 14:06:56 -0800 (Sat, 17 Nov 2018)

Log Message

[PSON] ASSERTION FAILED: m_uncommittedState.state == State::Committed
https://bugs.webkit.org/show_bug.cgi?id=191781

Reviewed by Ryosuke Niwa.

The crash was happening when switching to a suspended page that is not yet done
suspending (e.g. in case of very fast back/forward navigation). The WebPageProxy
would reattach to the suspended process and get load notifications that it did
not expect since it did not schedule any load yet. Those notifications are for
the about:blank load we do for page suspension.

To address the issue, make swapToWebProcess() asynchronous and take a completion
handler. When we try to unsuspend a SuspendedPageProxy, we first make sure it
is actually done suspending. If it is not done suspending, we wait until it is
before telling in to unsuspend and proceeding with the new load.

* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::unsuspend):
(WebKit::SuspendedPageProxy::didFinishLoad):
* UIProcess/SuspendedPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::swapToWebProcess):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
* UIProcess/WebPageProxy.h:

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (238352 => 238353)


--- trunk/Source/WebKit/ChangeLog	2018-11-17 22:01:15 UTC (rev 238352)
+++ trunk/Source/WebKit/ChangeLog	2018-11-17 22:06:56 UTC (rev 238353)
@@ -1,5 +1,32 @@
 2018-11-17  Chris Dumez  <cdu...@apple.com>
 
+        [PSON] ASSERTION FAILED: m_uncommittedState.state == State::Committed
+        https://bugs.webkit.org/show_bug.cgi?id=191781
+
+        Reviewed by Ryosuke Niwa.
+
+        The crash was happening when switching to a suspended page that is not yet done
+        suspending (e.g. in case of very fast back/forward navigation). The WebPageProxy
+        would reattach to the suspended process and get load notifications that it did
+        not expect since it did not schedule any load yet. Those notifications are for
+        the about:blank load we do for page suspension.
+
+        To address the issue, make swapToWebProcess() asynchronous and take a completion
+        handler. When we try to unsuspend a SuspendedPageProxy, we first make sure it
+        is actually done suspending. If it is not done suspending, we wait until it is
+        before telling in to unsuspend and proceeding with the new load.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::unsuspend):
+        (WebKit::SuspendedPageProxy::didFinishLoad):
+        * UIProcess/SuspendedPageProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::swapToWebProcess):
+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
+        * UIProcess/WebPageProxy.h:
+
+2018-11-17  Chris Dumez  <cdu...@apple.com>
+
         Unreviewed follow-up to r238343 to address debug assertions in 2 API tests.
 
         * UIProcess/Network/NetworkProcessProxy.cpp:

Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp (238352 => 238353)


--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp	2018-11-17 22:01:15 UTC (rev 238352)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp	2018-11-17 22:06:56 UTC (rev 238353)
@@ -103,13 +103,22 @@
     });
 }
 
-void SuspendedPageProxy::unsuspend()
+void SuspendedPageProxy::unsuspend(CompletionHandler<void()>&& completionHandler)
 {
     ASSERT(m_isSuspended);
 
-    m_isSuspended = false;
-    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
-    m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
+    auto doUnsuspend = [this, completionHandler = WTFMove(completionHandler)]() mutable {
+        m_isSuspended = false;
+        m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
+        m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
+        completionHandler();
+    };
+
+    if (!m_finishedSuspending) {
+        ASSERT(!m_finishedSuspendingHandler);
+        m_finishedSuspendingHandler = WTFMove(doUnsuspend);
+    } else
+        doUnsuspend();
 }
 
 void SuspendedPageProxy::didFinishLoad()
@@ -116,11 +125,12 @@
 {
     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);
+
+    if (auto finishedSuspendingHandler = WTFMove(m_finishedSuspendingHandler))
+        finishedSuspendingHandler();
 }
 
 void SuspendedPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder& decoder)

Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h (238352 => 238353)


--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h	2018-11-17 22:01:15 UTC (rev 238352)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h	2018-11-17 22:06:56 UTC (rev 238353)
@@ -46,7 +46,7 @@
     uint64_t mainFrameID() const { return m_mainFrameID; }
     const String& registrableDomain() const { return m_registrableDomain; }
 
-    void unsuspend();
+    void unsuspend(CompletionHandler<void()>&&);
 
 #if !LOG_DISABLED
     const char* loggingString() const;
@@ -66,9 +66,8 @@
 
     bool m_isSuspended { true };
 
-#if !LOG_DISABLED
     bool m_finishedSuspending { false };
-#endif
+    CompletionHandler<void()> m_finishedSuspendingHandler;
 };
 
 } // namespace WebKit

Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (238352 => 238353)


--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp	2018-11-17 22:01:15 UTC (rev 238352)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp	2018-11-17 22:06:56 UTC (rev 238353)
@@ -750,7 +750,7 @@
     return true;
 }
 
-void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess)
+void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess, CompletionHandler<void()>&& completionHandler)
 {
     ASSERT(!m_isClosed);
     RELEASE_LOG_IF_ALLOWED(Process, "%p WebPageProxy::swapToWebProcess\n", this);
@@ -760,29 +760,39 @@
         if (backForwardListItem->suspendedPage() && &backForwardListItem->suspendedPage()->process() == process.ptr()) {
             destinationSuspendedPage = this->process().processPool().takeSuspendedPage(*backForwardListItem->suspendedPage());
             ASSERT(destinationSuspendedPage);
-            destinationSuspendedPage->unsuspend();
         }
     }
 
-    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
-    bool didSuspendPreviousPage = suspendCurrentPageIfPossible(navigation, mainFrameIDInPreviousProcess);
-    m_process->removeWebPage(*this, m_pageID);
+    auto* destinationSuspendedPagePtr = destinationSuspendedPage.get();
+    auto doSwap = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), process = WTFMove(process), mainFrameIDInPreviousProcess, destinationSuspendedPage = WTFMove(destinationSuspendedPage), completionHandler = WTFMove(completionHandler)]() mutable {
+        processDidTerminate(ProcessTerminationReason::NavigationSwap);
 
-    m_process = WTFMove(process);
-    m_isValid = true;
+        m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
+        bool didSuspendPreviousPage = suspendCurrentPageIfPossible(navigation, mainFrameIDInPreviousProcess);
+        m_process->removeWebPage(*this, m_pageID);
 
-    // If we are reattaching to a SuspendedPage, then the WebProcess' WebPage already exists and
-    // WebPageProxy::didCreateMainFrame() will not be called to initialize m_mainFrame. In such
-    // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
-    // already exists and already has a main frame.
-    if (destinationSuspendedPage) {
-        ASSERT(!m_mainFrame);
-        ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
-        m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
-        m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
-    }
+        m_process = WTFMove(process);
+        m_isValid = true;
 
-    finishAttachingToWebProcess(didSuspendPreviousPage ? ShouldDelayAttachingDrawingArea::Yes : ShouldDelayAttachingDrawingArea::No);
+        // If we are reattaching to a SuspendedPage, then the WebProcess' WebPage already exists and
+        // WebPageProxy::didCreateMainFrame() will not be called to initialize m_mainFrame. In such
+        // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
+        // already exists and already has a main frame.
+        if (destinationSuspendedPage) {
+            ASSERT(!m_mainFrame);
+            ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
+            m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
+            m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
+        }
+
+        finishAttachingToWebProcess(didSuspendPreviousPage ? ShouldDelayAttachingDrawingArea::Yes : ShouldDelayAttachingDrawingArea::No);
+        completionHandler();
+    };
+
+    if (destinationSuspendedPagePtr)
+        destinationSuspendedPagePtr->unsuspend(WTFMove(doSwap));
+    else
+        doSwap();
 }
 
 void WebPageProxy::finishAttachingToWebProcess(ShouldDelayAttachingDrawingArea shouldDelayAttachingDrawingArea)
@@ -2648,71 +2658,69 @@
 
     ASSERT(m_process.ptr() != process.ptr());
 
-    processDidTerminate(ProcessTerminationReason::NavigationSwap);
+    swapToWebProcess(WTFMove(process), navigation, mainFrameIDInPreviousProcess, [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess, mainFrameURL = WTFMove(mainFrameURL)]() mutable {
+        if (auto* item = navigation->targetItem()) {
+            LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
 
-    swapToWebProcess(WTFMove(process), navigation, mainFrameIDInPreviousProcess);
+            auto transaction = m_pageLoadState.transaction();
+            m_pageLoadState.setPendingAPIRequestURL(transaction, item->url());
 
-    if (auto* item = navigation.targetItem()) {
-        LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
+            auto itemStates = m_backForwardList->filteredItemStates([this, targetItem = item](WebBackForwardListItem& item) {
+                if (auto* page = item.suspendedPage()) {
+                    if (&page->process() == m_process.ptr())
+                        return false;
+                }
+                return &item != targetItem;
+            });
+            m_process->send(Messages::WebPage::UpdateBackForwardListForReattach(WTFMove(itemStates)), m_pageID);
+            m_process->send(Messages::WebPage::GoToBackForwardItem(navigation->navigationID(), item->itemID(), *navigation->backForwardFrameLoadType(), ShouldTreatAsContinuingLoad::Yes), m_pageID);
+            m_process->responsivenessTimer().start();
 
-        auto transaction = m_pageLoadState.transaction();
-        m_pageLoadState.setPendingAPIRequestURL(transaction, item->url());
+            return;
+        }
 
-        auto itemStates = m_backForwardList->filteredItemStates([this, targetItem = item](WebBackForwardListItem& item) {
-            if (auto* page = item.suspendedPage()) {
-                if (&page->process() == m_process.ptr())
-                    return false;
-            }
-            return &item != targetItem;
-        });
-        m_process->send(Messages::WebPage::UpdateBackForwardListForReattach(WTFMove(itemStates)), m_pageID);
-        m_process->send(Messages::WebPage::GoToBackForwardItem(navigation.navigationID(), item->itemID(), *navigation.backForwardFrameLoadType(), ShouldTreatAsContinuingLoad::Yes), m_pageID);
-        m_process->responsivenessTimer().start();
+        if (navigation->lockBackForwardList() == LockBackForwardList::Yes || navigation->lockHistory() == LockHistory::Yes) {
+            // If WebCore is supposed to lock the history for this load, then the new process needs to know about the current history item so it can update
+            // it instead of creating a new one.
+            auto itemStates = m_backForwardList->filteredItemStates([currentItem = m_backForwardList->currentItem()](WebBackForwardListItem& item) {
+                return &item == currentItem;
+            });
+            RELEASE_ASSERT(itemStates.size() == 1);
+            m_process->send(Messages::WebPage::SetCurrentHistoryItemForReattach(itemStates[0]), m_pageID);
+        }
 
-        return;
-    }
+        // FIXME: Work out timing of responding with the last policy delegate, etc
+        ASSERT(!navigation->currentRequest().isEmpty());
+        if (auto& substituteData = navigation->substituteData())
+            loadDataWithNavigation(navigation, { substituteData->content.data(), substituteData->content.size() }, substituteData->MIMEType, substituteData->encoding, substituteData->baseURL, substituteData->userData.get());
+        else
+            loadRequestWithNavigation(navigation, ResourceRequest { navigation->currentRequest() }, WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, nullptr, ShouldTreatAsContinuingLoad::Yes);
 
-    if (navigation.lockBackForwardList() == LockBackForwardList::Yes || navigation.lockHistory() == LockHistory::Yes) {
-        // If WebCore is supposed to lock the history for this load, then the new process needs to know about the current history item so it can update
-        // it instead of creating a new one.
-        auto itemStates = m_backForwardList->filteredItemStates([currentItem = m_backForwardList->currentItem()](WebBackForwardListItem& item) {
-            return &item == currentItem;
-        });
-        RELEASE_ASSERT(itemStates.size() == 1);
-        m_process->send(Messages::WebPage::SetCurrentHistoryItemForReattach(itemStates[0]), m_pageID);
-    }
+        ASSERT(!m_mainFrame);
+        m_mainFrameCreationHandler = [this, protectedThis = makeRef(*this), navigation = navigation.copyRef(), request =  navigation->currentRequest(), mainFrameURL, isServerRedirect = navigation->currentRequestIsRedirect()]() mutable {
+            ASSERT(m_mainFrame);
+            // Restore the main frame's committed URL as some clients may rely on it until the next load is committed.
+            m_mainFrame->frameLoadState().setURL(mainFrameURL);
 
-    // FIXME: Work out timing of responding with the last policy delegate, etc
-    ASSERT(!navigation.currentRequest().isEmpty());
-    if (auto& substituteData = navigation.substituteData())
-        loadDataWithNavigation(navigation, { substituteData->content.data(), substituteData->content.size() }, substituteData->MIMEType, substituteData->encoding, substituteData->baseURL, substituteData->userData.get());
-    else
-        loadRequestWithNavigation(navigation, ResourceRequest { navigation.currentRequest() }, WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, nullptr, ShouldTreatAsContinuingLoad::Yes);
+            // Normally, notification of a server redirect comes from the WebContent process.
+            // If we are process swapping in response to a server redirect then that notification will not come from the new WebContent process.
+            // In this case we have the UIProcess synthesize the redirect notification at the appropriate time.
+            if (isServerRedirect) {
+                m_mainFrame->frameLoadState().didStartProvisionalLoad(request.url());
+                didReceiveServerRedirectForProvisionalLoadForFrame(m_mainFrame->frameID(), navigation->navigationID(), WTFMove(request), { });
+            }
+        };
 
-    ASSERT(!m_mainFrame);
-    m_mainFrameCreationHandler = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), request =  navigation.currentRequest(), mainFrameURL, isServerRedirect = navigation.currentRequestIsRedirect()]() mutable {
-        ASSERT(m_mainFrame);
-        // Restore the main frame's committed URL as some clients may rely on it until the next load is committed.
-        m_mainFrame->frameLoadState().setURL(mainFrameURL);
+        bool isInitialNavigationInNewWindow = openedByDOM() && !hasCommittedAnyProvisionalLoads();
+        if (!isInitialNavigationInNewWindow || !mainFrameIDInPreviousProcess)
+            return;
 
-        // Normally, notification of a server redirect comes from the WebContent process.
-        // If we are process swapping in response to a server redirect then that notification will not come from the new WebContent process.
-        // In this case we have the UIProcess synthesize the redirect notification at the appropriate time.
-        if (isServerRedirect) {
-            m_mainFrame->frameLoadState().didStartProvisionalLoad(request.url());
-            didReceiveServerRedirectForProvisionalLoadForFrame(m_mainFrame->frameID(), navigation->navigationID(), WTFMove(request), { });
-        }
-    };
-
-    bool isInitialNavigationInNewWindow = openedByDOM() && !hasCommittedAnyProvisionalLoads();
-    if (!isInitialNavigationInNewWindow || !mainFrameIDInPreviousProcess)
-        return;
-
-    m_mainFrameWindowCreationHandler = [this, previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess = *mainFrameIDInPreviousProcess](const GlobalWindowIdentifier& windowIdentifier) {
-        ASSERT(m_mainFrame);
-        GlobalFrameIdentifier navigatedFrameIdentifierInNewProcess { pageID(), m_mainFrame->frameID() };
-        previousProcess->send(Messages::WebPage::FrameBecameRemote(mainFrameIDInPreviousProcess, navigatedFrameIdentifierInNewProcess, windowIdentifier), pageID());
-    };
+        m_mainFrameWindowCreationHandler = [this, previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess = *mainFrameIDInPreviousProcess](const GlobalWindowIdentifier& windowIdentifier) {
+            ASSERT(m_mainFrame);
+            GlobalFrameIdentifier navigatedFrameIdentifierInNewProcess { pageID(), m_mainFrame->frameID() };
+            previousProcess->send(Messages::WebPage::FrameBecameRemote(mainFrameIDInPreviousProcess, navigatedFrameIdentifierInNewProcess, windowIdentifier), pageID());
+        };
+    });
 }
 
 void WebPageProxy::setUserAgent(String&& userAgent)

Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (238352 => 238353)


--- trunk/Source/WebKit/UIProcess/WebPageProxy.h	2018-11-17 22:01:15 UTC (rev 238352)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h	2018-11-17 22:06:56 UTC (rev 238353)
@@ -1543,7 +1543,7 @@
     void setCanShortCircuitHorizontalWheelEvents(bool canShortCircuitHorizontalWheelEvents) { m_canShortCircuitHorizontalWheelEvents = canShortCircuitHorizontalWheelEvents; }
 
     void reattachToWebProcess();
-    void swapToWebProcess(Ref<WebProcessProxy>&&, API::Navigation&, std::optional<uint64_t> mainFrameIDInPreviousProcess);
+    void swapToWebProcess(Ref<WebProcessProxy>&&, API::Navigation&, std::optional<uint64_t> mainFrameIDInPreviousProcess, CompletionHandler<void()>&&);
 
     void finishAttachingToWebProcess(ShouldDelayAttachingDrawingArea = ShouldDelayAttachingDrawingArea::No);
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to