Skip to content

fix: ensure standard schemes are registered in nw service process#22867

Merged
deepak1556 merged 3 commits into
masterfrom
robo/fix_nw_process_crash
Apr 1, 2020
Merged

fix: ensure standard schemes are registered in nw service process#22867
deepak1556 merged 3 commits into
masterfrom
robo/fix_nw_process_crash

Conversation

@deepak1556

@deepak1556 deepak1556 commented Mar 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Description of Change

Crash in Network service process due to #20546 regressed after f5b9e49 .

This PR fixes by making a clear indication of how the schemes are registered in respective Browser, Renderer and Network process.

Identified while testing #20625 but ci logs in master also show the crash.

Checklist

Release Notes

Notes: fix crash in network service process when using protocol.registerSchemeAsPrivileged api

@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Mar 27, 2020
@deepak1556
deepak1556 requested review from a team, nornagon and zcbenz March 27, 2020 19:50
@deepak1556
deepak1556 force-pushed the robo/fix_nw_process_crash branch from dd6c8f7 to ff670d1 Compare March 27, 2020 19:53
nornagon
nornagon reviewed Mar 27, 2020
View reviewed changes

@nornagon nornagon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we get a test?

@deepak1556

deepak1556 commented Mar 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nornagon thoughts on how to test this ? when this crash happens it doesn't seem to affect the resource loading on those custom schemes, so thats why existing test suites where passing. Only the service process goes on a loop restarting.

@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Mar 28, 2020
@nornagon

nornagon commented Mar 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@nornagon thoughts on how to test this ? when this crash happens it doesn't seem to affect the resource loading on those custom schemes, so thats why existing test suites where passing. Only the service process goes on a loop restarting.

Maybe I'm confused then... how is this change visible to the user?

@deepak1556

deepak1556 commented Mar 31, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Just infinite logs from network service

[14085:0331/150914.566974:WARNING:pref_notifier_impl.cc(61)] Init observer found at shutdown.
[14085:0331/150914.568450:ERROR:validation_errors.cc(76)] Invalid message: VALIDATION_ERROR_DESERIALIZATION_FAILED (network.mojom.NetworkContext.1731934142 )

The registered scheme itself works fine, files are served over the protocol, renderer window.location object parses the host and other properties.

And just observed that shutdown of app isn't graceful. Let me see if I can write a test that verifies app shutdown when using this api.

@deepak1556
deepak1556 force-pushed the robo/fix_nw_process_crash branch from ff670d1 to cb91e5c Compare March 31, 2020 23:37
@deepak1556

deepak1556 commented Mar 31, 2020

Copy link
Copy Markdown
Member Author

@nornagon added simple test, PTAL.

nornagon
nornagon approved these changes Mar 31, 2020
View reviewed changes

@nornagon nornagon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems weird to me that the network service would function fine but continually crash..? but OK.

Comment thread spec-main/api-protocol-spec.ts Outdated
Comment thread spec-main/api-protocol-spec.ts Outdated
@deepak1556
deepak1556 force-pushed the robo/fix_nw_process_crash branch from 4209b9a to de656d6 Compare April 1, 2020 00:19
@deepak1556

deepak1556 commented Apr 1, 2020

Copy link
Copy Markdown
Member Author

Failing test is unrelated, merging.

@deepak1556
deepak1556 merged commit bac1c7f into master Apr 1, 2020
@release-clerk

release-clerk Bot commented Apr 1, 2020

Copy link
Copy Markdown

Release Notes Persisted

fix crash in network service process when using protocol.registerSchemeAsPrivileged api

@deepak1556
deepak1556 deleted the robo/fix_nw_process_crash branch April 1, 2020 02:33
@trop trop Bot mentioned this pull request Apr 1, 2020
Merged
@trop

trop Bot commented Apr 1, 2020

Copy link
Copy Markdown
Contributor

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

sentialx pushed a commit to sentialx/electron that referenced this pull request Apr 7, 2020
@deepak1556 @nornagon @sentialx
fix: ensure standard schemes are registered in nw service process (el…
bdea28d 
…ectron#22867)

* fix: ensure standard schemes are registered in nw service process

Refs electron#20546

* chore: add test

* chore: apply suggestions from code review

Co-Authored-By: Jeremy Apthorp <[email protected]>

Co-authored-by: Jeremy Apthorp <[email protected]>
@sentialx sentialx mentioned this pull request Apr 9, 2020
Closed
3 tasks
This was referenced Jul 1, 2026
Open
Merged
yyq1025 added a commit to yyq1025/laufey that referenced this pull request Jul 7, 2026
@yyq1025 @claude
fix(cef): register custom schemes in sub-processes so app:// pages load
dfe84bf 
OnRegisterCustomSchemes was implemented only on LaufeyApp (the browser
process). The CefApp handed to CefExecuteProcess for every sub-process
(helper.cc -> LaufeyRendererApp) did not override it, so custom standard
schemes were declared only in the browser. A custom standard scheme must be
registered in every process, including the network service utility process;
otherwise the network service rejects navigations to it
(VALIDATION_ERROR_DESERIALIZATION_FAILED on network.mojom.NetworkContext) and
the page stays blank. Only built-in schemes (data:, https:) worked.

Mirror LaufeyApp::OnRegisterCustomSchemes on LaufeyRendererApp so the scheme
is registered in the renderer, GPU, and network-service processes too. Same
fix Electron applied in electron/electron#22867.

Also add an app:// case to the cef_e2e harness (it previously only exercised a
data: URL, which cannot hit this bug): before, the navigation floods
network.mojom.NetworkContext errors and never loads; after, it loads and the
page script round-trips through a binding.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@nornagon nornagon nornagon approved these changes
@zcbenz zcbenz zcbenz approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@deepak1556 @nornagon @zcbenz @sofianguy