ProcessSwap.BackWithoutSuspendedPage API test hits assertion under WebPageProxy:...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Sep 2018 20:52:34 +0000 (20:52 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Sep 2018 20:52:34 +0000 (20:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189599

Reviewed by Geoffrey Garen.

The code in WebPageProxy::reattachToWebProcess() was re-initializing m_mainFrame unconditionally in case
of a HistoryNavigation. The reason we need to initialize m_mainFrame in reattachToWebProcess() is if the
process we're reattaching to already has a WebPage (with a main frame), in which case
WebPageProxy::didCreateMainFrame() would not get called to initialize WebPageProxy::m_mainFrame.

The process we're reattaching to can be in such a state only if it comes from a SuspendedPageProxy (we
detached the WebProcessProxy from the WebPageProxy but kept the WebPage in the "suspended" WebProcess).
It is true that we're only reattaching to a SuspendedPageProxy's process in the event of history
navigations. However, it is not true that all history navigations will use a SuspendedPageProxy's process.
For example, no SuspendedPageProxy may be available for the history navigation because the history
was restored to a new view from disk, or because the WebBackForwardListItem no longer has a
SuspendedPageProxy (we currently only keep a single SuspendedPageProxy for the last HistoryItem).

Therefore, unconditionally initializating m_mainFrame in reattachToWebProcess() for history navigations
is incorrect and we should instead check if we're reattaching to a SuspendedPage's process.

Change is covered by ProcessSwap.BackWithoutSuspendedPage API test which is no longer crashes and
existing Back/Forward PSON API tests which are still passing.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::reattachToWebProcess):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
* UIProcess/WebPageProxy.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@235988 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h

index 47d57ff..3cec092 100644 (file)
@@ -1,5 +1,36 @@
 2018-09-13  Chris Dumez  <cdumez@apple.com>
 
+        ProcessSwap.BackWithoutSuspendedPage API test hits assertion under WebPageProxy::didCreateMainFrame()
+        https://bugs.webkit.org/show_bug.cgi?id=189599
+
+        Reviewed by Geoffrey Garen.
+
+        The code in WebPageProxy::reattachToWebProcess() was re-initializing m_mainFrame unconditionally in case
+        of a HistoryNavigation. The reason we need to initialize m_mainFrame in reattachToWebProcess() is if the
+        process we're reattaching to already has a WebPage (with a main frame), in which case
+        WebPageProxy::didCreateMainFrame() would not get called to initialize WebPageProxy::m_mainFrame.
+
+        The process we're reattaching to can be in such a state only if it comes from a SuspendedPageProxy (we
+        detached the WebProcessProxy from the WebPageProxy but kept the WebPage in the "suspended" WebProcess).
+        It is true that we're only reattaching to a SuspendedPageProxy's process in the event of history
+        navigations. However, it is not true that all history navigations will use a SuspendedPageProxy's process.
+        For example, no SuspendedPageProxy may be available for the history navigation because the history
+        was restored to a new view from disk, or because the WebBackForwardListItem no longer has a
+        SuspendedPageProxy (we currently only keep a single SuspendedPageProxy for the last HistoryItem).
+
+        Therefore, unconditionally initializating m_mainFrame in reattachToWebProcess() for history navigations
+        is incorrect and we should instead check if we're reattaching to a SuspendedPage's process.
+
+        Change is covered by ProcessSwap.BackWithoutSuspendedPage API test which is no longer crashes and
+        existing Back/Forward PSON API tests which are still passing.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::reattachToWebProcess):
+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
+        * UIProcess/WebPageProxy.h:
+
+2018-09-13  Chris Dumez  <cdumez@apple.com>
+
         Add release logging to help debug PSON issues
         https://bugs.webkit.org/show_bug.cgi?id=189562
 
index 7eca8fd..29195c6 100644 (file)
@@ -705,7 +705,7 @@ void WebPageProxy::handleSynchronousMessage(IPC::Connection& connection, const S
 void WebPageProxy::reattachToWebProcess()
 {
     auto process = makeRef(m_process->processPool().createNewWebProcessRespectingProcessCountLimit(m_websiteDataStore.get()));
-    reattachToWebProcess(WTFMove(process), nullptr, ReattachForBackForward::No);
+    reattachToWebProcess(WTFMove(process), nullptr);
 }
 
 SuspendedPageProxy* WebPageProxy::maybeCreateSuspendedPage(WebProcessProxy& process, API::Navigation& navigation)
@@ -731,7 +731,7 @@ void WebPageProxy::suspendedPageClosed(SuspendedPageProxy& page)
     m_suspendedPage = nullptr;
 }
 
-void WebPageProxy::reattachToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation* navigation, ReattachForBackForward reattachForBackForward)
+void WebPageProxy::reattachToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation* navigation)
 {
     ASSERT(!m_isClosed);
     ASSERT(!isValid());
@@ -753,7 +753,11 @@ void WebPageProxy::reattachToWebProcess(Ref<WebProcessProxy>&& process, API::Nav
     if (m_process->state() == WebProcessProxy::State::Running)
         m_webProcessLifetimeTracker.webPageEnteringWebProcess();
 
-    if (reattachForBackForward == ReattachForBackForward::Yes) {
+    // 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 (currentSuspendedPage && currentSuspendedPage->process() == m_process.ptr()) {
         ASSERT(!m_mainFrame);
         ASSERT(m_mainFrameID);
         m_mainFrame = WebFrameProxy::create(this, *m_mainFrameID);
@@ -2482,7 +2486,7 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, R
 
     // FIXME: this is to fix the ASSERT(isValid()) inside reattachToWebProcess, some other way to fix this is needed.
     m_isValid = false;
-    reattachToWebProcess(WTFMove(process), &navigation, navigation.targetItem() ? ReattachForBackForward::Yes : ReattachForBackForward::No);
+    reattachToWebProcess(WTFMove(process), &navigation);
 
     if (auto* item = navigation.targetItem()) {
         LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
index 3d33720..a9d3072 100644 (file)
@@ -1502,12 +1502,7 @@ private:
     void setCanShortCircuitHorizontalWheelEvents(bool canShortCircuitHorizontalWheelEvents) { m_canShortCircuitHorizontalWheelEvents = canShortCircuitHorizontalWheelEvents; }
 
     void reattachToWebProcess();
-
-    enum class ReattachForBackForward {
-        Yes,
-        No
-    };
-    void reattachToWebProcess(Ref<WebProcessProxy>&&, API::Navigation*, ReattachForBackForward);
+    void reattachToWebProcess(Ref<WebProcessProxy>&&, API::Navigation*);
 
     RefPtr<API::Navigation> reattachToWebProcessForReload();
     RefPtr<API::Navigation> reattachToWebProcessWithItem(WebBackForwardListItem&);