Skip to content

feat: enable code cache for custom protocols - #40544

Merged
deepak1556 merged 1 commit into
mainfrom
code-cache-custom-protocol
Dec 6, 2023
Merged

deepak1556 merged 1 commit into
mainfrom
code-cache-custom-protocol

Conversation

@zcbenz

@zcbenz zcbenz commented Nov 16, 2023 •

Copy link
Copy Markdown
Contributor

Description of Change

Add an option in protocol.registerSchemesAsPrivileged to allow V8 code cache in custom schemes.

Upstream Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1500437.

/cc @deepak1556 and @bpasero since there will need changes in VS Code.

Fixes #27075

Checklist

Release Notes

Notes: Add an option in protocol.registerSchemesAsPrivileged to allow V8 code cache in custom schemes.

@zcbenz zcbenz added the semver/minor backwards-compatible functionality label Nov 16, 2023
@zcbenz
zcbenz requested a review from deepak1556 November 16, 2023 07:00
@zcbenz
zcbenz requested a review from a team as a code owner November 16, 2023 07:00
@zcbenz zcbenz added target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. labels Nov 16, 2023
@deepak1556

Copy link
Copy Markdown
Member

Should we enforce a good default be only allowing code cache for standard custom schemes, since the cache key relies on origin lock of the process when available. Custom protocols with null origin will have their code cache grouped together and I am not sure cache will be useful in that case since there is more chance of collision.

@zcbenz
zcbenz force-pushed the code-cache-custom-protocol branch from b556fc1 to 976689f Compare November 21, 2023 01:36
@zcbenz

zcbenz commented Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

Should we enforce a good default be only allowing code cache for standard custom schemes, since the cache key relies on origin lock of the process when available.

I updated the API to throw when trying to enabling code cache for non-standard scheme.

@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Nov 23, 2023
@zcbenz
zcbenz requested a review from a team November 28, 2023 00:01
@zcbenz
zcbenz force-pushed the code-cache-custom-protocol branch from 132f235 to 2aad9dd Compare November 30, 2023 23:56

@jkleinsc jkleinsc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

API LGTM

@zcbenz
zcbenz force-pushed the code-cache-custom-protocol branch from 1453c59 to 035726d Compare December 5, 2023 01:20

@samuelmaddock samuelmaddock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

API LGTM

@deepak1556
deepak1556 merged commit 9aa73ab into main Dec 6, 2023
@deepak1556
deepak1556 deleted the code-cache-custom-protocol branch December 6, 2023 02:22
@release-clerk

release-clerk Bot commented Dec 6, 2023

Copy link
Copy Markdown

Release Notes Persisted

Add an option in protocol.registerSchemesAsPrivileged to allow V8 code cache in custom schemes.

@trop

trop Bot commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "27-x-y", please check out #40708

@trop trop Bot added in-flight/27-x-y and removed target/27-x-y PR should also be added to the "27-x-y" branch. labels Dec 6, 2023
@trop

trop Bot commented Dec 6, 2023

Copy link
Copy Markdown
Contributor

I have automatically backported this PR to "28-x-y", please check out #40709

@trop trop Bot added in-flight/28-x-y and removed target/28-x-y PR should also be added to the "28-x-y" branch. labels Dec 6, 2023
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@trop trop Bot added merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. and removed in-flight/27-x-y in-flight/28-x-y labels Dec 13, 2023
nexustech added a commit to nexustech/webforge that referenced this pull request Nov 9, 2026
## Summary

Closes #893: a V8 code cache for the editor's scripts. Main's compile
cache was measured and left out, with the reasons in ADR 0023.

### What changes
- **`codeCache: true` on `webforge-app://`** (`main/app-scheme.ts`). V8
keeps the code it compiled for the editor's and Help's scripts in the
profile's `Code Cache`, and later starts use it.
- **No `module.enableCompileCache()` for main.** It was tried, and
Node's log showed every cached module accepted, but it saved 23 ms,
within noise: since #892, main loads 1.4 MB at start-up. Doing it
properly also needed a bootstrap entry, since main.ts's imports have all
loaded by its first line. Neither is in this PR.
- **The cold-start check** now records `domInteractive`, and times
`DOMContentLoaded` from it, which is the issue's measure.
- **Docs:**
- [The start-up review](docs/reviews/2026-11-09-startup-performance.md),
recommendation 2: the result.
  - ADR 0023: an amendment with the security note.

### Before and after
The cold-start check three times with each build, alternating, on the
review's laptop (Windows 11, i7-8750H), on `main` after #892, #894 and
#895. These are medians of the 12 launches after the first on each
profile, with the first launch in brackets, in ms. "With" here had both
caches; main's share is the 23 ms.

| Stage | Without | With |
| --- | --- | --- |
| Main's first mark, after the process starts | 387 (407) | 364 (454) |
| `DOMContentLoaded` − `domInteractive` | 781 (803) | 802 (1040) |
| The editor's first render, after navigation | 1922 (2033) | 1660
(2142) |
| A styled canvas, after the launch | 3211 (3380) | 3036 (3477) |

- **The renderer's cache helps later than the issue expected:** V8
compiles most functions only when they first run, so the saving shows in
React's first render (−0.26 s), not before `DOMContentLoaded`. The
issue's acceptance criterion names `DOMContentLoaded − domInteractive`,
which didn't move; the review records why.
- **The first launch on a new profile is about 0.1 s slower,** as it
builds the cache.

### Security (ADR 0023's amendment)
- **The privilege grants a page nothing.** Electron passes the scheme to
Chromium's code-cache schemes and to Blink's
`RegisterURLSchemeAsCodeCacheWithHashing`
([electron#40544](electron/electron#40544)).
- **How entries are checked:** as for Chromium's WebUI. Blink's
`ScriptCachedMetadataHandlerWithHashing` stores a SHA-256 of the
script's text with each entry and checks it against the served text
before using it. A mismatch is dropped and evicted, so an earlier
version's entry, or one for another script, is never used. The scripts'
names change with each build as well.
- **The threat, someone who can write to the profile:**
- The hash doesn't authenticate the compiled code, so such a writer
could still plant code that runs with the preload API.
- That's the trust Chromium and every Electron app's http(s) pages
already place in the profile's code cache. It's accepted for the
measured gain, and recorded for the next security review.
- Main's compile cache would have given the same writer a path into the
main process, past the asar integrity fuse, with weaker checks (V8
checks little more than the source's length), for no measurable gain.
That's why it's left out.

## Testing
- `pnpm check` passes.
- **`app.test.ts`:** the scheme is registered with `codeCache: true`.
- `pnpm e2e`, the app's project: 277 passed, 1 skipped, and 1 failed:
`canvas-toolbar.spec.ts`'s window-resize test. That file also failed
once on #903's run, with a different test, and passed 18 of 18 when
rerun here.
- `pnpm e2e:packaged`: 12 passed, including the fuses read from the
binary and the editor loaded from `webforge-app://` (S5).
- The update test (an installed 0.0.1 updating itself to 0.0.2 on its
profile, so with 0.0.1's code cache) installs Webforge, so it runs only
on CI's Windows runner (`WEBFORGE_E2E_INSTALL`). It's this PR's check
that a new version doesn't use a stale cache.
- **Measured by hand:** Node's `NODE_DEBUG_NATIVE=COMPILE_CACHE` log for
main's cache. For the renderer, a 4.4 MB `Code Cache` after one launch,
and the timings above.

## Code review (`/code-review`)
- Found and changed before this PR: the first version turned on main's
compile cache from a new bootstrap entry, which split `index.ts` into
`index.ts` and `main.ts`. Once measured, that change was dropped with
the cache.
- Checked with nothing to fix:
  - `codeCache` is valid only with `standard`, which the scheme has.
  - The Help window shares the scheme, so it gets the cache too.
  - Nothing else in the app reads `APP_SCHEME_PRIVILEGES`.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-review/approved ✅ merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. semver/minor backwards-compatible functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Code cache doesn't persist for custom standard protocols

6 participants