Skip to content

fix(webkit): support Cross-Origin-Opener-Policy navigation#9120

Closed
yury-s wants to merge 11 commits into
microsoft:masterfrom
yury-s:wk-coop-navigation
Closed

fix(webkit): support Cross-Origin-Opener-Policy navigation#9120
yury-s wants to merge 11 commits into
microsoft:masterfrom
yury-s:wk-coop-navigation

Conversation

@yury-s
Copy link
Copy Markdown
Member

@yury-s yury-s commented Sep 24, 2021

WebKit recently enabled support for Cross-Origin-Opener-Policy / Cross-Origin-Embedder-Policy and decision about whether navigation should happen in a new process now made twice:

  • navigation policy decision
  • response policy decision

In the latter case the requestWillBeSent and responseReceivedEvents are fired in the original process and then (if process should be swapped) fired again in the new process (with new request id but same loaderId). Moreover, after the response policy decision the old process will send Page.frameStoppedLoading and Network.loadingFailed(canceled) events. We now ignore those events if there is ongoing navigation in a provisional page.

Fixes #9065

@yury-s yury-s added the CQ1 label Sep 24, 2021
shouldIgnoreFrameStoppedLoading(frameId: string): boolean {
if (frameId !== this._frameId)
return false;
// Navigation in the original frame is canceled and actually continues in the
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid this might not be always the case. For example, we navigate to cross-origin 204 page that creates a provisional page but never commits?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coincidentally that works because we generate loadingFinished event in WebKit ourselves when 204 response comes:

if (response.status() === 204) {
this._onLoadingFailed({
requestId: event.requestId,
errorText: 'Aborted: 204 No Content',
timestamp: event.timestamp
});
}
}

With network failures we will receive Playwright.provisionalLoadFailed and that will abort the navigation, so we are good in that case too. Added a test.

}

checkIfDuplicateResponseEvent(event: Protocol.Network.responseReceivedPayload): boolean {
if (this._originalRequestId === event.requestId || this._provisionalPageRequestId === event.requestId) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we ignore responses from the old web process only? This ensures that we get correct response from the new web process, in case WebKit fixes more security holes in the old web process and send bogus response from there (if any).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment response notification comes from old process we don't know if it's gonna be a cross-process or same process navigation. In case of same-process navigation we have to dispatch messages from the old process. Unless we introduce a separate notification that the navigation turned into cross-process and buffer up some messages from old web process before the notification arrives we cannot safely ignore responses from the old web process .

@mxschmitt
Copy link
Copy Markdown
Contributor

Seems not needed anymore, closing.

@mxschmitt mxschmitt closed this Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] playwright 1.15.0 causes page.goto to throw with 'Frame load interrupted' in webkit

3 participants