electron: enable context isolation (with inversify)

[paul-marechal]

What it does

This is work on top of #12299 to use Inversify in the preload context.

It also breaks up the monolithic TheiaCoreAPI into several smaller components.

The challenges that had to be solved are the following:

• How to share Inversify bindings from one context to another:

  An instance of TheiaPreloadContext is exposed to the browser global scope. When initializing the browser Inversify containers, we call theiaPreloadContext.getAllServices() to get pairs of [serviceIdentifier: string, serviceProxy: object] which we use to rebind each component as bind(serviceIdentifier).toConstantValue(serviceProxy).

• How to share proxies of instances with prototypes:

  Electron only supports proxying objects used as records/dictionaries and won't follow prototype chains. This means we have to pre-process objects to make it work with Electron's cross-context APIs. This is done with the help of a decorator-based API @proxyable() and @proxy(). Then IpcHandleConverter is handling the pre-processing of instances using the metadata defined using the decorators.

It is now possible to bind and rebind preload components as a framework user.

Please see ./packages/core/ELECTRON-COMMON.md for more details about how to contribute preload APIs.

I also write more information in my commit messages.

How to test

Execute commands such as:

• Toggle Developer Tools
• View: Toggle Full Screen
• File: Reveal in File Explorer

Maximazing/minimizing/closing windows should still work.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

@tsmaeder @msujew Note that even though I said that this work builds "on top" of #12299, it does so in a way that replaces completely what is done in the original PR. I had to split up and re-organize code which is the extend of "built on top".

In short, unless there's big show-stoppers I'm proposing to drop #12299 and keep going forward with this PR.

For instance, the issue when closing a window not preventing application exit that Mark mentioned wasn't an issue in this PR. This is because by adding typings to the channel messages I caught the issue from the original code.

[paul-marechal]

I fixed an issue where the button to maximize the window wasn't updating: I was sending the events but not listening for them, oops. I don't think there's much we can do with typings to catch these bugs ;)

Also made so TheiaIpcMain.on/once handlers can just return the value they wish to answer calls to sendSync with.

[kittaakos]

Hi, what's the plan with this PR? Having a new diff against the current master (9245bb9) would be great. Thanks!

[msujew]

@kittaakos I believe the newest version is already rebased on the current master.

[paul-marechal]

I just noticed an issue with the current context isolation feature and what's being contributed here: Essentially what we do is setup a bridge in the preload context that talks to the main context on behalf of the browser context, all via message passing.

But there already is a mechanism to communicate from the browser to the main context using JSON-RPC proxies:

theia/examples/api-samples/src/electron-main/update/sample-updater-main-module.ts

