- Revision
- 240561
- Author
- [email protected]
- Date
- 2019-01-28 01:40:36 -0800 (Mon, 28 Jan 2019)
Log Message
Cherry-pick r240442. rdar://problem/47586826
Regression(PSON) Back/Forward list items' URL sometimes gets replaced with the URL of a subframe
https://bugs.webkit.org/show_bug.cgi?id=193761
<rdar://problem/47456405>
Reviewed by Alex Christensen.
Source/WebKit:
When doing a history navigation cross-process, the UIProcess would first send the back/forward list items
to the destination WebProcess via WebPage::updateBackForwardListForReattach(), then ask the process to
navigate to the expected back/forward list item.
WebPage::updateBackForwardListForReattach() would call restoreSessionInternal(), which would call
toHistoryItem() on each BackForwardListItem. This may end up creating more than one HistoryItem for each
BackForwardListItem because HistoryItems are part of a tree (each frame has its own list of history items).
Note that BackForwardListItems and HistoryItem share a BackForwardItemIdentifier which is a
(processIdentifier, itemIdentifier) pair. We normally generate the HistoryItem's identifier from inside
its constructor like so:
`{ Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() }`
However, when calling updateBackForwardListForReattach() and constructing children HistoryItem,
applyFrameState() would generate the identifier by itself and passing it to the HistoryItem constructor.
Its genegates the ID the exact same way so this would in theory not be a problem. Unfortunately, both
calls to generateObjectIdentifier() get inlined and both call sites end up with their own static counter
to generate ids. As a result, we may end up with conflicts as HistoryItems for child frames (restored
by restoreSessionInternal()) can end up with the same identifier as HistoryItems for top frames.
This confusion would lead to the WebContent process sending the UIProcess bad information and the URL
of subframes could end up as the WebBackForwardListItem's mainframe URL.
* WebProcess/WebCoreSupport/SessionStateConversion.cpp:
(WebKit::applyFrameState):
Stop calling generateObjectIdentifier() explicitly and let the HistoryItem constructor take care of it.
Calling generateObjectIdentifier() for the same type from different places is not safe due to inlining.
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240442 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Diff
Modified: branches/safari-607-branch/Source/WebKit/ChangeLog (240560 => 240561)
--- branches/safari-607-branch/Source/WebKit/ChangeLog 2019-01-28 09:40:33 UTC (rev 240560)
+++ branches/safari-607-branch/Source/WebKit/ChangeLog 2019-01-28 09:40:36 UTC (rev 240561)
@@ -1,3 +1,87 @@
+2019-01-28 Babak Shafiei <[email protected]>
+
+ Cherry-pick r240442. rdar://problem/47586826
+
+ Regression(PSON) Back/Forward list items' URL sometimes gets replaced with the URL of a subframe
+ https://bugs.webkit.org/show_bug.cgi?id=193761
+ <rdar://problem/47456405>
+
+ Reviewed by Alex Christensen.
+
+ Source/WebKit:
+
+ When doing a history navigation cross-process, the UIProcess would first send the back/forward list items
+ to the destination WebProcess via WebPage::updateBackForwardListForReattach(), then ask the process to
+ navigate to the expected back/forward list item.
+
+ WebPage::updateBackForwardListForReattach() would call restoreSessionInternal(), which would call
+ toHistoryItem() on each BackForwardListItem. This may end up creating more than one HistoryItem for each
+ BackForwardListItem because HistoryItems are part of a tree (each frame has its own list of history items).
+
+ Note that BackForwardListItems and HistoryItem share a BackForwardItemIdentifier which is a
+ (processIdentifier, itemIdentifier) pair. We normally generate the HistoryItem's identifier from inside
+ its constructor like so:
+ `{ Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() }`
+
+ However, when calling updateBackForwardListForReattach() and constructing children HistoryItem,
+ applyFrameState() would generate the identifier by itself and passing it to the HistoryItem constructor.
+ Its genegates the ID the exact same way so this would in theory not be a problem. Unfortunately, both
+ calls to generateObjectIdentifier() get inlined and both call sites end up with their own static counter
+ to generate ids. As a result, we may end up with conflicts as HistoryItems for child frames (restored
+ by restoreSessionInternal()) can end up with the same identifier as HistoryItems for top frames.
+
+ This confusion would lead to the WebContent process sending the UIProcess bad information and the URL
+ of subframes could end up as the WebBackForwardListItem's mainframe URL.
+
+ * WebProcess/WebCoreSupport/SessionStateConversion.cpp:
+ (WebKit::applyFrameState):
+ Stop calling generateObjectIdentifier() explicitly and let the HistoryItem constructor take care of it.
+ Calling generateObjectIdentifier() for the same type from different places is not safe due to inlining.
+
+ Tools:
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240442 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-01-24 Chris Dumez <[email protected]>
+
+ Regression(PSON) Back/Forward list items' URL sometimes gets replaced with the URL of a subframe
+ https://bugs.webkit.org/show_bug.cgi?id=193761
+ <rdar://problem/47456405>
+
+ Reviewed by Alex Christensen.
+
+ When doing a history navigation cross-process, the UIProcess would first send the back/forward list items
+ to the destination WebProcess via WebPage::updateBackForwardListForReattach(), then ask the process to
+ navigate to the expected back/forward list item.
+
+ WebPage::updateBackForwardListForReattach() would call restoreSessionInternal(), which would call
+ toHistoryItem() on each BackForwardListItem. This may end up creating more than one HistoryItem for each
+ BackForwardListItem because HistoryItems are part of a tree (each frame has its own list of history items).
+
+ Note that BackForwardListItems and HistoryItem share a BackForwardItemIdentifier which is a
+ (processIdentifier, itemIdentifier) pair. We normally generate the HistoryItem's identifier from inside
+ its constructor like so:
+ `{ Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() }`
+
+ However, when calling updateBackForwardListForReattach() and constructing children HistoryItem,
+ applyFrameState() would generate the identifier by itself and passing it to the HistoryItem constructor.
+ Its genegates the ID the exact same way so this would in theory not be a problem. Unfortunately, both
+ calls to generateObjectIdentifier() get inlined and both call sites end up with their own static counter
+ to generate ids. As a result, we may end up with conflicts as HistoryItems for child frames (restored
+ by restoreSessionInternal()) can end up with the same identifier as HistoryItems for top frames.
+
+ This confusion would lead to the WebContent process sending the UIProcess bad information and the URL
+ of subframes could end up as the WebBackForwardListItem's mainframe URL.
+
+ * WebProcess/WebCoreSupport/SessionStateConversion.cpp:
+ (WebKit::applyFrameState):
+ Stop calling generateObjectIdentifier() explicitly and let the HistoryItem constructor take care of it.
+ Calling generateObjectIdentifier() for the same type from different places is not safe due to inlining.
+
2019-01-24 Alan Coon <[email protected]>
Cherry-pick r240461. rdar://problem/47536283
Modified: branches/safari-607-branch/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp (240560 => 240561)
--- branches/safari-607-branch/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp 2019-01-28 09:40:33 UTC (rev 240560)
+++ branches/safari-607-branch/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp 2019-01-28 09:40:36 UTC (rev 240561)
@@ -178,7 +178,7 @@
#endif
for (const auto& childFrameState : frameState.children) {
- Ref<HistoryItem> childHistoryItem = HistoryItem::create(childFrameState.urlString, { }, { }, { Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() });
+ Ref<HistoryItem> childHistoryItem = HistoryItem::create(childFrameState.urlString, { }, { });
applyFrameState(childHistoryItem, childFrameState);
historyItem.addChildItem(WTFMove(childHistoryItem));
Modified: branches/safari-607-branch/Tools/ChangeLog (240560 => 240561)
--- branches/safari-607-branch/Tools/ChangeLog 2019-01-28 09:40:33 UTC (rev 240560)
+++ branches/safari-607-branch/Tools/ChangeLog 2019-01-28 09:40:36 UTC (rev 240561)
@@ -1,3 +1,63 @@
+2019-01-28 Babak Shafiei <[email protected]>
+
+ Cherry-pick r240442. rdar://problem/47586826
+
+ Regression(PSON) Back/Forward list items' URL sometimes gets replaced with the URL of a subframe
+ https://bugs.webkit.org/show_bug.cgi?id=193761
+ <rdar://problem/47456405>
+
+ Reviewed by Alex Christensen.
+
+ Source/WebKit:
+
+ When doing a history navigation cross-process, the UIProcess would first send the back/forward list items
+ to the destination WebProcess via WebPage::updateBackForwardListForReattach(), then ask the process to
+ navigate to the expected back/forward list item.
+
+ WebPage::updateBackForwardListForReattach() would call restoreSessionInternal(), which would call
+ toHistoryItem() on each BackForwardListItem. This may end up creating more than one HistoryItem for each
+ BackForwardListItem because HistoryItems are part of a tree (each frame has its own list of history items).
+
+ Note that BackForwardListItems and HistoryItem share a BackForwardItemIdentifier which is a
+ (processIdentifier, itemIdentifier) pair. We normally generate the HistoryItem's identifier from inside
+ its constructor like so:
+ `{ Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() }`
+
+ However, when calling updateBackForwardListForReattach() and constructing children HistoryItem,
+ applyFrameState() would generate the identifier by itself and passing it to the HistoryItem constructor.
+ Its genegates the ID the exact same way so this would in theory not be a problem. Unfortunately, both
+ calls to generateObjectIdentifier() get inlined and both call sites end up with their own static counter
+ to generate ids. As a result, we may end up with conflicts as HistoryItems for child frames (restored
+ by restoreSessionInternal()) can end up with the same identifier as HistoryItems for top frames.
+
+ This confusion would lead to the WebContent process sending the UIProcess bad information and the URL
+ of subframes could end up as the WebBackForwardListItem's mainframe URL.
+
+ * WebProcess/WebCoreSupport/SessionStateConversion.cpp:
+ (WebKit::applyFrameState):
+ Stop calling generateObjectIdentifier() explicitly and let the HistoryItem constructor take care of it.
+ Calling generateObjectIdentifier() for the same type from different places is not safe due to inlining.
+
+ Tools:
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240442 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-01-24 Chris Dumez <[email protected]>
+
+ Regression(PSON) Back/Forward list items' URL sometimes gets replaced with the URL of a subframe
+ https://bugs.webkit.org/show_bug.cgi?id=193761
+ <rdar://problem/47456405>
+
+ Reviewed by Alex Christensen.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2019-01-23 Alan Coon <[email protected]>
Cherry-pick r240178. rdar://problem/47494727
Modified: branches/safari-607-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (240560 => 240561)
--- branches/safari-607-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-28 09:40:33 UTC (rev 240560)
+++ branches/safari-607-branch/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-28 09:40:36 UTC (rev 240561)
@@ -2063,6 +2063,79 @@
EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
}
+static const char* withSubframesTestBytes = R"PSONRESOURCE(
+<body>
+<iframe src=""
+<iframe src=""
+</body>
+)PSONRESOURCE";
+
+TEST(ProcessSwap, HistoryItemIDConfusion)
+{
+ auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+ [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+ 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:failsToEnterPageCacheTestBytes];
+ [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:withSubframesTestBytes];
+ [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()];
+
+ 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.apple.com/main.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto applePID = [webView _webProcessIdentifier];
+ EXPECT_NE(webkitPID, applePID);
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.google.com/main.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto googlePID = [webView _webProcessIdentifier];
+ EXPECT_NE(applePID, googlePID);
+ EXPECT_NE(webkitPID, googlePID);
+
+ [webView goBack];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
+
+ [webView goBack];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+ auto* backForwardList = [webView backForwardList];
+ EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html", [backForwardList.currentItem.URL absoluteString]);
+ EXPECT_EQ(2U, backForwardList.forwardList.count);
+ EXPECT_EQ(0U, backForwardList.backList.count);
+ EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [backForwardList.forwardList[0].URL absoluteString]);
+ EXPECT_WK_STREQ(@"pson://www.google.com/main.html", [backForwardList.forwardList[1].URL absoluteString]);
+}
+
static const char* mainFramesOnlyMainFrame = R"PSONRESOURCE(
<script>
function loaded() {
