Changeset 238353 in webkit


Ignore:
Timestamp:
Nov 17, 2018 2:06:56 PM (5 years ago)
Author:
Chris Dumez
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:
Location:
trunk/Source/WebKit
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r238351 r238353  
     12018-11-17  Chris Dumez  <cdumez@apple.com>
     2
     3        [PSON] ASSERTION FAILED: m_uncommittedState.state == State::Committed
     4        https://bugs.webkit.org/show_bug.cgi?id=191781
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        The crash was happening when switching to a suspended page that is not yet done
     9        suspending (e.g. in case of very fast back/forward navigation). The WebPageProxy
     10        would reattach to the suspended process and get load notifications that it did
     11        not expect since it did not schedule any load yet. Those notifications are for
     12        the about:blank load we do for page suspension.
     13
     14        To address the issue, make swapToWebProcess() asynchronous and take a completion
     15        handler. When we try to unsuspend a SuspendedPageProxy, we first make sure it
     16        is actually done suspending. If it is not done suspending, we wait until it is
     17        before telling in to unsuspend and proceeding with the new load.
     18
     19        * UIProcess/SuspendedPageProxy.cpp:
     20        (WebKit::SuspendedPageProxy::unsuspend):
     21        (WebKit::SuspendedPageProxy::didFinishLoad):
     22        * UIProcess/SuspendedPageProxy.h:
     23        * UIProcess/WebPageProxy.cpp:
     24        (WebKit::WebPageProxy::swapToWebProcess):
     25        (WebKit::WebPageProxy::continueNavigationInNewProcess):
     26        * UIProcess/WebPageProxy.h:
     27
    1282018-11-17  Chris Dumez  <cdumez@apple.com>
    229

  • trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp

    r238061 r238353  
    104104}
    105105
    106 void SuspendedPageProxy::unsuspend()
     106void SuspendedPageProxy::unsuspend(CompletionHandler<void()>&& completionHandler)
    107107{
    108108    ASSERT(m_isSuspended);
    109109
    110     m_isSuspended = false;
    111     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
    112     m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
     110    auto doUnsuspend = [this, completionHandler = WTFMove(completionHandler)]() mutable {
     111        m_isSuspended = false;
     112        m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
     113        m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
     114        completionHandler();
     115    };
     116
     117    if (!m_finishedSuspending) {
     118        ASSERT(!m_finishedSuspendingHandler);
     119        m_finishedSuspendingHandler = WTFMove(doUnsuspend);
     120    } else
     121        doUnsuspend();
    113122}
    114123
    … …  
    117126    LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
    118127
    119 #if !LOG_DISABLED
    120128    m_finishedSuspending = true;
    121 #endif
    122129
    123130    m_process->send(Messages::WebProcess::UpdateActivePages(), 0);
     131
     132    if (auto finishedSuspendingHandler = WTFMove(m_finishedSuspendingHandler))
     133        finishedSuspendingHandler();
    124134}
    125135

  • trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h

    r237735 r238353  
    4747    const String& registrableDomain() const { return m_registrableDomain; }
    4848
    49     void unsuspend();
     49    void unsuspend(CompletionHandler<void()>&&);
    5050
    5151#if !LOG_DISABLED
    … …  
    6767    bool m_isSuspended { true };
    6868
    69 #if !LOG_DISABLED
    7069    bool m_finishedSuspending { false };
    71 #endif
     70    CompletionHandler<void()> m_finishedSuspendingHandler;
    7271};
    7372

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r238338 r238353  
    751751}
    752752
    753 void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess)
     753void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess, CompletionHandler<void()>&& completionHandler)
    754754{
    755755    ASSERT(!m_isClosed);
    … …  
    761761            destinationSuspendedPage = this->process().processPool().takeSuspendedPage(*backForwardListItem->suspendedPage());
    762762            ASSERT(destinationSuspendedPage);
    763             destinationSuspendedPage->unsuspend();
    764763        }
    765764    }
    766765
    767     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
    768     bool didSuspendPreviousPage = suspendCurrentPageIfPossible(navigation, mainFrameIDInPreviousProcess);
    769     m_process->removeWebPage(*this, m_pageID);
    770 
    771     m_process = WTFMove(process);
    772     m_isValid = true;
    773 
    774     // If we are reattaching to a SuspendedPage, then the WebProcess' WebPage already exists and
    775     // WebPageProxy::didCreateMainFrame() will not be called to initialize m_mainFrame. In such
    776     // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
    777     // already exists and already has a main frame.
    778     if (destinationSuspendedPage) {
    779         ASSERT(!m_mainFrame);
    780         ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
    781         m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
    782         m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
    783     }
    784 
    785     finishAttachingToWebProcess(didSuspendPreviousPage ? ShouldDelayAttachingDrawingArea::Yes : ShouldDelayAttachingDrawingArea::No);
     766    auto* destinationSuspendedPagePtr = destinationSuspendedPage.get();
     767    auto doSwap = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), process = WTFMove(process), mainFrameIDInPreviousProcess, destinationSuspendedPage = WTFMove(destinationSuspendedPage), completionHandler = WTFMove(completionHandler)]() mutable {
     768        processDidTerminate(ProcessTerminationReason::NavigationSwap);
     769
     770        m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
     771        bool didSuspendPreviousPage = suspendCurrentPageIfPossible(navigation, mainFrameIDInPreviousProcess);
     772        m_process->removeWebPage(*this, m_pageID);
     773
     774        m_process = WTFMove(process);
     775        m_isValid = true;
     776
     777        // If we are reattaching to a SuspendedPage, then the WebProcess' WebPage already exists and
     778        // WebPageProxy::didCreateMainFrame() will not be called to initialize m_mainFrame. In such
     779        // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
     780        // already exists and already has a main frame.
     781        if (destinationSuspendedPage) {
     782            ASSERT(!m_mainFrame);
     783            ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
     784            m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
     785            m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
     786        }
     787
     788        finishAttachingToWebProcess(didSuspendPreviousPage ? ShouldDelayAttachingDrawingArea::Yes : ShouldDelayAttachingDrawingArea::No);
     789        completionHandler();
     790    };
     791
     792    if (destinationSuspendedPagePtr)
     793        destinationSuspendedPagePtr->unsuspend(WTFMove(doSwap));
     794    else
     795        doSwap();
    786796}
    787797
    … …  
    26492659    ASSERT(m_process.ptr() != process.ptr());
    26502660
    2651     processDidTerminate(ProcessTerminationReason::NavigationSwap);
    2652 
    2653     swapToWebProcess(WTFMove(process), navigation, mainFrameIDInPreviousProcess);
    2654 
    2655     if (auto* item = navigation.targetItem()) {
    2656         LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
    2657 
    2658         auto transaction = m_pageLoadState.transaction();
    2659         m_pageLoadState.setPendingAPIRequestURL(transaction, item->url());
    2660 
    2661         auto itemStates = m_backForwardList->filteredItemStates([this, targetItem = item](WebBackForwardListItem& item) {
    2662             if (auto* page = item.suspendedPage()) {
    2663                 if (&page->process() == m_process.ptr())
    2664                     return false;
     2661    swapToWebProcess(WTFMove(process), navigation, mainFrameIDInPreviousProcess, [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess, mainFrameURL = WTFMove(mainFrameURL)]() mutable {
     2662        if (auto* item = navigation->targetItem()) {
     2663            LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
     2664
     2665            auto transaction = m_pageLoadState.transaction();
     2666            m_pageLoadState.setPendingAPIRequestURL(transaction, item->url());
     2667
     2668            auto itemStates = m_backForwardList->filteredItemStates([this, targetItem = item](WebBackForwardListItem& item) {
     2669                if (auto* page = item.suspendedPage()) {
     2670                    if (&page->process() == m_process.ptr())
     2671                        return false;
     2672                }
     2673                return &item != targetItem;
     2674            });
     2675            m_process->send(Messages::WebPage::UpdateBackForwardListForReattach(WTFMove(itemStates)), m_pageID);
     2676            m_process->send(Messages::WebPage::GoToBackForwardItem(navigation->navigationID(), item->itemID(), *navigation->backForwardFrameLoadType(), ShouldTreatAsContinuingLoad::Yes), m_pageID);
     2677            m_process->responsivenessTimer().start();
     2678
     2679            return;
     2680        }
     2681
     2682        if (navigation->lockBackForwardList() == LockBackForwardList::Yes || navigation->lockHistory() == LockHistory::Yes) {
     2683            // 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
     2684            // it instead of creating a new one.
     2685            auto itemStates = m_backForwardList->filteredItemStates([currentItem = m_backForwardList->currentItem()](WebBackForwardListItem& item) {
     2686                return &item == currentItem;
     2687            });
     2688            RELEASE_ASSERT(itemStates.size() == 1);
     2689            m_process->send(Messages::WebPage::SetCurrentHistoryItemForReattach(itemStates[0]), m_pageID);
     2690        }
     2691
     2692        // FIXME: Work out timing of responding with the last policy delegate, etc
     2693        ASSERT(!navigation->currentRequest().isEmpty());
     2694        if (auto& substituteData = navigation->substituteData())
     2695            loadDataWithNavigation(navigation, { substituteData->content.data(), substituteData->content.size() }, substituteData->MIMEType, substituteData->encoding, substituteData->baseURL, substituteData->userData.get());
     2696        else
     2697            loadRequestWithNavigation(navigation, ResourceRequest { navigation->currentRequest() }, WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, nullptr, ShouldTreatAsContinuingLoad::Yes);
     2698
     2699        ASSERT(!m_mainFrame);
     2700        m_mainFrameCreationHandler = [this, protectedThis = makeRef(*this), navigation = navigation.copyRef(), request =  navigation->currentRequest(), mainFrameURL, isServerRedirect = navigation->currentRequestIsRedirect()]() mutable {
     2701            ASSERT(m_mainFrame);
     2702            // Restore the main frame's committed URL as some clients may rely on it until the next load is committed.
     2703            m_mainFrame->frameLoadState().setURL(mainFrameURL);
     2704
     2705            // Normally, notification of a server redirect comes from the WebContent process.
     2706            // If we are process swapping in response to a server redirect then that notification will not come from the new WebContent process.
     2707            // In this case we have the UIProcess synthesize the redirect notification at the appropriate time.
     2708            if (isServerRedirect) {
     2709                m_mainFrame->frameLoadState().didStartProvisionalLoad(request.url());
     2710                didReceiveServerRedirectForProvisionalLoadForFrame(m_mainFrame->frameID(), navigation->navigationID(), WTFMove(request), { });
    26652711            }
    2666             return &item != targetItem;
    2667         });
    2668         m_process->send(Messages::WebPage::UpdateBackForwardListForReattach(WTFMove(itemStates)), m_pageID);
    2669         m_process->send(Messages::WebPage::GoToBackForwardItem(navigation.navigationID(), item->itemID(), *navigation.backForwardFrameLoadType(), ShouldTreatAsContinuingLoad::Yes), m_pageID);
    2670         m_process->responsivenessTimer().start();
    2671 
    2672         return;
    2673     }
    2674 
    2675     if (navigation.lockBackForwardList() == LockBackForwardList::Yes || navigation.lockHistory() == LockHistory::Yes) {
    2676         // 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
    2677         // it instead of creating a new one.
    2678         auto itemStates = m_backForwardList->filteredItemStates([currentItem = m_backForwardList->currentItem()](WebBackForwardListItem& item) {
    2679             return &item == currentItem;
    2680         });
    2681         RELEASE_ASSERT(itemStates.size() == 1);
    2682         m_process->send(Messages::WebPage::SetCurrentHistoryItemForReattach(itemStates[0]), m_pageID);
    2683     }
    2684 
    2685     // FIXME: Work out timing of responding with the last policy delegate, etc
    2686     ASSERT(!navigation.currentRequest().isEmpty());
    2687     if (auto& substituteData = navigation.substituteData())
    2688         loadDataWithNavigation(navigation, { substituteData->content.data(), substituteData->content.size() }, substituteData->MIMEType, substituteData->encoding, substituteData->baseURL, substituteData->userData.get());
    2689     else
    2690         loadRequestWithNavigation(navigation, ResourceRequest { navigation.currentRequest() }, WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, nullptr, ShouldTreatAsContinuingLoad::Yes);
    2691 
    2692     ASSERT(!m_mainFrame);
    2693     m_mainFrameCreationHandler = [this, protectedThis = makeRef(*this), navigation = makeRef(navigation), request =  navigation.currentRequest(), mainFrameURL, isServerRedirect = navigation.currentRequestIsRedirect()]() mutable {
    2694         ASSERT(m_mainFrame);
    2695         // Restore the main frame's committed URL as some clients may rely on it until the next load is committed.
    2696         m_mainFrame->frameLoadState().setURL(mainFrameURL);
    2697 
    2698         // Normally, notification of a server redirect comes from the WebContent process.
    2699         // If we are process swapping in response to a server redirect then that notification will not come from the new WebContent process.
    2700         // In this case we have the UIProcess synthesize the redirect notification at the appropriate time.
    2701         if (isServerRedirect) {
    2702             m_mainFrame->frameLoadState().didStartProvisionalLoad(request.url());
    2703             didReceiveServerRedirectForProvisionalLoadForFrame(m_mainFrame->frameID(), navigation->navigationID(), WTFMove(request), { });
    2704         }
    2705     };
    2706 
    2707     bool isInitialNavigationInNewWindow = openedByDOM() && !hasCommittedAnyProvisionalLoads();
    2708     if (!isInitialNavigationInNewWindow || !mainFrameIDInPreviousProcess)
    2709         return;
    2710 
    2711     m_mainFrameWindowCreationHandler = [this, previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess = *mainFrameIDInPreviousProcess](const GlobalWindowIdentifier& windowIdentifier) {
    2712         ASSERT(m_mainFrame);
    2713         GlobalFrameIdentifier navigatedFrameIdentifierInNewProcess { pageID(), m_mainFrame->frameID() };
    2714         previousProcess->send(Messages::WebPage::FrameBecameRemote(mainFrameIDInPreviousProcess, navigatedFrameIdentifierInNewProcess, windowIdentifier), pageID());
    2715     };
     2712        };
     2713
     2714        bool isInitialNavigationInNewWindow = openedByDOM() && !hasCommittedAnyProvisionalLoads();
     2715        if (!isInitialNavigationInNewWindow || !mainFrameIDInPreviousProcess)
     2716            return;
     2717
     2718        m_mainFrameWindowCreationHandler = [this, previousProcess = WTFMove(previousProcess), mainFrameIDInPreviousProcess = *mainFrameIDInPreviousProcess](const GlobalWindowIdentifier& windowIdentifier) {
     2719            ASSERT(m_mainFrame);
     2720            GlobalFrameIdentifier navigatedFrameIdentifierInNewProcess { pageID(), m_mainFrame->frameID() };
     2721            previousProcess->send(Messages::WebPage::FrameBecameRemote(mainFrameIDInPreviousProcess, navigatedFrameIdentifierInNewProcess, windowIdentifier), pageID());
     2722        };
     2723    });
    27162724}
    27172725

  • trunk/Source/WebKit/UIProcess/WebPageProxy.h

    r238330 r238353  
    15441544
    15451545    void reattachToWebProcess();
    1546     void swapToWebProcess(Ref<WebProcessProxy>&&, API::Navigation&, std::optional<uint64_t> mainFrameIDInPreviousProcess);
     1546    void swapToWebProcess(Ref<WebProcessProxy>&&, API::Navigation&, std::optional<uint64_t> mainFrameIDInPreviousProcess, CompletionHandler<void()>&&);
    15471547
    15481548    void finishAttachingToWebProcess(ShouldDelayAttachingDrawingArea = ShouldDelayAttachingDrawingArea::No);
Note: See TracChangeset for help on using the changeset viewer.