-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Adopt utility process for shared process and file watchers #154050
Comments
settings sync is safe to be removed as it does not use any non sandbox stuff like node apis. |
To clarify: we can still have a shared process in the end, I do not have strong feelings, but it would be node.js only and not electron browser enabled. I think the benefit of having another process offload work from the main process is still valid. |
One concern I have with shared process not having electron browser enabled is the network proxy and If we can enable request service to redirect to main as you mentioned, this shall be fine. But I am wondering how much load we gonna put on main and if that impacts perf. |
As far as I know, network load is handled from another utility process, so I think that should be fine, right @deepak1556 ? |
Yes that is correct, the main process is only responsible for initiating the request from the JS thread, the actual work is done by the network utility process. |
For the terminal part in shared proc, it just passes manages the pty host process and passing requests to/from it, so good there. #153182 seems like a blocker to enabling utility processes though. |
Sorry for the confusion, I was not suggesting the issues are related, rather was wondering if moving to utility process would help for #133895 |
Oh, I haven't looked at any of the utility process stuff yet, so not sure what the difference between a regular node (pty host now) and utility processes are. If the pty host was a utility process off the main process would I be able to setup direct comm channels between the renderer and the pty host? |
@Tyriar yes that is the idea, in short:
(the extension host has adopted and is validating this approach in insiders) |
I think you can even keep having a PTY host (as a utility process) and then child processes for each terminal, but I would then do 1 message port per window to offload communication. @deepak1556 just wondering: can I send a message port further down to child processes to even offload work from the utility process or is the message port limited to operating only from the utility process? |
Sounds great! 🙂 |
Message port is limited to operating only from the utility process. I would prefer to keep it this way since otherwise we could have untrusted child processes talking directly to the renderer and serves as a way to break out of the renderer sandbox. |
* Reactor telemetry appenders to reduce duplicated code * Update tsconfig * Do not use Object.create(null) because we need object.hasOwnProperty
Reverted request service in shared process to use browser XHR because of the bug #155204 |
Since the request service issue has been addressed, I am starting to work on this for January. I already figured out that the file watcher has to move out of the shared process into its own utility process. @Tyriar I will let you know how that work progresses because I feel once I made the move, PTY host should also move out and by then I am hopefully having a set of utilities in place to make the adoption easier. I am building on top of the code from AlexD used for the extension host. |
app.enableSandbox()
app.enableSandbox()
Some initial work landed on There are some issues with integrated terminal that I cannot reproduce running out of sources but appear in the built version. But I was able to browse and install an extension. |
app.enableSandbox()
app.enableSandbox()
üpoiu9z88u9i+#ü
üß
app.enableSandbox()
üpoiu9z88u9i+#ü
üßapp.enableSandbox()
Terminal works as well now! It was just an issue where we passed an unsupported |
I will close this one out for Feb since the significant portion of the work has already happened. Will create a new issue for moving terminals out, but for now they coexist as before. |
app.enableSandbox()
This is a follow up from #92164 and covers remaining work to eventually enable sandboxed renderers fully in Electron via
app.enableSandbox()
.This means that our shared process has to move away from a node.js enabled browser window to the new utility process.
Breaking down the usages today:
Some initial thoughts:
net
APIs from theelectron-main
process to not loose proxy supportIRequestService
that is backed by a main process service implementation//cc @alexdima
The text was updated successfully, but these errors were encountered: