Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable COI for desktop with credentialless #186614

Open
jrieken opened this issue Jun 29, 2023 · 3 comments · Fixed by #186720
Open

Enable COI for desktop with credentialless #186614

jrieken opened this issue Jun 29, 2023 · 3 comments · Fixed by #186720
Assignees
Labels
debt Code quality issues electron Issues and items related to Electron engineering VS Code - Build / issue tracking / etc. feature-request Request for new features or functionality workbench-electron Electron-VS Code issues

Comments

@jrieken
Copy link
Member

jrieken commented Jun 29, 2023

For desktop we can enable COI with COEP: credentialless. That's possible because chromium supports this new/proposed header value which relaxed loading cross-origin resources lacking the CORP header.

@jrieken jrieken self-assigned this Jun 29, 2023
@jrieken jrieken added the engineering VS Code - Build / issue tracking / etc. label Jun 29, 2023
@jrieken jrieken added this to the July 2023 milestone Jun 29, 2023
@jrieken jrieken added debt Code quality issues feature-request Request for new features or functionality electron Issues and items related to Electron web Issues related to running VSCode in the web workbench-electron Electron-VS Code issues and removed web Issues related to running VSCode in the web labels Jun 29, 2023
jrieken added a commit that referenced this issue Jun 30, 2023
…e flag from "enable-coi" to "disable-coi"

fixes #186614
jrieken added a commit that referenced this issue Jun 30, 2023
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Jun 30, 2023
@jrieken jrieken reopened this Jul 5, 2023
@jrieken
Copy link
Member Author

jrieken commented Jul 5, 2023

Reverted, reopened because this has very likely caused #187030

@VSCodeTriageBot VSCodeTriageBot removed the unreleased Patch has not yet been released in VS Code Insiders label Jul 5, 2023
@jrieken
Copy link
Member Author

jrieken commented Jul 10, 2023

Enabling COI caused a perf regression (#187030) and therefore we are holding back. @deepak1556 Any idea why enabling COI would cause this?

@jrieken jrieken modified the milestones: July 2023, August 2023 Jul 18, 2023
@jrieken jrieken modified the milestones: August 2023, September 2023 Aug 18, 2023
deepak1556 pushed a commit that referenced this issue Sep 6, 2023
@deepak1556
Copy link
Collaborator

Looked into this today and I have an answer for the slowdown,

In all cases, the first navigation to an origin happens from about:blank. So chromium creates a render process for the null origin and if the destination origin in our case vscode-file://vscode-app can commit in this process (always the case for first navigation) then the process will be reused without any browserinstance swap. This is what we see when coi is disabled,

BrowserInstance Swap result with COI disabled:
COI_disabled_swap_result

Process list result with COI disabled:
Process_list_with_COI_disabled

When coi is enabled on an origin, chromium has logic (added in https://chromium-review.googlesource.com/c/chromium/src/+/3945190) to always force swap the browserinstance which leads to creation of new render process. This seems to apply to even when navigating from the null origin. Following screenshots show that there is a force swap on the browserinstance and there are two renderer process, pid 17824 from the null origin and pid 14756 for the vscode-file://vscode-app origin.

BrowserInstance Swap result with COI enabled:
swap_result_coi_enabled

Process list result with COI enabled:
process_list_coi_enabled

RenderFrameHostImpl::DidCommitProvisionalLoad for COI enabled
didcommit_coi_enabled

It looks like the site instances now have a new lock attribute called process_lock which is {} for normal navigations, but ends up being { vscode-file://vscode-app/ cross-origin-isolated coi-origin='vscode-file://vscode-app' } in the case of coi origin, so a coi origin does not get committed to a process that previously loaded a non coi origin.

tl:dr; there is a restart of the render process when coi is enabled and this is a penalty we will have to pay with coi origins for the initial navigation. Maybe chromium can adjust if first navigation on the app launch is to a coi origin then the process lock on null origin can be retagged and reused ? This needs discussion in upstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Code quality issues electron Issues and items related to Electron engineering VS Code - Build / issue tracking / etc. feature-request Request for new features or functionality workbench-electron Electron-VS Code issues
Projects
None yet
3 participants