Attachment 359994 Details for Bug 193761 – Patch

[patch] Patch

bug-193761-20190123215443.patch (text/plain), 7.95 KB, created by Chris Dumez on 2019-01-23 21:54:45 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-23 21:54:45 PST

Size: 7.95 KB

patch

obsolete

>Subversion Revision: 240342
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index ea380fb0140d8bed0d82c11f64fc1b631cb5f3d4..b1e023de5d9bb522f9974a58f6f81ece4735a06d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,39 @@
>+2019-01-23  Chris Dumez  <cdumez@apple.com>
>+
>+        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 NOBODY (OOPS!).
>+
>+        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-23  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         Introduce UndoStep::label() and adopt it in WebKitLegacy and WebKit
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp b/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
>index 63c36a3db69a82ad3bea660f6b06dfe763c55617..8e2dc46cec1556708129d5b68d3e34de7c9074d0 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/SessionStateConversion.cpp
>@@ -178,7 +178,7 @@ static void applyFrameState(HistoryItem& historyItem, const FrameState& frameSta
> #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));
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index a0c6b0d0ed5e1ef6b9060c7b98fda543985bdd6f..e42be32a6560bcfc2fcbdb35fadd3e829920bbac 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-23  Chris Dumez  <cdumez@apple.com>
>+
>+        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 NOBODY (OOPS!).
>+
>+        Add layout test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-01-23  Jonathan Bedard  <jbedard@apple.com>
> 
>         webkitpy: Use correct config for --iphone-simulator and --ipad-simulator
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index fda46a597c8d3c03e3d04834dc31ea6250a2e230..af30aacefa776abefd21bf1f71e3f13c3456f85b 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -2063,6 +2063,79 @@ TEST(ProcessSwap, ReuseSuspendedProcessOnBackEvenIfPageCacheFails)
>     EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
> }
> 
>+static const char* withSubframesTestBytes = R"PSONRESOURCE(
>+<body>
>+<iframe src="about:blank"></iframe>
>+<iframe src="about:blank"></iframe>
>+</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() {

Flags:

achristensen: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193761: 359994