Bug 302678 – [Site Isolation] Redundant call to WebFrameProxy::didStartProvisionalLoad when browsing context switch happens after window.open

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Process Model

[Anthony Tarbinian]

Summary:
With site isolation, there is a case where `WebFrameProxy::didStartProvisionalLoad` can be called twice when a window is opened. This is an issue because calling `WebFrameProxy::didStartProvisionalLoad` twice with the same URL will hit this ASSERT in `FrameLoadState::didStartProvisionalLoad` https://github.com/WebKit/WebKit/blob/3db54ba8bc59c2b0226d5400edb63a60a2b86a4e/Source/WebKit/UIProcess/FrameLoadState.cpp#L49.


Steps To Reproduce:
Run `LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/javascript-url.https.html` with WebKitTestRunner and site isolation on. The crash only happens when a browsing context switch happens (I’ve been mainly looking at test case 3 and 4 in that file).
Reproduction command:
```
Tools/Scripts/run-webkit-tests --debug --additional-header=SiteIsolationEnabled=true LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/javascript-url.https.html
```

The w3c test uses the fact that `javascript:` urls inherit the current document’s COOP.

Alternatively, I will attach a reduced version to this Radar’s discussion.

I’ve hit this when a window is opened with a different Cross-Origin-Opener-Policy (COOP) https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Cross-Origin-Opener-Policy. This triggers a new browsing context group change which creates a new process as well (`m_isProcessSwappingForNewWindow` is true in the `ProvisionalPageProxy` constructor). This means `m_mainFrame` is set to the page’s previous mainFrame (see the `ProvisionalPageProxy` constructor https://github.com/WebKit/WebKit/blob/5d6036bf8f1240b2c2545e0a86e0af52913416f1/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp#L124-L151). Finally, `didStartProvisionalLoad` is called on `m_mainFrame`. However, remember that `m_mainFrame` was just set to the previous frame.

This means that when `window.open` is called `didStartProvisionalLoad` is called the first time (from the loading code). When the COOP headers are parsed and a mismatch is detected (i.e. the opener has `unsafe-none` and the child has `same-origin-allow-popups`), it will call `triggerBrowsingContextGroupSwitchForNavigation` which will in turn call `continueNavigationInNewProcess` and create a `ProvisionalPageProxy`. Then in the `ProvisionalPageProxy` constructor, the logic leads to `WebFrameProxy::didStartProvisionalLoad` being called a second time for the same frame.


Results:
Here is a stack trace when hitting the ASSERT:
```
ASSERTION FAILED: m_provisionalURL.isEmpty()
/Users/atarbinian/Dev/Safari3/OpenSource/Source/WebKit/UIProcess/FrameLoadState.cpp(50) : void WebKit::FrameLoadState::didStartProvisionalLoad(URL &&)
1   0x12dd17e28 WebKit::FrameLoadState::didStartProvisionalLoad(WTF::URL&&)
2   0x12ddda684 WebKit::WebFrameProxy::didStartProvisionalLoad(WTF::URL&&)
3   0x12dd3c01c WebKit::ProvisionalPageProxy::ProvisionalPageProxy(WebKit::WebPageProxy&, WTF::Ref<WebKit::FrameProcess, WTF::RawPtrTraits<WebKit::FrameProcess>, WTF::DefaultRefDerefTraits<WebKit::FrameProcess>>&&, WebKit::BrowsingContextGroup&, WTF::RefPtr<WebKit::SuspendedPageProxy, WTF::RawPtrTraits<WebKit::SuspendedPageProxy>, WTF::DefaultRefDerefTraits<WebKit::SuspendedPageProxy>>&&, API::Navigation&, bool, WebCore::ResourceRequest const&, WebKit::ProcessSwapRequestedByClient, bool, API::WebsitePolicies*, WebKit::WebsiteDataStore*)
4   0x12dd3cfec WebKit::ProvisionalPageProxy::ProvisionalPageProxy(WebKit::WebPageProxy&, WTF::Ref<WebKit::FrameProcess, WTF::RawPtrTraits<WebKit::FrameProcess>, WTF::DefaultRefDerefTraits<WebKit::FrameProcess>>&&, WebKit::BrowsingContextGroup&, WTF::RefPtr<WebKit::SuspendedPageProxy, WTF::RawPtrTraits<WebKit::SuspendedPageProxy>, WTF::DefaultRefDerefTraits<WebKit::SuspendedPageProxy>>&&, API::Navigation&, bool, WebCore::ResourceRequest const&, WebKit::ProcessSwapRequestedByClient, bool, API::WebsitePolicies*, WebKit::WebsiteDataStore*)
5   0x12de47a5c WTF::Ref<WebKit::ProvisionalPageProxy, WTF::RawPtrTraits<WebKit::ProvisionalPageProxy>, WTF::DefaultRefDerefTraits<WebKit::ProvisionalPageProxy>> WebKit::ProvisionalPageProxy::create<WebKit::WebPageProxy&, WTF::Ref<WebKit::FrameProcess, WTF::RawPtrTraits<WebKit::FrameProcess>, WTF::DefaultRefDerefTraits<WebKit::FrameProcess>>, WebKit::BrowsingContextGroup&, WTF::RefPtr<WebKit::SuspendedPageProxy, WTF::RawPtrTraits<WebKit::SuspendedPageProxy>, WTF::DefaultRefDerefTraits<WebKit::SuspendedPageProxy>>, API::Navigation&, bool&, WebCore::ResourceRequest const&, WebKit::ProcessSwapRequestedByClient&, bool&, API::WebsitePolicies*, WebKit::WebsiteDataStore*&>(WebKit::WebPageProxy&, WTF::Ref<WebKit::FrameProcess, WTF::RawPtrTraits<WebKit::FrameProcess>, WTF::DefaultRefDerefTraits<WebKit::FrameProcess>>&&, WebKit::BrowsingContextGroup&, WTF::RefPtr<WebKit::SuspendedPageProxy, WTF::RawPtrTraits<WebKit::SuspendedPageProxy>, WTF::DefaultRefDerefTraits<WebKit::SuspendedPageProxy>>&&, API::Navigation&, bool&, WebCore::ResourceRequest const&, WebKit::ProcessSwapRequestedByClient&, bool&, API::WebsitePolicies*&&, WebKit::WebsiteDataStore*&)
6   0x12de475f8 WebKit::WebPageProxy::continueNavigationInNewProcess(API::Navigation&, WebKit::WebFrameProxy&, WTF::RefPtr<WebKit::SuspendedPageProxy, WTF::RawPtrTraits<WebKit::SuspendedPageProxy>, WTF::DefaultRefDerefTraits<WebKit::SuspendedPageProxy>>&&, WebKit::BrowsingContextGroup&, WTF::Ref<WebKit::WebProcessProxy, WTF::RawPtrTraits<WebKit::WebProcessProxy>, WTF::DefaultRefDerefTraits<WebKit::WebProcessProxy>>&&, WebKit::ProcessSwapRequestedByClient, WebCore::ShouldTreatAsContinuingLoad, std::__1::optional<WTF::ObjectIdentifierGeneric<WebKit::NetworkResourceLoadIdentifierType, WTF::ObjectIdentifierMainThreadAccessTraits<unsigned long long>, unsigned long long>>, WebKit::LoadedWebArchive, WebCore::IsPerformingHTTPFallback, WebCore::ProcessSwapDisposition, WebKit::WebsiteDataStore*)
7   0x12df5ee20 WebKit::WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation(WTF::ObjectIdentifierGeneric<WebCore::NavigationIdentifierType, WTF::ObjectIdentifierMainThreadAccessTraits<unsigned long long>, unsigned long long>, WebCore::BrowsingContextGroupSwitchDecision, WebCore::Site const&, WTF::ObjectIdentifierGeneric<WebKit::NetworkResourceLoadIdentifierType, WTF::ObjectIdentifierMainThreadAccessTraits<unsigned long long>, unsigned long long>, WTF::CompletionHandler<void (bool)>&&)::$_0::operator()()
8   0x12df5eb98 WTF::Detail::CallableWrapper<WebKit::WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation(WTF::ObjectIdentifierGeneric<WebCore::NavigationIdentifierType, WTF::ObjectIdentifierMainThreadAccessTraits<unsigned long long>, unsigned long long>, WebCore::BrowsingContextGroupSwitchDecision, WebCore::Site const&, WTF::ObjectIdentifierGeneric<WebKit::NetworkResourceLoadIdentifierType, WTF::ObjectIdentifierMainThreadAccessTraits<unsigned long long>, unsigned long long>, WTF::CompletionHandler<void (bool)>&&)::$_0, void>::call()
9   0x12a7ce400 WTF::Function<void ()>::operator()() const
10  0x12a848078 WTF::CompletionHandler<void ()>::operator()()
11  0x12cf85c40 decltype(std::declval<WTF::CompletionHandler<void ()>>()()) std::__1::__invoke[abi:sn200100]<WTF::CompletionHandler<void ()>>(WTF::CompletionHandler<void ()>&&)
12  0x12cfbd1a8 decltype(auto) std::__1::__apply_tuple_impl[abi:sn200100]<WTF::CompletionHandler<void ()>, std::__1::tuple<>>(WTF::CompletionHandler<void ()>&&, std::__1::tuple<>&&, std::__1::__tuple_indices<>)
13  0x12cfbd17c decltype(auto) std::__1::apply[abi:sn200100]<WTF::CompletionHandler<void ()>, std::__1::tuple<>>(WTF::CompletionHandler<void ()>&&, std::__1::tuple<>&&)
14  0x12e34d9c4 void IPC::Connection::callReply<Messages::NetworkProcess::AddAllowedFirstPartyForCookies, WTF::CompletionHandler<void ()>>(IPC::Connection*, IPC::Decoder&, WTF::CompletionHandler<void ()>&&)
15  0x12e34d8e8 WTF::CompletionHandler<void (IPC::Connection*, IPC::Decoder*)> IPC::Connection::makeAsyncReplyCompletionHandler<Messages::NetworkProcess::AddAllowedFirstPartyForCookies, WTF::CompletionHandler<void ()>>(WTF::CompletionHandler<void ()>&&, WTF::ThreadLikeAssertion)::'lambda'(IPC::Connection*, IPC::Decoder*)::operator()(IPC::Connection*, IPC::Decoder*)
16  0x12e34d790 WTF::Detail::CallableWrapper<WTF::CompletionHandler<void (IPC::Connection*, IPC::Decoder*)> IPC::Connection::makeAsyncReplyCompletionHandler<Messages::NetworkProcess::AddAllowedFirstPartyForCookies, WTF::CompletionHandler<void ()>>(WTF::CompletionHandler<void ()>&&, WTF::ThreadLikeAssertion)::'lambda'(IPC::Connection*, IPC::Decoder*), void, IPC::Connection*, IPC::Decoder*>::call(IPC::Connection*, IPC::Decoder*)
17  0x12d5359f0 WTF::Function<void (IPC::Connection*, IPC::Decoder*)>::operator()(IPC::Connection*, IPC::Decoder*) const
18  0x12d517800 WTF::CompletionHandler<void (IPC::Connection*, IPC::Decoder*)>::operator()(IPC::Connection*, IPC::Decoder*)
19  0x12dced4f4 WebKit::AuxiliaryProcessProxy::sendMessage(WTF::UniqueRef<IPC::Encoder>&&, WTF::OptionSet<IPC::SendOption, (WTF::ConcurrencyTag)0>, std::__1::optional<IPC::ConnectionAsyncReplyHandler>, WebKit::AuxiliaryProcessProxy::ShouldStartProcessThrottlerActivity)::$_1::operator()(IPC::Connection*, IPC::Decoder*)
20  0x12dced328 WTF::Detail::CallableWrapper<WebKit::AuxiliaryProcessProxy::sendMessage(WTF::UniqueRef<IPC::Encoder>&&, WTF::OptionSet<IPC::SendOption, (WTF::ConcurrencyTag)0>, std::__1::optional<IPC::ConnectionAsyncReplyHandler>, WebKit::AuxiliaryProcessProxy::ShouldStartProcessThrottlerActivity)::$_1, void, IPC::Connection*, IPC::Decoder*>::call(IPC::Connection*, IPC::Decoder*)
21  0x12d5359f0 WTF::Function<void (IPC::Connection*, IPC::Decoder*)>::operator()(IPC::Connection*, IPC::Decoder*) const
22  0x12d517800 WTF::CompletionHandler<void (IPC::Connection*, IPC::Decoder*)>::operator()(IPC::Connection*, IPC::Decoder*)
23  0x12f34600c IPC::Connection::dispatchMessage(IPC::Decoder&)
24  0x12f346650 IPC::Connection::dispatchMessage(WTF::UniqueRef<IPC::Decoder>)
25  0x12f3469c4 IPC::Connection::dispatchOneIncomingMessage()
26  0x12f3a1998 IPC::Connection::enqueueIncomingMessage(WTF::UniqueRef<IPC::Decoder>)::$_1::operator()() const
27  0x12f3a18d8 WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(WTF::UniqueRef<IPC::Decoder>)::$_1, void>::call()
28  0x10ccab2dc WTF::Function<void ()>::operator()() const
29  0x10cd87cd8 WTF::RunLoop::performWork()
30  0x10cd8fc10 WTF::RunLoop::performWork(void*)
31  0x1886c89e8 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
```

[Anthony Tarbinian]

rdar://164680770

[Anthony Tarbinian]

Pull request: https://github.com/WebKit/WebKit/pull/54084

[EWS]

Committed 303420@main (24039354222e): <https://commits.webkit.org/303420@main>

Reviewed commits have been landed. Closing PR #54084 and removing active labels.