WebKit Bugzilla
Attachment 362144 Details for
Bug 194717
: Regression(PSON) Navigating quickly back and forth can lead to getting 'about:blank' in the backforward list
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-194717-20190215121835.patch (text/plain), 13.38 KB, created by
Chris Dumez
on 2019-02-15 12:18:36 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Chris Dumez
Created:
2019-02-15 12:18:36 PST
Size:
13.38 KB
patch
obsolete
>Subversion Revision: 241557 >diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog >index 008f9d6544055b05bc01b4837bccc89989ac375c..c1488ad2f3940ced323fed537e8149d3d5063d0e 100644 >--- a/Source/WebKit/ChangeLog >+++ b/Source/WebKit/ChangeLog >@@ -1,3 +1,48 @@ >+2019-02-15 Chris Dumez <cdumez@apple.com> >+ >+ Regression(PSON) Navigating quickly back and forth can lead to getting 'about:blank' in the backforward list >+ https://bugs.webkit.org/show_bug.cgi?id=194717 >+ <rdar://problem/47884404> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ When the client does a history navigation, the UIProcess sends a WebPage::GoToBackForwardItem IPC to the >+ WebProcess and the WebProcess sends a WebPageProxy::BackForwardGoToItem IPC back to the UIProcess to >+ update the current item in the BackForwardList. This means that there is a slight delay between the >+ point a client requests a history navigation and the point where the BackForwardList's current item gets >+ update. This delay is pre-existing behavior and not new to PSON. >+ >+ However, with PSON enabled, if we decide to process-swap for the history navigation, we'll tell the >+ previous (committed) process to ignore the load and we ask a new (provisional) process to do the history >+ navigation. When the previous process receives the request to ignore the history navigation, it restores >+ the History's current item to the one previous the navigation, which sends a WebPageProxy::GoToBackForwardItem >+ IPC to the UIProcess to update the BackForwardList as well. In parallel, the new process starts the >+ history navigation and also sends a WebPageProxy::GoToBackForwardItem to update the BackForwardList's >+ current item as well. We end up with a race between the 2 GoToBackForwardItem IPC messages coming from >+ the old and new process. If the old process's message loses the race, we end up with the wrong current >+ history item getting set in the UIProcess. Later, when we commit the provisional load and try to suspend >+ the previous page, we would save the SuspendedPage on the *wrong* BackForwardList item. If one tries to >+ load this BackForwardList item later, we'll use its SuspendedPage and try to unsuspend it. However, >+ because the PageCache entry is saved on another HistoryItem than the one getting loaded in the WebProcess >+ side, we attempt to do a regular load instead of a PageCache restore. We end up failing the load because >+ pages cannot trigger new loads while in page cache. Because the load fails, we end up loading the >+ initial empty document and this is how we end up with 'about:blank' in the back forward list. >+ >+ To address the issue, update WebPageProxy::backForwardGoToItem() to ignore messages from the old/committed >+ WebProcess when there is a pending provisional load. If the committed processes starts a legit new >+ load, it would clear any existing pending provisional load before attempting the call backForwardGoToItem(). >+ As a result, ignoring such messages from the old processes when there is a pending provisional load is >+ safe. >+ >+ In the future, we should probably move more of the history / backForwardList management to the UIProcess >+ to avoid this sort of issues. This would be a much larger refactoring though so I am going with this >+ simpler fix that is easily cherry-pickable for now. >+ >+ * UIProcess/WebPageProxy.cpp: >+ (WebKit::WebPageProxy::suspendCurrentPageIfPossible): >+ (WebKit::WebPageProxy::continueNavigationInNewProcess): >+ (WebKit::WebPageProxy::backForwardGoToItem): >+ > 2019-02-14 Chris Dumez <cdumez@apple.com> > > [PSON] Introduce a WebContent Process cache >diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp >index 174531a4345fb85bbdc8089d5fd93f7e35781ff9..f0a3d4c8fafaa111337f87e2dd058c81260baade 100644 >--- a/Source/WebKit/UIProcess/WebPageProxy.cpp >+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp >@@ -245,6 +245,7 @@ > #define MESSAGE_CHECK_URL(process, url) MESSAGE_CHECK_BASE(checkURLReceivedFromCurrentOrPreviousWebProcess(process, url), process->connection()) > > #define RELEASE_LOG_IF_ALLOWED(channel, fmt, ...) RELEASE_LOG_IF(isAlwaysOnLoggingAllowed(), channel, "%p - WebPageProxy::" fmt, this, ##__VA_ARGS__) >+#define RELEASE_LOG_ERROR_IF_ALLOWED(channel, fmt, ...) RELEASE_LOG_ERROR_IF(isAlwaysOnLoggingAllowed(), channel, "%p - WebPageProxy::" fmt, this, ##__VA_ARGS__) > > // Represents the number of wheel events we can hold in the queue before we start pushing them preemptively. > static const unsigned wheelEventQueueSizeThreshold = 10; >@@ -760,23 +761,36 @@ bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Opt > return false; > > // If the client forced a swap then it may not be Web-compatible to suspend the previous page because other windows may have an opener link to the page. >- if (processSwapRequestedByClient == ProcessSwapRequestedByClient::Yes) >+ if (processSwapRequestedByClient == ProcessSwapRequestedByClient::Yes) { >+ RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because the swap was requested by the client", m_process->processIdentifier()); > return false; >+ } > >- if (isPageOpenedByDOMShowingInitialEmptyDocument()) >+ if (isPageOpenedByDOMShowingInitialEmptyDocument()) { >+ RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because it is showing the initial empty document", m_process->processIdentifier()); > return false; >+ } > > auto* fromItem = navigation.fromItem(); > if (!fromItem) { >- LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " unable to create suspended page for process pid %i - No current back/forward item", pageID(), m_process->processIdentifier()); >+ RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because the navigation does not have a fromItem", m_process->processIdentifier()); > return false; > } > > // If the source and the destination back / forward list items are the same, then this is a client-side redirect. In this case, > // there is no need to suspend the previous page as there will be no way to get back to it. >- if (fromItem == m_backForwardList->currentItem()) >+ if (fromItem == m_backForwardList->currentItem()) { >+ RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because this is a client-side redirect", m_process->processIdentifier()); > return false; >+ } > >+ if (fromItem->url() != pageLoadState().url()) { >+ RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because fromItem's URL does not match the page URL.", m_process->processIdentifier()); >+ ASSERT_NOT_REACHED(); >+ return false; >+ } >+ >+ RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Suspending current page for process pid %i", m_process->processIdentifier()); > auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *fromItem, *mainFrameID); > > LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), suspendedPage->loggingString(), m_process->processIdentifier(), fromItem->itemID().logString()); >@@ -2836,6 +2850,7 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, s > LOG(Loading, "Continuing navigation %" PRIu64 " '%s' in a new web process", navigation.navigationID(), navigation.loggingString()); > > if (m_provisionalPage) { >+ RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "continueNavigationInNewProcess: There is already a pending provisional load, cancelling it (provisonalNavigationID: %llu, navigationID: %llu)", m_provisionalPage->navigationID(), navigation.navigationID()); > if (m_provisionalPage->navigationID() != navigation.navigationID()) > m_provisionalPage->cancel(); > m_provisionalPage = nullptr; >@@ -5475,6 +5490,12 @@ void WebPageProxy::backForwardAddItem(BackForwardListItemState&& itemState) > > void WebPageProxy::backForwardGoToItem(const BackForwardItemIdentifier& itemID, SandboxExtension::Handle& sandboxExtensionHandle) > { >+ // On process swap, we tell the previous process to ignore the load, which causes it so restore its current back forward item to its previous >+ // value. Since the load is really going on in a new provisional process, we want to ignore such requests from the committed process. >+ // Any real new load in the committed process would have cleared m_provisionalPage. >+ if (m_provisionalPage) >+ return; >+ > backForwardGoToItemShared(m_process.copyRef(), itemID, sandboxExtensionHandle); > } > >diff --git a/Tools/ChangeLog b/Tools/ChangeLog >index d9750ee42f028baa8311312f45a02594f64532d2..a37b248312b73f43bd2478415dcdc2ccb941c2e2 100644 >--- a/Tools/ChangeLog >+++ b/Tools/ChangeLog >@@ -1,3 +1,15 @@ >+2019-02-15 Chris Dumez <cdumez@apple.com> >+ >+ Regression(PSON) Navigating quickly back and forth can lead to getting 'about:blank' in the backforward list >+ https://bugs.webkit.org/show_bug.cgi?id=194717 >+ <rdar://problem/47884404> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Add API test coverage. >+ >+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm: >+ > 2019-02-14 Chris Dumez <cdumez@apple.com> > > [PSON] Introduce a WebContent Process cache >diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm >index c2778501858dbe8b7c5bc59c1bd440c5783b614e..510ffba5b3e54c8e9f8fd52f9011c64fb944d3fe 100644 >--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm >+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm >@@ -2023,6 +2023,81 @@ TEST(ProcessSwap, NavigationWithLockedHistoryWithPSON) > runNavigationWithLockedHistoryTest(ShouldEnablePSON::Yes); > } > >+static void runQuickBackForwardNavigationTest(ShouldEnablePSON shouldEnablePSON) >+{ >+ auto processPoolConfiguration = psonProcessPoolConfiguration(); >+ processPoolConfiguration.get().processSwapsOnNavigation = shouldEnablePSON == ShouldEnablePSON::Yes ? YES : NO; >+ auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]); >+ >+ auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]); >+ [webViewConfiguration setProcessPool:processPool.get()]; >+ auto handler = adoptNS([[PSONScheme alloc] init]); >+ [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"pson"]; >+ >+ auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]); >+ auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]); >+ [webView setNavigationDelegate:delegate.get()]; >+ >+ [webView configuration].preferences.safeBrowsingEnabled = NO; >+ >+ NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]]; >+ [webView loadRequest:request]; >+ >+ TestWebKitAPI::Util::run(&done); >+ done = false; >+ >+ auto webkitPID = [webView _webProcessIdentifier]; >+ >+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]]; >+ [webView loadRequest:request]; >+ >+ TestWebKitAPI::Util::run(&done); >+ done = false; >+ >+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]); >+ >+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]]; >+ [webView loadRequest:request]; >+ >+ TestWebKitAPI::Util::run(&done); >+ done = false; >+ >+ auto applePID = [webView _webProcessIdentifier]; >+ if (shouldEnablePSON == ShouldEnablePSON::Yes) >+ EXPECT_NE(webkitPID, applePID); >+ else >+ EXPECT_EQ(webkitPID, applePID); >+ >+ for (unsigned i = 0; i < 10; ++i) { >+ [webView goBack]; >+ TestWebKitAPI::Util::sleep(0.1); >+ [webView goForward]; >+ TestWebKitAPI::Util::spinRunLoop(0.1); >+ } >+ >+ Vector<String> backForwardListURLs; >+ auto* backForwardList = [webView backForwardList]; >+ for (unsigned i = 0; i < backForwardList.backList.count; ++i) >+ backForwardListURLs.append([backForwardList.backList[i].URL absoluteString]); >+ backForwardListURLs.append([backForwardList.currentItem.URL absoluteString]); >+ for (unsigned i = 0; i < backForwardList.forwardList.count; ++i) >+ backForwardListURLs.append([backForwardList.forwardList[i].URL absoluteString]); >+ EXPECT_EQ(3u, backForwardListURLs.size()); >+ EXPECT_WK_STREQ("pson://www.webkit.org/main1.html", backForwardListURLs[0]); >+ EXPECT_WK_STREQ("pson://www.webkit.org/main2.html", backForwardListURLs[1]); >+ EXPECT_WK_STREQ("pson://www.apple.com/main.html", backForwardListURLs[2]); >+} >+ >+TEST(ProcessSwap, QuickBackForwardNavigationWithoutPSON) >+{ >+ runQuickBackForwardNavigationTest(ShouldEnablePSON::No); >+} >+ >+TEST(ProcessSwap, QuickBackForwardNavigationWithPSON) >+{ >+ runQuickBackForwardNavigationTest(ShouldEnablePSON::Yes); >+} >+ > TEST(ProcessSwap, NavigationWithLockedHistoryWithoutPSON) > { > runNavigationWithLockedHistoryTest(ShouldEnablePSON::No);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194717
: 362144