Skip to content

Instantly share code, notes, and snippets.

@deepak1556
Created June 21, 2021 16:42
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save deepak1556/1a5f694b5361a71de33a34ceba38deab to your computer and use it in GitHub Desktop.
Save deepak1556/1a5f694b5361a71de33a34ceba38deab to your computer and use it in GitHub Desktop.
OOP shared worker
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>Hello World!</title>
</head>
<body>
<h1>Hello World!</h1>
</body>
<script>
// https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker
var myWorker = new SharedWorker("worker.js");
// Message ports allow IPC between the OOP worker and this document
myWorker.port.onmessage = function(e) {
console.log('Message received from worker');
console.log(e.data)
}
myWorker.port.postMessage('start');
</script>
</html>
// Modules to control application life and create native browser window
const {app, BrowserWindow, protocol} = require('electron')
const path = require('path')
protocol.registerSchemesAsPrivileged([
{
scheme: 'foo',
privileges: {
standard: true,
secure: true
}
}
])
function createWindow () {
protocol.registerFileProtocol('foo', (request, cb) => {
const url = new URL(request.url)
console.log(`Loading : ${__dirname + url.pathname}`)
if (url.pathname === "/worker.js") {
cb({
path: __dirname + url.pathname
})
} else {
// The headers below force the shared worker to be Out-Of-Process
cb({
path: __dirname + url.pathname,
headers: {
"Cross-Origin-Opener-Policy": "same-origin",
"Cross-Origin-Embedder-Policy": "require-corp"
}
})
}
})
// Create the browser window.
const mainWindow = new BrowserWindow({
width: 800,
height: 600,
webPreferences: {
}
})
// and load the index.html of the app.
mainWindow.loadURL('foo://custom/index.html')
// Open the DevTools.
// mainWindow.webContents.openDevTools()
}
// This method will be called when Electron has finished
// initialization and is ready to create browser windows.
// Some APIs can only be used after this event occurs.
app.whenReady().then(() => {
createWindow()
app.on('activate', function () {
// On macOS it's common to re-create a window in the app when the
// dock icon is clicked and there are no other windows open.
if (BrowserWindow.getAllWindows().length === 0) createWindow()
})
})
// Quit when all windows are closed, except on macOS. There, it's common
// for applications and their menu bar to stay active until the user quits
// explicitly with Cmd + Q.
app.on('window-all-closed', function () {
if (process.platform !== 'darwin') app.quit()
})
// In this file you can include the rest of your app's specific main process
// code. You can also put them in separate files and require them here.
onconnect = function(e) {
var port = e.ports[0];
port.onmessage = function(e) {
port.postMessage('worker connected');
}
}
@deepak1556
Copy link
Author

Pros:

Cons:

  • No finer control on the process spawn (i-e cannot pass custom argv)

@bpasero
Copy link

bpasero commented Jun 21, 2021

Also: we currently give extensions graceful time to do stuff in the deactivate method, in other words, the extension host survives the renderer process or even shutdown of the application (iirc). We would need similar lifecycle control for worker process too I guess.

Another thing to ask is: how different is this runtime from a normal node.js process (since all extensions and their child processes would operate from there).

@deepak1556
Copy link
Author

Also: we currently give extensions graceful time to do stuff in the deactivate method, in other words, the extension host survives the renderer process or even shutdown of the application (iirc). We would need similar lifecycle control for worker process too I guess.

For the render process shutdown case, we emulate a similar behavior with the worker. A shared worker's lifetime is associated with a document that connects to it (i-e) as long as a document remains connected to the worker it can be alive. So for our case, we can create a temporary background window that is hidden just to connect to this existing shared worker and when the main document goes down, we give the necessary graceful time to shutdown the worker via this background window.

For the application shutdown case, does today's extension host outlive the application shutdown ? This is hard to provide support via the worker.

how different is this runtime from a normal node.js process (since all extensions and their child processes would operate from there).

Since we enable node integration in the worker, the operating capabilities will be similar to our todays' node enabled workbench with the exception of no access to DOM.

@alexdima
Copy link

alexdima commented Jun 22, 2021

