Create a clean messaging API for internal use

[trev]

Background

For our E10S support, we are currently using message manager directly. This results in overly complicated code however: message names have to avoid conflicts with other extensions, message handlers have to be removed explicitly, response handling has to be implemented explicitly, messaging code is duplicated for the first-run page.

What to change

Create a clean internal API, accessible in both parent and child via the messaging module:

• Port constructor binds a communication port to a message manager.
• port instance is the default port (communicating with the parent for process scripts, broadcasting to all children for the parent).
• Port.disconnect() shuts down the port (done automatically for the default port when the extension is shut down).
• Port.on() adds a handler for a particular message (message names are specific to Adblock Plus and won't conflict with other extensions).
• Port.off() removes a handler.
• Port.emit() sends/broadcasts an asynchronous message.
• Port.emitWithResponse() sends/broadcasts an asynchronous message. It returns a promise that will be resolved when the response is received.
• Port.emitSync() sends a synchronous message, response is returned from the function.

Message handlers have two parameters: payload (the data sent by the sender) and sender (a Port instance). The handler has the choice between returning undefined (no response), a value (actual response) or a promise (indicates an async response). We should warn if multiple handlers choose to return a value for the same message or if a handler chooses to return a promise for a sync message.

[sebastian]

Replying to trev:

and sendResponse (callback to be called with the response).

What do we need that callback for if the return value can be the result or a promise that provides the result?

[trev]

True, I forgot to remove that parameter.

Actually, there is a complication I didn't consider. When the parent is broadcasting a message, there will normally be two child processes receiving it. How is the parent going to know that it received the response if each child process is going to send one? It seems that the parent needs to wait for two responses (more precisely, nsIMessageBroadcaster.childCount) to arrive, then choose the one that isn't undefined (also warn if more than one is).

[sebastian]

I suppose it can bail out as soon as it gets a non-undefined response. Things probably get a little more complicated with promises though.

[trev]

That's not going to make things simpler - even if all responses are undefined it still has to recognize that the message has been processed and the callback can be removed. Also, collecting all responses allows warning properly if multiple processes decide to respond (indicates a bug). So that's what I implemented now.

[sebastian]

Well, it would still be more efficient in the average case with multiple message responders, if we already call the message listener as soon as a response arrived. We can still log an error if later another response arrived.

[sergz]

I would like to share a fresh view on the API without looking at the real code.

1. regarding Port.emit:

What about returning of an array of promises? In that case it seems the caller of Port.emit can decide and easily express what he wants using Promise API, e.g. results of resolving of all promises or when at least one is rejected (Promise.all), or when only one result is resolved or at least one promise is rejected (Promise.race).
We still can add some post-action to each promise for debug, for instance.

As I understand from the description Port is supposed to be used either in parent process or in client process. Another thing here is that we don't know how many listeners are in ​chrome process or in parent process, and they can "send response" not in the order of receiving of the message, which can result in a really tricky race condition bugs. So we cannot actually always use nsIMessageBroadcaster.childCount and cannot rely on the order of the response messages.

2. if we see that we often need to wait only for first non-undefined resolved value or we are expecting to have only one promise in that array then we can generalize it by creating a helper function (and extend Promise API) or by adding a function with the more specific name into Port.prototype. The implementation of such functions seems quite straightforward. Actually, waiting for a first non-undefined value can result in some bugs because of a race conditions.

3. regarding Port.emitSync. I understand that it can be very convenient and I have no objections regarding it now, but do we actually need it, especially in Chrome process? May be it's a signal to improve something, so we avoid possible unnecessary blocking.

BTW, speaking about efficiency, does it really makes sense to try to optimize something here now? Just wonder what are the cases when we expect to have big arrays and chains of promises?

So, I would propose to think rather on less generalization, may be different API for Port in Chrome and Content process, don't complicate the logic in Port.emit and Port.emitSync. The class by itself only from the description looks already extremely helpful, try to use it and come later with the necessary logic or optimizations improvements.

[trev]

Replying to sergz:

1. regarding Port.emit:

What about returning of an array of promises?

There are two drawbacks here:

1) Complexity. As it is now, we don't have 1:n communication anywhere, at least when a response is expected. It's always 1:1 communication, and we broadcast merely because the parent doesn't know which child is going to respond. Right now the framework will select the one "actual" response and this simplifies the callers a lot.

2) Performance. If we return promises then we cannot know whether the caller actually expects a response. Currently, we won't even ask for a response if the caller doesn't provide a callback (meaning: no call tracking overhead, no response messages sent). And that's the majority of calls.

So we cannot actually always use nsIMessageBroadcaster.childCount and cannot rely on the order of the response messages.

Right now we can because the messaging module will already choose one response when the message is processed in the child - only one response is sent back to parent, not all of them.

3. regarding Port.emitSync. I understand that it can be very convenient and I have no objections regarding it now, but do we actually need it, especially in Chrome process?

Yes, we need it. And - yes, the JSDoc comment currently says "DO NOT USE UNLESS ABSOLUTELY NECESSARY." And it is absolutely necessary when it comes to content policies, these expect a synchronous response right now. I hope that Mozilla will fix this eventually. Chrome doesn't have that issue and there we wouldn't need it (not that we can implement it at all there).

BTW, speaking about efficiency, does it really makes sense to try to optimize something here now?

Not too much I think.

[sebastian]

I agree, that we rather shouldn't support multiple responses, for reasons already mentioned. However, I like the idea of Port.emit() returning a promise. But I also see that this will add extra costs in cases where no response is expected. So how about providing two methods, one that returns a promise, and one that returns nothing.

[trev]

Ok, I added Port.emitWithResponse() to the description, returning a promise.

[abpbot]

A commit referencing this issue has landed:
​https://hg.adblockplus.org/adblockplus/rev/0d68dbc94bee

[abpbot]

Some commits referencing this issue have landed:

• ​https://hg.adblockplus.org/adblockplus/rev/cdab7df413fb
• ​https://hg.adblockplus.org/adblockplus/rev/48ed90ef035e
• ​https://hg.adblockplus.org/adblockplus/rev/117df104f789
• ​https://hg.adblockplus.org/adblockplus/rev/dbbc74e34d27
• ​https://hg.adblockplus.org/adblockplus/rev/0cb5a38700e1

[Ross]

Both first-run and subscribe functionality are still working as expected in both non-E10S and E10S Firefox after these changes.

ABP 2.7.2.4163
Firefox 38 / 44 / Windows 8
Nightly 48.0a1 (2016-04-19) / Windows 8
Firefox 38 / 44 / OS X 10.11
Firefox 45.01 / Ubuntu 14.04
Nightly 48.0a1 (2016-04-19) / Ubuntu 14.04