Title: [235988] trunk/Source/WebKit
- Revision
- 235988
- Author
- cdu...@apple.com
- Date
- 2018-09-13 13:52:34 -0700 (Thu, 13 Sep 2018)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (235987 => 235988)
--- trunk/Source/WebKit/ChangeLog 2018-09-13 20:44:43 UTC (rev 235987)
+++ trunk/Source/WebKit/ChangeLog 2018-09-13 20:52:34 UTC (rev 235988)
@@ -1,5 +1,36 @@
2018-09-13 Chris Dumez <cdu...@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 <cdu...@apple.com>
+
Add release logging to help debug PSON issues
https://bugs.webkit.org/show_bug.cgi?id=189562
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (235987 => 235988)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-09-13 20:44:43 UTC (rev 235987)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-09-13 20:52:34 UTC (rev 235988)
@@ -705,7 +705,7 @@
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 @@
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 @@
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 @@
// 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());
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (235987 => 235988)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.h 2018-09-13 20:44:43 UTC (rev 235987)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h 2018-09-13 20:52:34 UTC (rev 235988)
@@ -1502,13 +1502,8 @@
void setCanShortCircuitHorizontalWheelEvents(bool canShortCircuitHorizontalWheelEvents) { m_canShortCircuitHorizontalWheelEvents = canShortCircuitHorizontalWheelEvents; }
void reattachToWebProcess();
+ void reattachToWebProcess(Ref<WebProcessProxy>&&, API::Navigation*);
- enum class ReattachForBackForward {
- Yes,
- No
- };
- void reattachToWebProcess(Ref<WebProcessProxy>&&, API::Navigation*, ReattachForBackForward);
-
RefPtr<API::Navigation> reattachToWebProcessForReload();
RefPtr<API::Navigation> reattachToWebProcessWithItem(WebBackForwardListItem&);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
