Attachment 349689 Details for Bug 189599 – Patch

[patch] Patch

bug-189599-20180913121647.patch (text/plain), 5.91 KB, created by Chris Dumez on 2018-09-13 12:16:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-13 12:16:47 PDT

Size: 5.91 KB

patch

obsolete

>Subversion Revision: 235979
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index f4b2485b20eef799a959ac72bfdc64657f016e86..57e650905ecede08b0db9485afe76e37eab12d1b 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,34 @@
>+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 NOBODY (OOPS!).
>+
>+        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  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r235953.
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 79dcc5980039dca239869d77f11287b7ae1a1ad5..db5310cf7eef369048b1a4c0a4d49db51f814262 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -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());
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index 3d3372086281f0749cff235bf36a458f862dc5c3..a9d3072975cec2989e7542de7ef12e696b4a5c48 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -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&);

Actions: View | Formatted Diff | Diff

Attachments on bug 189599: 349689