Backport #12877 to 2.0 #13021
Backport #12877 to 2.0 #13021
Conversation
lib/sandboxed_renderer/init.js
Outdated
| value: webContentsId | ||
| }) | ||
|
|
||
| require('../renderer/web-frame-init')() |
There was a problem hiding this comment.
@tarruda looks like this file doesn't exist in the 2-0-x branch
There was a problem hiding this comment.
+ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event, preloadPath) {I hate to be "that guy" but this seems more like a new feature that happens to also fix a bug.
I like both the feature and the fix, but the feature doesn't seem appropriate for the bugfix-only 2-0-x series -- I'd prefer to see a less intrusive patch.
a75c5df to
4c8ae6c
Compare
I could adapt the RPC call to remove the parts that are not available in 2.0, such as I see no way to do this without adding the new RPC |
|
@ckerr I've added a fixup commit that reduces the amount of public API changes. |
f950825 to
b946f46
Compare
|
@ckerr just to be clear, is the new RPC handler what you are seeing as a new feature? If so, remember that the RPC methods starting with uppercase are internal to electron, and not part of the public API. |
b946f46 to
e51cf36
Compare
|
If we are backporting this to 2.0, why not 1.8 as well? |
| return loadedModules.get(module) | ||
| } | ||
| if (remoteModules.has(module)) { | ||
| return require(module) |
There was a problem hiding this comment.
It's not clear to me what the criteria is for putting a module in loadedModules vs remoteModules?
If Node caches require(foo) calls s.t. if (remoteModules.has(module)) return require(modules) is cheap, why preload anything at all in loadedModules?
OTOH if this setup exists because require(foo) still has some nonzero cost, why not make loadedModules a lazy-populated map that caches previous require(foo) calls?
There was a problem hiding this comment.
If Node caches require(foo) calls s.t. if (remoteModules.has(module)) return require(modules) is cheap, why preload anything at all in loadedModules?
This is a browserify require and browserify can only bundle modules specified as string literals.
OTOH if this setup exists because require(foo) still has some nonzero cost, why not make loadedModules a lazy-populated map that caches previous require(foo) calls?
As I said above, it is not about the cost, but I can change it so it returns from loadedModules directly
There was a problem hiding this comment.
I don't have a strong opinion on whether to lazy-cache these in loadedModules. My uninformed guess is that it would be faster than re-requiring a module but I don't know how often we'll hit that use case.
However, I'm still interested in understanding this patch better -- what's the criteria for putting a module in loadedModules vs remoteModules? That is, how to decide which modules to preload and which to lazy-load?
There was a problem hiding this comment.
remoteModules are modules which are proxies to node.js modules in the browser process. We do this because the browserify-provided shims for some modules
(child_process, os, fs and path) are not ideal for Electron apps.
Here's how browserify is invoked by Electron build system: https://github.com/electron/electron/blob/master/electron.gyp#L513-L519
These arguments provide replacements for browserify shims, but they also cause electron's remote module to be loaded, which is what we want to avoid in early startup for sandbox
loadedModules are browserify shims that are already available in the renderer process, so it is possible for an Electron app to create a preload bundle and pass -e to exclude from their own bundle, and the require will work since it is provided by loadedModules.
There was a problem hiding this comment.
these things should have been discussed in the original PR. this is a backport.
lib/renderer/api/remote.js
Outdated
| @@ -297,10 +297,13 @@ exports.getCurrentWebContents = () => { | |||
|
|
|||
| const CONTEXT_ARG = '--context-id=' | |||
| let initialContext = process.argv.find(arg => arg.startsWith(CONTEXT_ARG)) | |||
| if (initialContext) { | |||
| if (process.webContentsId) { | |||
There was a problem hiding this comment.
(minor) if this clause is true then we don't need to look for CONTEXT_ARG in argv at all, so that work should go into an } else { ... block after this block
| platform: process.platform, | ||
| execPath: process.execPath | ||
| } | ||
| }) |
There was a problem hiding this comment.
I agree with @miniak and @MarshallOfSound that this is a "read any file plz" API. The consensus in the master PR seems to have been "address that in a followup change" but that's not been written yet so what we're doing here is proposing only the risky bits for a bugfix-only stable release.
I see the value in fixing #12316 for sandboxed mode, but this seems like a risky fix.
Maybe we should wait for this to finish baking in master, then backport it to a future 2-0-x release
There was a problem hiding this comment.
As I said in previous comments, there's no new risk being introduced here, because renderers currently have unfiltered access to the full range of Electron IPC messages. That means if some code can do IPC, it can do anything such as loading modules remotely and calling arbitrary functions.
Even if I make this change now it won't have any benefit for the reason stated above: Untrusted code can still invoke any function remotely. OTOH I will have to test more scenarios since taking the preloadPath from webPreferences is not guaranteed to work for every webContents (think of windows created with window.open).
This is why I wanted to make another PR for changing the preload path behavior: It is a refactoring change that doesn't have much to do with the bugfix being addressed here.
There was a problem hiding this comment.
In case it was not clear, the "read any file plz" API is already available through the IPC messages that implement electron.remote (as well as "write/delete any file plz" APIs, since it is all available through electron.remote.fs)
689da7b to
26ea586
Compare
cde93d6 to
41167ad
Compare
Use a single synchronous IPC call to retrieve data required by early
sandbox scripts. This has two purposes:
- Optimize preload script initialization by:
- Using one synchronous IPC call to retrieve preload script,
webContentsId (more on that later), process.{platform,execPath,env}
- Lazy loading as many modules as possible.
- Fix #12316 for sandbox. @MarshallOfSound addressed the issue in
#12342, but it was still present in sandbox mode. By loading
webContentsId very early and skipping
6D40
remote module at early
startup, we fix it for sandbox.
41167ad to
931cb29
Compare
|
Hi, sorry, I come very late in this discussion but I would like to raise 2 points 1 - Performance impact 2 - Affinity (one process hosting several windows) Regards |
|
As far as I know, Chromium can decide to host several windows in the same process: site strategy, js co-location, ... move away from process args could open Electron to new kind of smart strategies. |
I'd be interested (somewhere else so not to flood this pr) to know your settings and usage of that API, afaik it causes noticeable memory leaks and is very incompatible with lots of node API's and logic that assume single-node-per-process model (E.g. native modules). |
|
Going through the 2.0.x project board and came across this. My apologies for letting it sit as long as it did. At this point, I think it's better to focus on the 3-0-x branch |
#12877