- Revision
- 269170
- Author
- [email protected]
- Date
- 2020-10-29 14:35:14 -0700 (Thu, 29 Oct 2020)
Log Message
Source/WebCore:
Regression(PSON): Back/forward navigation may hang
https://bugs.webkit.org/show_bug.cgi?id=216611
<rdar://problem/68992714>
Reviewed by Geoffrey Garen.
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadItem):
Add assertion that would have hit when reproducing the bug. A crash in debug is better than
hanging.
Source/WebKit:
Regression(PSON): Back/forward navigation may hang
https://bugs.webkit.org/show_bug.cgi?id=216611
<rdar://problem/68992714>
Reviewed by Geoffrey Garen.
The scenario was as follows:
1. Load A in Process P1
2. Load A#foo in Process P1 (fragment navigation)
3. Load B in Process P2
4. history.go(-2) to load A again in P1
5. history.go(2) to load B again in P2
6. history.back() to go back to A#foo in P1
-> Hang
We doing the process swap at step 3, we would suspend the page containing A#foo
in P1 when doing the new load in P2. When process swapping again at step 4,
we would go back to process P1 (because it is same origin) but we did not have
any suspended WebPage for A so we would create a new WebPage (with a new
WebPageIdentifier). The reason we do this is to allow the suspended WebPage
for A#foo to remain in process, as we may want it for another back/forward
navigation later.
The issue was that when constructing the new WebPage in P1 for A at step 4,
we would restore all history items from the WebPageProxy's BackForwardList
and overwrite any existing HistoryItems with the same IDs in the WebProcess.
This meant that WebKit layer would construct new HistoryItems for
HistoryItems that already existing and are used by the Suspended WebPage
containing A#foo.
Later on, at step 6, when trying to go back A#foo in P1, we would ask
the Suspended WebPage in P1 to load a given HistoryItem. This HistoryItem
is supposed to be present in the Page's back/forward list and is supposed
to have an associated back/forward cache entry. However, it was not the
case here because we ended up with a new HistoryItem instance that was
restored from the UIProcess earlier.
To address the issue, The WebPage constructor no longer overwrites
existing HistoryItems. Note that before prior to r231048, the WebPage
constructor was NOT overwriting existing HistoryItems so this restores
previous behavior. Presumably, we started overwriting to stop hitting
the assertion in WebBackForwardListProxy::addItemFromUIProcess() that
checked that we either wanted to overwrite or that there was no
existing HistoryItem with the given ID. This is no longer an issue
since I replaced this assertion with an if-check and an early return.
* WebProcess/WebPage/WebBackForwardListProxy.cpp:
(WebKit::WebBackForwardListProxy::addItemFromUIProcess):
* WebProcess/WebPage/WebPage.cpp:
Tools:
Regression(PSON): Back/forward navigation may hang
https://bugs.webkit.org/show_bug.cgi?id=216611
<rdar://problem/68992714>
Reviewed by Geoffrey Garen.
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (269169 => 269170)
--- trunk/Source/WebCore/ChangeLog 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Source/WebCore/ChangeLog 2020-10-29 21:35:14 UTC (rev 269170)
@@ -1,3 +1,16 @@
+2020-10-29 Chris Dumez <[email protected]>
+
+ Regression(PSON): Back/forward navigation may hang
+ https://bugs.webkit.org/show_bug.cgi?id=216611
+ <rdar://problem/68992714>
+
+ Reviewed by Geoffrey Garen.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::loadItem):
+ Add assertion that would have hit when reproducing the bug. A crash in debug is better than
+ hanging.
+
2020-10-29 Jiewen Tan <[email protected]>
[WebAuthn] Make WebContent process talk to the WebAuthn process for WebAuthn requests
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (269169 => 269170)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2020-10-29 21:35:14 UTC (rev 269170)
@@ -3832,6 +3832,9 @@
HistoryItem* currentItem = history().currentItem();
bool sameDocumentNavigation = currentItem && item.shouldDoSameDocumentNavigationTo(*currentItem);
+ // If we're continuing this history navigation in a new process, then doing a same document navigation never makes sense.
+ ASSERT(!sameDocumentNavigation || shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::No);
+
if (sameDocumentNavigation)
loadSameDocumentItem(item);
else
Modified: trunk/Source/WebKit/ChangeLog (269169 => 269170)
--- trunk/Source/WebKit/ChangeLog 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Source/WebKit/ChangeLog 2020-10-29 21:35:14 UTC (rev 269170)
@@ -1,3 +1,55 @@
+2020-10-29 Chris Dumez <[email protected]>
+
+ Regression(PSON): Back/forward navigation may hang
+ https://bugs.webkit.org/show_bug.cgi?id=216611
+ <rdar://problem/68992714>
+
+ Reviewed by Geoffrey Garen.
+
+ The scenario was as follows:
+ 1. Load A in Process P1
+ 2. Load A#foo in Process P1 (fragment navigation)
+ 3. Load B in Process P2
+ 4. history.go(-2) to load A again in P1
+ 5. history.go(2) to load B again in P2
+ 6. history.back() to go back to A#foo in P1
+ -> Hang
+
+ We doing the process swap at step 3, we would suspend the page containing A#foo
+ in P1 when doing the new load in P2. When process swapping again at step 4,
+ we would go back to process P1 (because it is same origin) but we did not have
+ any suspended WebPage for A so we would create a new WebPage (with a new
+ WebPageIdentifier). The reason we do this is to allow the suspended WebPage
+ for A#foo to remain in process, as we may want it for another back/forward
+ navigation later.
+
+ The issue was that when constructing the new WebPage in P1 for A at step 4,
+ we would restore all history items from the WebPageProxy's BackForwardList
+ and overwrite any existing HistoryItems with the same IDs in the WebProcess.
+ This meant that WebKit layer would construct new HistoryItems for
+ HistoryItems that already existing and are used by the Suspended WebPage
+ containing A#foo.
+
+ Later on, at step 6, when trying to go back A#foo in P1, we would ask
+ the Suspended WebPage in P1 to load a given HistoryItem. This HistoryItem
+ is supposed to be present in the Page's back/forward list and is supposed
+ to have an associated back/forward cache entry. However, it was not the
+ case here because we ended up with a new HistoryItem instance that was
+ restored from the UIProcess earlier.
+
+ To address the issue, The WebPage constructor no longer overwrites
+ existing HistoryItems. Note that before prior to r231048, the WebPage
+ constructor was NOT overwriting existing HistoryItems so this restores
+ previous behavior. Presumably, we started overwriting to stop hitting
+ the assertion in WebBackForwardListProxy::addItemFromUIProcess() that
+ checked that we either wanted to overwrite or that there was no
+ existing HistoryItem with the given ID. This is no longer an issue
+ since I replaced this assertion with an if-check and an early return.
+
+ * WebProcess/WebPage/WebBackForwardListProxy.cpp:
+ (WebKit::WebBackForwardListProxy::addItemFromUIProcess):
+ * WebProcess/WebPage/WebPage.cpp:
+
2020-10-29 Jiewen Tan <[email protected]>
[WebAuthn] Make WebContent process talk to the WebAuthn process for WebAuthn requests
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp (269169 => 269170)
--- trunk/Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp 2020-10-29 21:35:14 UTC (rev 269170)
@@ -57,8 +57,9 @@
void WebBackForwardListProxy::addItemFromUIProcess(const BackForwardItemIdentifier& itemID, Ref<HistoryItem>&& item, PageIdentifier pageID, OverwriteExistingItem overwriteExistingItem)
{
- // This item/itemID pair should not already exist in our map.
- ASSERT_UNUSED(overwriteExistingItem, overwriteExistingItem == OverwriteExistingItem::Yes || !idToHistoryItemMap().contains(itemID));
+ if (overwriteExistingItem == OverwriteExistingItem::No && idToHistoryItemMap().contains(itemID))
+ return;
+
idToHistoryItemMap().set(itemID, item.ptr());
clearCachedListCounts();
}
Modified: trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp (269169 => 269170)
--- trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp 2020-10-29 21:35:14 UTC (rev 269170)
@@ -683,8 +683,11 @@
m_userAgent = parameters.userAgent;
+ // Do not overwrite existing items. Due to process swapping and back/forward cache support, there may be other
+ // (suspended) WebPages in this process for the same WebPageProxy in the UIProcess. Overwriting the HistoryItems
+ // would break back/forward cache for those other pages since the HistoryItems hold the CachedPage.
if (!parameters.itemStates.isEmpty())
- restoreSessionInternal(parameters.itemStates, parameters.itemStatesWereRestoredByAPIRequest ? WasRestoredByAPIRequest::Yes : WasRestoredByAPIRequest::No, WebBackForwardListProxy::OverwriteExistingItem::Yes);
+ restoreSessionInternal(parameters.itemStates, parameters.itemStatesWereRestoredByAPIRequest ? WasRestoredByAPIRequest::Yes : WasRestoredByAPIRequest::No, WebBackForwardListProxy::OverwriteExistingItem::No);
m_drawingArea->enablePainting();
Modified: trunk/Tools/ChangeLog (269169 => 269170)
--- trunk/Tools/ChangeLog 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Tools/ChangeLog 2020-10-29 21:35:14 UTC (rev 269170)
@@ -1,3 +1,15 @@
+2020-10-29 Chris Dumez <[email protected]>
+
+ Regression(PSON): Back/forward navigation may hang
+ https://bugs.webkit.org/show_bug.cgi?id=216611
+ <rdar://problem/68992714>
+
+ Reviewed by Geoffrey Garen.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2020-10-29 Sihui Liu <[email protected]>
Unreviewed, reverting r268192.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (269169 => 269170)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2020-10-29 21:22:41 UTC (rev 269169)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2020-10-29 21:35:14 UTC (rev 269170)
@@ -131,6 +131,15 @@
done = true;
}
+- (void)_webView:(WKWebView *)webView navigation:(WKNavigation *)navigation didSameDocumentNavigation:(_WKSameDocumentNavigationType)navigationType
+{
+ if (navigationType != _WKSameDocumentNavigationTypeAnchorNavigation)
+ return;
+
+ seenPIDs.add([webView _webProcessIdentifier]);
+ done = true;
+}
+
- (void)webView:(WKWebView *)webView didStartProvisionalNavigation:(null_unspecified WKNavigation *)navigation
{
didStartProvisionalLoad = true;
@@ -3341,6 +3350,80 @@
EXPECT_EQ(2u, seenPIDs.size());
}
+TEST(ProcessSwap, BackForwardCacheSkipBackForwardListItem)
+{
+ auto processPoolConfiguration = psonProcessPoolConfiguration();
+ auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+ auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ [webViewConfiguration setProcessPool:processPool.get()];
+ auto handler = adoptNS([[PSONScheme alloc] init]);
+ [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:pageCache1Bytes];
+ [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:pageCache1Bytes];
+ [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+ auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
+ [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"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()];
+
+ NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+
+ [webView loadRequest:request];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto webkitPID = [webView _webProcessIdentifier];
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html#foo"]];
+
+ [webView loadRequest:request];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+ EXPECT_EQ(1U, [[[webView backForwardList] backList] count]);
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+
+ [webView loadRequest:request];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto applePID = [webView _webProcessIdentifier];
+ EXPECT_NE(webkitPID, applePID);
+ EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
+
+ // Go 2 items back.
+ EXPECT_EQ(2U, [[[webView backForwardList] backList] count]);
+ [webView goToBackForwardListItem:[[webView backForwardList] itemAtIndex:-2]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+ EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html", [[webView URL] absoluteString]);
+
+ // Go 2 items forward.
+ EXPECT_EQ(2U, [[[webView backForwardList] forwardList] count]);
+ [webView goToBackForwardListItem:[[webView backForwardList] itemAtIndex:2]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
+ EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
+
+ // Go back.
+ EXPECT_EQ(2U, [[[webView backForwardList] backList] count]);
+ [webView goBack];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+ EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html#foo", [[webView URL] absoluteString]);
+}
+
TEST(ProcessSwap, ClearWebsiteDataWithSuspendedPage)
{
auto processPoolConfiguration = psonProcessPoolConfiguration();
