Skip to content

Commit

Permalink
Main frame URL is wrong after server-side redirect to a page serving …
Browse files Browse the repository at this point in the history
…the COOP header

https://bugs.webkit.org/show_bug.cgi?id=260046
rdar://111855179

Reviewed by Brent Fulgham and Alex Christensen.

In the poc, the page is opening a popup (without opener) to the same origin URL1.
This URL1 does a server-side redirect to URL2 which serves the `COOP: same-origin`
HTTP header. After the navigation, Safari was displaying URL1 instead of URL2 in
the URL bar.

It is important to note that that 2 process-swap occur here. The first occurs when
we do the navigation to URL1 in a popup that doesn't have an opener (in the
decidePolicyForNavigationAction). The second one occurs when we receive the
COOP header from URL2 (on navigation response).

In ProvisionalPageProxy::didCreateMainFrame(), we have code which does the following:
```
if (previousMainFrame && !previousMainFrame->provisionalURL().isEmpty()) {
        // In case of a process swap after response policy, the didStartProvisionalLoad already happened but the new main frame doesn't know about it
        // so we need to tell it so it can update its provisional URL.
        m_mainFrame->didStartProvisionalLoad(previousMainFrame->provisionalURL());
    }
```

During the second process-swap, we forward the provisional URL from the committed
frame to the provisional one. This is because the didStartProvisionalLoad IPC was
handled by the committed main frame, before we decided to process-swap on resource
response later on. As a result, the provisional main frame doesn't know yet about
the provisional load and we have to let it know about it so it sets its provisional
URL.

This worked fine in the usual case where the COOP process-swap doesn't follow
another process swap. However, in this case, the provisional URL got updated by
an earlier server side redirect which got handled by a provisional frame, not the
committed one. As a result, the committed frame didn't know about the latest
provisional URL, only the original one before the server side redirect.

To address the issue, whenever a provisional main frame receives a server-side
redirect, we now let the committed main frame know about it too so that the
committed frame's provisional URL always stays up-to-date. As a result, when
ProvisionalPageProxy::didCreateMainFrame() forwards the committed frame's URL to
the new provisional frame, it is now accurate.

* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrameShared):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Originally-landed-as: 265870.357@safari-7616-branch (8e677b3). rdar://117809466
Canonical link: https://commits.webkit.org/270140@main
  • Loading branch information
cdumez authored and robert-jenner committed Nov 2, 2023
1 parent a330ea7 commit 9507587
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 2 deletions.
8 changes: 6 additions & 2 deletions Source/WebKit/UIProcess/WebPageProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5743,9 +5743,13 @@ void WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrameShared(Ref<

auto transaction = internals().pageLoadState.transaction();

if (frame->isMainFrame())
if (frame->isMainFrame()) {
internals().pageLoadState.didReceiveServerRedirectForProvisionalLoad(transaction, request.url().string());

// If the main frame in a provisional page is getting a server-side redirect, make sure the
// committed page's provisional URL is kept up-to-date too.
if (frame != m_mainFrame && !m_mainFrame->frameLoadState().provisionalURL().isEmpty())
m_mainFrame->didReceiveServerRedirectForProvisionalLoad(request.url());
}
frame->didReceiveServerRedirectForProvisionalLoad(request.url());

internals().pageLoadState.commitChanges();
Expand Down
49 changes: 49 additions & 0 deletions Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
Original file line number Diff line number Diff line change
Expand Up @@ -7424,6 +7424,55 @@ HTTPServer server({
} while (isFrozen);
}

TEST(ProcessSwap, MainFrameURLAfterServerSideRedirectToCOOP)
{
using namespace TestWebKitAPI;
HTTPServer server({
{ "/source.html"_s, { "<a href='redirect.html' target='_blank' id='testLink'>Click here</a>"_s } },
{ "/redirect.html"_s, { 302, { { "Location"_s, "destination.html"_s } }, "redirecting..."_s } },
{ "/destination.html"_s, { { { "Content-Type"_s, "text/html"_s }, { "Cross-Origin-Opener-Policy"_s, "same-origin"_s } }, "bar"_s } },
}, HTTPServer::Protocol::Https);

auto processPoolConfiguration = psonProcessPoolConfiguration();
auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);

auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
[webViewConfiguration setProcessPool:processPool.get()];
for (_WKFeature *feature in [WKPreferences _features]) {
if ([feature.key isEqualToString:@"CrossOriginOpenerPolicyEnabled"])
[[webViewConfiguration preferences] _setEnabled:YES forFeature:feature];
else if ([feature.key isEqualToString:@"CrossOriginEmbedderPolicyEnabled"])
[[webViewConfiguration preferences] _setEnabled:YES forFeature:feature];
}

auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
[webView setNavigationDelegate:navigationDelegate.get()];
auto uiDelegate = adoptNS([[PSONUIDelegate alloc] initWithNavigationDelegate:navigationDelegate.get()]);
[webView setUIDelegate:uiDelegate.get()];

done = false;
[webView loadRequest:server.request("/source.html"_s)];
Util::run(&done);

done = false;
[webView evaluateJavaScript:@"document.getElementById('testLink').click()" completionHandler:nil];

TestWebKitAPI::Util::run(&didCreateWebView);
didCreateWebView = false;

TestWebKitAPI::Util::run(&done);

auto pid1 = [webView _webProcessIdentifier];
EXPECT_TRUE(!!pid1);
auto pid2 = [createdWebView _webProcessIdentifier];
EXPECT_TRUE(!!pid2);
EXPECT_NE(pid1, pid2);

EXPECT_WK_STREQ(server.request("/destination.html"_s).URL.absoluteString, [[createdWebView URL] absoluteString]);
EXPECT_WK_STREQ(server.request("/destination.html"_s).URL.absoluteString, [[createdWebView _mainFrameURL] absoluteString]);
}

TEST(ProcessSwap, NavigateBackAfterNavigatingAwayFromCOOP)
{
using namespace TestWebKitAPI;
Expand Down

0 comments on commit 9507587

Please sign in to comment.