Lines 28 to 34 in 877f962

	bind(ElectronConnectionHandler).toDynamicValue(context =>
	new JsonRpcConnectionHandler<SampleUpdaterClient>(SampleUpdaterPath, client => {
	const server = context.container.get<SampleUpdater>(SampleUpdater);
	server.setClient(client);
	client.onDidCloseConnection(() => server.disconnectClient(client));
	return server;
	})

This infrastructure overlaps very much with what has been introduced to handle context isolation, the key differences being:

• The new system, with or without rewrite, requires much more boilerplate than the JSON-RPC proxies.
• Only the new system supports synchronous calls.

Instead of keeping the two somewhat redundant systems (communication between browser and main), I'd rather consolidate both. I think we should replace the old JSON-RPC proxies with proxies based on Electron's IPC mechanisms.

I think it would be neat to allow proxies to follow the following naming convention for fields:

• on<EventName> is expected to be an Event and will forward notifications.
• <someMethodName> is a regular method that returns a result as a promise.
• <someMethodName>Sync is a method that returns a result synchronously.
• notify<SomeMethodName> is a method that will not wait for any result/error (void).

This naming convention is easily implementable from a proxy handler point-of-view, and Electron's IPC APIs support all the use cases mentioned.

Creating proxies this way would also most likely lower the amount of boilerplate required to setup communication.

The system introduced to support context isolation requires you to:

1. Define your API identifier and interface.
2. Define your IPC message channels for each method.
3. Implement the instance sending messages from the preload context.
4. Implement the instance handling messages in the main context.
5. Bind both implementation in their respective runtimes.

The proxy system I propose would require you to:

1. Define your API identifier and interface.
2. Implement the API in the main context.
3. Bind the API in the main context and create a proxy reference in the browser context.

Again, the main goal being to consolidate the browser/main communication infrastructure and reduce boilerplate.

I think this current PR is irrelevant if we decide to go the route of consolidation, because I don't want to expose the APIs implemented here if we plan on getting rid of them later to implement the consolidated proxy API.

[tsmaeder]

@paul-marechal I think using the same mechanism for exposing electron-main API as we do for exposing back-end services is a great idea! I think it would tick all the boxes:

1. It does not expose arbitrary electron-main API as you would have to bind the electron-main service at compile time, so security: check!
2. The preload.js just becomes a static file that sets up the message forwarding.
3. We have the ability to rebind electron-main API, as it's just another rpc service.

The problem that prevents us from reusing the current rpc system "as is" is that we need to have synchronous service calls for electron-main API, whereas the current system only supports asynchronous calls. However, that seems more of an implementation detail than a conceptual problem: proxy-factory.ts already implements a naming convention much like the one you're proposing. Adding a case for "-sync" would be simple. However, I don't think that would be the right way to go: the ability to synchronously wait for a result feels like a property of the underlying channel more than the service interface, IMO. So I would be leaning towards having a "synchronous proxy factory" where method calls would be waited for vs. the current async proxy factory. What would we do if we use the proxy factory with an interface containing a "-sync" method with a connection that does not support synchronous message sends?
As for a new naming convention: I'm not really sure why it would be an improvement over the current one. Could you explain how it would simplify things?

[paul-marechal]

the ability to synchronously wait for a result feels like a property of the underlying channel more than the service interface

I would call it a capability. From the proxy instance point-of-view, I'm having it deal with an RpcClient that abstracts the connection details from the final proxy instance. The internal wiring will be in charge of implementing and providing such RpcClient for proxies meant to reference main services from a browser context.

// expected interface to abstract away the connection from the proxying bits:
export interface RpcClient {
    sendNotification?(method: string, params: unknown[]): void;
    sendRequest?(method: string, params: unknown[]): Promise<unknown>;
    sendRequestSync?(method: string, params: unknown[]): unknown;
}

When I said:

I think we should replace the old JSON-RPC proxies with proxies based on Electron's IPC mechanisms.

I meant to not emulate a buffer message connection on top of Electron's ipcRenderer/ipcMain APIs and JSON-RPC (or the custom message-rpc protocol), but rather to directly use Electron's APIs to send our messages:

// simplified implementation to illustrate:
const client: RpcClient = {
    sendNotification: (method, params) => {
        ipcRenderer.send('rpc-notification-channel', method, params);
    },
    sendRequest: (method, params) => ipcRenderer.invoke('rpc-request-channel', method, params),
    sendRequestSync: (method, params) => ipcRenderer.sendSync('ipc-request-sync-channel', method, params),
};

I believe that we will be able to provide valid implementations for RpcClient for connections later, as the synchronous support is optional (don't provide the sendRequestSync method and that's it.)

As for a new naming convention: I'm not really sure why it would be an improvement over the current one. Could you explain how it would simplify things?

Less boilerplate, and it avoids having to think about whole APIs having to be synchronous or asynchronous without the ability of mixing. Consider the NodeJS fs API: most functions are async, and a sync version is available as somethingSync within the same namespace. It's much more convenient than having to split your APIs in sync vs async e.g. require('fs') and require('fs-sync') (the latter is not a thing). It would go about as well when injecting APIs. So allowing mixing when the transport allows it is much more convenient from a usage point-of-view, and in principle it is not difficult to implement in Electron's case. Synchronous APIs would still not be supported in other contexts for now.

---

All this talk about synchronous API support but ultimately it might be flawed. Yes we can make it work and it gets us out of a pickle with our own infrastructure that expects sync values here and there, but that's the main problem we should fix long term: We should not expect sync values from remote contexts because sync calls will block the thread they are made from until they return. It's an anti-pattern as far as the JS event-loop is concerned. I don't plan on fixing this now.

[paul-marechal]

As for a new naming convention: I'm not really sure why it would be an improvement over the current one. Could you explain how it would simplify things?

For one the method and methodSync convention allow easily mixing sync and async APIs.

The onEvent convention allows reacting to events sent from the remote service as it seems to be a common feature required by a lot of other proxied services through the service.client-model. I don't plan on replicating this model because honestly it has been misused more than once where you get multiple proxies all pointing to the same service, so now the question is "who's client should be assigned to service.client?": and for that we have delegating clients that's actually registered as the client and individual clients registers themselves to this delegate instead. I don't want this mess again so I'll do without. You need to send requests from main to browser? You'll be able to create a proxy in both directions.

Don't get me wrong, the more generic concept of "client" is still relevant to what we're doing (one main process, potentially multiple windows = multiple clients) but I will just avoid relying on the way it's been implemented with our other proxies. In the future the new system could replace the old and it would help some implementations make a lot more sense (read "more maintainable".)

[paul-marechal]

Further work required in a future alternative PR.