Thank you for this exploration! I have a bit of concern to migrate the extension host to this immediately. I am concerned that we might break extensions that contain node modules that depend on some specific nodejs behavior that might be slightly different in a SharedWorker. So for example, even today IIRC we have seen some crashes because we run electron.exe with ELECTRON_RUN_AS_NODE to simulate node.exe, namely IIRC in some cryptography stuff, in some i18n stuff, with some env variables, etc. My concern is that we might introduce subtle differences and we might not even know exactly what they are.

My other concern is that enabling node integration in SharedWorker might defeat the purpose of sandboxing the renderer process; an attacker could now simply create a SharedWorker and then gain spawning privileges on the system; I fear it might invalidate the sandboxing effort of the renderer process.

I suggest we continue as proposed, although the progress is slow (and I'm sorry about that, but I am very busy on other stuff):

  • move the spawning of the extension host to the main process or the shared process (more likely the shared process)
  • establish a nodejs socket from the extension host socket to the renderer socket
  • see if we have any MessagePort from Electron, and if not, we can look into using a WebSocket to localhost from the sandboxed renderer.

We can then explore having a shared worker extension host (like how we have the web worker extension host) but maybe that can be something that we can experiment with, like first moving some builtin extensions there, collect telemetry, see if there are any memory improvements/benefits/etc.

@deepak1556
Copy link
Author

deepak1556 commented Jun 22, 2021

Thanks for the feedback!

So for example, even today IIRC we have seen some crashes because we run electron.exe with ELECTRON_RUN_AS_NODE to simulate node.exe, namely IIRC in some cryptography stuff, in some i18n stuff, with some env variables, etc.

Agree, this arises from the fact electron had to share the following libraries from chromium with node to integrate both together. But since this done at build time, the libraries are shared even if we spawn a pure node child process from electron. So behavior won't be different with node integrated shared worker and a node child process in terms of the api surface.

  • BoringSSL the cryptographic library from chromium replaces the openSSL version used by node
  • V8 engine
  • I18n library

As you have mentioned, the cases we don't know that can break in this model can only be found through experimentation down the line.

with some env variables, etc.

This issue is eliminated with the shared worker process as the way chromium creates the process is slightly different from what we do with the child process module.

My other concern is that enabling node integration in SharedWorker might defeat the purpose of sandboxing the renderer process; an attacker could now simply create a SharedWorker and then gain spawning privileges on the system; I fear it might invalidate the sandboxing effort of the renderer process.

This is a great point, when adding the node capability to shared workers I plan to expose an event on the app module based on https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/content_browser_client.h;l=682-690 that will get called whenever a shared worker is requested to be created with relevant details about the requestor document as well as the worker endpoint. This allows app to finely control whether they want to allow node integration in the worker or not and also whether to start the worker. Does this address the security concern ?

app.on('will-create-shared-worker', (event, details) => {
  /**
   * event Object
     * preventDefault Function
     * enableNodeIntegration Boolean
   **/ 
  /**
   * details Object
     * workerURL String
     * documentOrigin String
     * document webContents
   **/
});

I suggest we continue as proposed

Agree with your plan, just to add some pointers to 3)

see if we have any MessagePort from Electron, and if not, we can look into using a WebSocket to localhost from the sandboxed renderer.

This not currently worked on in the Electron land, and I did start my exploration from here. Originally, I looked to establish message ports between a node child process and a chromium renderer process. Digging at the internals, the message ports are closely tied to the event loop mechanism of chromium, so it required binding some chromium libraries in the node child process which would eventually end up as a chromium process with node integration enabled. From there, I landed on this out-of-process shared worker which was already performing most of what is required.

In conclusion, is there a general interest to pursue this model further for vscode ? Could we experiment with migrating the file watcher service or any other candidates that are less complex to extension host ? which can give us data points to drive future conversations about this model ?

@bpasero
Copy link

bpasero commented Jun 23, 2021

I like the idea of exploring shared workers with node.js integration for file services specifically because I do not think this service benefits from running in the background from the shared process. It still needs to be isolated process wise given file watch events can be expensive. One thing that would be nice to have is if the shared worker could also have a direct communication channel to the extension host to deliver file events, but that is maybe P2.

My main worry for file service adoption is how expensive / slow it is to create such a shared worker. The file service is obviously one of those things we need right from the beginning because many things get resolved on startup. We can try to offload some of that work into the initial config object we send over but it might become unpractical very quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment