Issue 3499 - Create a clean messaging API for internal use

lib/messaging.js

+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+"use strict";
+
+const MESSAGE_NAME = "AdblockPlus:Message";
+const RESPONSE_NAME = "AdblockPlus:Response";
+
+function sendMessage(messageManager, messageName, payload, callbackID)
+{
+  let request = {messageName, payload, callbackID};
+  if (messageManager instanceof Ci.nsIMessageSender)
+  {
+    messageManager.sendAsyncMessage(MESSAGE_NAME, request);
+    return 1;
+  }
+  else if (messageManager instanceof Ci.nsIMessageBroadcaster)
+  {
+    messageManager.broadcastAsyncMessage(MESSAGE_NAME, request);
+    return messageManager.childCount;
+  }
+  else
+  {
+    Cu.reportError("Unexpected message manager, impossible to send message");
+    return 0;
+  }
+}
+
+function sendSyncMessage(messageManager, messageName, payload)
+{
+  let request = {messageName, payload};
+  let responses = messageManager.sendRpcMessage(MESSAGE_NAME, request);
+  for (let response of responses)
+    if (typeof response != "undefined")
+      return response;

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: What's the reason for not using `flattenResponses()` here?

[Wladimir Palant, 2016/03/21 15:31:31]

No real reason, flattenResponses() was simply introduced after the function
here. Done.

+  return undefined;
+}
+
+function flattenResponses(responses, messageName)
+{
+  let result = undefined;
+  for (let response of responses)
+  {
+    if (typeof response == "undefined")

[Erik, 2016/03/17 05:02:04]

nit `==` -> `===`

[Wladimir Palant, 2016/03/21 15:31:32]

We generally don't use === unless it makes a difference.

+      continue;
+
+    if (typeof result == "undefined")

[Erik, 2016/03/17 05:02:04]

nit `==` -> `===`

[Wladimir Palant, 2016/03/21 15:31:33]

Same as above.

+      result = response;
+    else
+      Cu.reportError("Got multiple responses to message '" + messageName + "', only first response was accepted.");
+  }
+  return result;
+}
+
+function getSender(origin)
+{
+  if (origin instanceof Ci.nsIDOMXULElement)
+    origin = origin.messageManager;
+
+  if (origin instanceof Ci.nsIMessageSender)
+    return new LightWeightPort(origin);
+  else
+    return null;
+}
+
+/**
+ * Lightweight communication port allowing only sending messages.
+ * @param {nsIMessageManager} messageManager
+ * @constructor
+ */
+function LightWeightPort(messageManager)
+{
+  this._messageManager = messageManager;
+}
+LightWeightPort.prototype =
+{
+  /**
+   * @see Port#emit
+   */
+  emit: function(messageName, payload)
+  {
+    sendMessage(this._messageManager, messageName, payload);
+  },
+
+  /**
+   * @see Port#emitSync
+   */
+  emitSync: function(messageName, payload)
+  {
+    return sendSyncMessage(this._messageManager, messageName, payload);
+  }
+};
+
+/**
+ * Communication port wrapping the message manager API to send and receive
+ * messages.
+ * @param {nsIMessageManager} messageManager
+ * @constructor
+ */
+function Port(messageManager)
+{
+  this._messageManager = messageManager;
+
+  this._callbacks = new Map();
+  this._responseCallbacks = new Map();
+  this._responseCallbackCounter = 0;
+
+  this._handleRequest = this._handleRequest.bind(this);
+  this._handleResponse = this._handleResponse.bind(this);
+  this._messageManager.addMessageListener(MESSAGE_NAME, this._handleRequest);
+  this._messageManager.addMessageListener(RESPONSE_NAME, this._handleResponse);
+}
+Port.prototype = {
+  /**
+   * Disables the port and makes it stop listening to incoming messages.
+   */
+  disconnect: function()
+  {
+    this._messageManager.removeMessageListener(MESSAGE_NAME, this._handleRequest);
+    this._messageManager.removeMessageListener(RESPONSE_NAME, this._handleResponse);
+  },
+
+  _sendResponse: function(sender, callbackID, payload)
+  {
+    if (!sender || typeof callbackID == "undefined")
+      return;
+
+    var response = {callbackID, payload};

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: Unless there's a particular reason not to, please use `let` here like in
the rest of this file.

[Wladimir Palant, 2016/03/21 15:31:33]

Done.

+    sender._messageManager.sendAsyncMessage(RESPONSE_NAME, response);
+  },
+
+  _handleRequest: function(message)
+  {
+    let sender = getSender(message.target);
+    let {callbackID, messageName, payload} = message.data;
+
+    let result = this._dispatch(messageName, payload, sender);
+    if (result && typeof result.then == "function")

[Erik, 2016/03/17 05:02:04]

It'd be nice to have a `isPromise` function to reuse, I think it might make this
bit of code a little easier to read.

[Thomas Greiner, 2016/03/17 12:09:26]

You mean `result instanceof Promise`?

[Wladimir Palant, 2016/03/21 15:31:33]

No, `result instanceof Promise` would be wrong - there isn't necessary a single
Promise class (shim/native + different frames), and there can also be
promise-like classes, e.g. Task.

I added a function isPromise().

+    {
+      // This is a promise - asynchronous response
+      if (message.sync)
+      {
+        Cu.reportError("Asynchronous response to the synchronous message '" + messageName + "' is not possible");
+        return undefined;
+      }
+
+      result.then(result => {
+        this._sendResponse(sender, callbackID, result)
+      }, e => {
+        Cu.reportError(e);
+        this._sendResponse(sender, callbackID, undefined);
+      });
+    }
+    else
+      this._sendResponse(sender, callbackID, result);
+
+    return result;
+  },
+
+  _handleResponse: function(message)
+  {
+    let {callbackID, payload} = message.data;
+    if (!this._responseCallbacks.has(callbackID))
+      return;
+
+    let [callback, messageName, responses, expectedResponses] =
+        this._responseCallbacks.get(callbackID);
+    responses.push(payload);

[Thomas Greiner, 2016/03/17 12:09:26]

Why do you wait for and store all the responses if you're throwing all but one
of them away anyway?

[Wladimir Palant, 2016/03/21 15:31:31]

Because otherwise this code gets rather complicated, at least if I still want to
warn about multiple responses.

[Thomas Greiner, 2016/03/21 18:43:22]

So decrementing `expectedResponses` is not an option or do you mean that you
intend to access individual responses in a later change?

[Wladimir Palant, 2016/03/21 20:42:34]

Without storing all the values in an array we cannot call flattenResponses and
reuse the code.

Anyway, I solved this by using a generator-like object. I used a proper
generator first but the generator protocol is too complicated for this scenario.

+    if (responses.length == expectedResponses)

[Erik, 2016/03/17 05:02:04]

nit `==` -> `===`

[Wladimir Palant, 2016/03/21 15:31:32]

Same as above.

+    {
+      this._responseCallbacks.delete(callbackID);
+      callback(flattenResponses(responses, messageName));
+    }
+  },
+
+  _dispatch: function(messageName, payload, sender)
+  {
+    let callbacks = this._callbacks.get(messageName);
+    if (!callbacks)
+      return undefined;
+
+    callbacks = callbacks.slice();

[Thomas Greiner, 2016/03/17 12:09:26]

Why do you copy the array? I don't see that a callback would modify the array
while iterating through it.

[Wladimir Palant, 2016/03/21 15:31:32]

It sure could. Classic example is a callback that removes itself when called.
This works fine as long it's the only callback. However, if there is a second
callback in the list it will be skipped. We actually hit that exact issue for
other code not too long ago.

[Thomas Greiner, 2016/03/21 18:43:22]

Of course, in theory it could happen because it's not a "private" variable.
However, that'd require other code to access `_callbacks` from outside which
it's not supposed to do. Do you think that convention is insufficient here?

[Wladimir Palant, 2016/03/21 20:42:34]

No, it would merely have to call port.off(arguments.callee). This would modify
the list of callbacks right under our feet.

[Thomas Greiner, 2016/03/22 11:15:07]

Ah, right. Nevermind then.

+    let responses = [];
+    for (let callback of callbacks)
+    {
+      try
+      {
+        responses.push(callback(payload, sender));
+      }
+      catch (e)
+      {
+        Cu.reportError(e);
+      }
+    }
+    return flattenResponses(responses, messageName);
+  },
+
+  /**
+   * Function to be called when a particular message is received
+   * @callback Port~messageHandler
+   * @param payload data attached to the message if any
+   * @param {LightWeightPort} sender object that can be used to communicate with

[Erik, 2016/03/17 05:02:05]

this param appears to just accept a function, and I don't think it would accept
a `LightWeightPort`.

[Wladimir Palant, 2016/03/21 15:31:32]

Explanation below.

+   *      the sender of the message, could be null
+   * @return the handler can return undefined (no response), a value (response
+   *      to be sent to sender immediately) or a promise (asynchronous
+   *      response).

[Erik, 2016/03/17 05:02:05]

`on` doesn't appear to return a promise, or anything.

[Wladimir Palant, 2016/03/21 15:31:32]

Explanation below.

+   */
+
+  /**
+   * Adds a handler for the specified message.
+   * @param {string} messageName message that would trigger the callback
+   * @param {Port~messageHandler} callback

[Erik, 2016/03/17 05:02:04]

same comment as above, this appears to just accept a function, also I'm not sure
why this function appears to have two sets of docs.

[Wladimir Palant, 2016/03/21 15:31:32]

Please see http://usejsdoc.org/tags-callback.html - the first block documents
the callback type.

+   */
+  on: function(messageName, callback)
+  {
+    if (!this._callbacks.has(messageName))
+      this._callbacks.set(messageName, []);

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: Since you're using `Map` I wonder why you're not using `Set` to avoid
having to deal with array indices.

[Wladimir Palant, 2016/03/21 15:31:31]

As Sebastian pointed out, Node.js (unlike Add-on SDK) allows duplicate
callbacks. I changed the logic here to be consistent.

[Thomas Greiner, 2016/03/21 18:43:23]

Interesting, I wonder why they thought that'd be a good idea…

[Wladimir Palant, 2016/03/21 20:42:34]

Probably because it's simpler/more efficient to implement. Also, arguably
eliminating duplicate callbacks tends to mask programming errors.

+
+    let callbacks = this._callbacks.get(messageName);
+    if (callbacks.indexOf(callback) < 0)
+      callbacks.push(callback);
+  },
+
+  /**
+   * Removes a handler for the specified message.
+   * @param {string} messageName message that would trigger the callback
+   * @param {Port~messageHandler} callback

[Erik, 2016/03/17 05:02:04]

if I am not mistaken about the above comments for `on`, then this should just be
`function` as well.

[Wladimir Palant, 2016/03/21 15:31:32]

Same as above.

+   */
+  off: function(messageName, callback)
+  {
+    let callbacks = this._callbacks.get(messageName);
+    if (!callbacks)
+      return;
+
+    let index = callbacks.indexOf(callback);
+    if (index >= 0)
+      callbacks.splice(index, 1);
+  },
+
+  /**
+   * Sends a message.
+   * @param {string} messageName message identifier
+   * @param [payload] data to attach to the message

[Thomas Greiner, 2016/03/17 12:09:26]

Detail: Is this valid JSDoc syntax even with the type missing?

[Wladimir Palant, 2016/03/21 15:31:32]

Yes, it is.

+   */
+  emit: function(messageName, payload)
+  {
+    sendMessage(this._messageManager, messageName, payload, undefined);
+  },
+
+  /**
+   * Sends a message and expects a response.
+   * @param {string} messageName message identifier
+   * @param [payload] data to attach to the message
+   * @return {Promise} promise that will be resolved with the response
+   */
+  emitWithResponse: function(messageName, payload)
+  {
+    let callbackID = ++this._responseCallbackCounter;
+    let expectedResponses = sendMessage(
+        this._messageManager, messageName, payload, callbackID);
+    return new Promise((resolve, reject) => {

[Erik, 2016/03/17 05:02:04]

nit add a newline here?

[Wladimir Palant, 2016/03/21 15:31:32]

Done.

+      this._responseCallbacks.set(callbackID,
+          [resolve, messageName, [], expectedResponses]);
+    });
+  },
+
+  /**
+   * Sends a synchonous message (DO NOT USE unless absolutely unavoidable).
+   * @param {string} messageName message identifier
+   * @param [payload] data to attach to the message
+   * @return response returned by the handler
+   */
+  emitSync: function(messageName, payload)
+  {
+    return sendSyncMessage(this._messageManager, messageName, payload);
+  }
+};
+exports.Port = Port;
+
+let messageManager;
+try
+{
+  // Child
+  messageManager = require("messageManager");
+}
+catch (e)
+{
+  // Parent
+  messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
+                     .getService(Ci.nsIMessageListenerManager);
+}
+
+let port = new Port(messageManager);
+onShutdown.add(() => port.disconnect());
+exports.port = port;