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-2015 Eyeo GmbH

[Sebastian Noack, 2016/03/16 10:44:13]

It's 2016.

[Wladimir Palant, 2016/03/16 11:04:49]

Right, this patch is older... Fixed.

+ * 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 isPromise(value)
+{
+  // value instanceof Promise won't work - there can be different Promise
+  // classes (e.g. in different contexts) and there can also be promise-like
+  // classes (e.g. Task).
+  return (value && typeof value.then == "function");
+}
+
 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)

[Sebastian Noack, 2016/03/16 10:44:13]

Nit: Redundant else

[Wladimir Palant, 2016/03/16 11:04:50]

As you know, I like redundant else clauses :). While the compiler doesn't need
them, humans like us can immediately see that only one branch is being taken
without having to look at the contents of the blocks.

   {
     messageManager.broadcastAsyncMessage(MESSAGE_NAME, request);
     return messageManager.childCount;
   }
   else

[Sebastian Noack, 2016/03/16 10:44:13]

Nit: Redundant else

[Wladimir Palant, 2016/03/16 11:04:49]

Same as above.

   {
     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);
+  let processor = new ResponseProcessor(messageName);
   for (let response of responses)
-    if (typeof response != "undefined")
-      return response;
-  return undefined;
-}
-
-function flattenResponses(responses, messageName)
-{
-  let result = undefined;
-  for (let response of responses)
+    processor.add(response);
+  return processor.value;
+}
+
+function ResponseProcessor(messageName)
+{
+  this.value = undefined;
+  this.add = function(response)
   {
     if (typeof response == "undefined")
-      continue;
-
-    if (typeof result == "undefined")
-      result = response;
+      return;
+
+    if (typeof this.value == "undefined")
+      this.value = response;
     else
       Cu.reportError("Got multiple responses to message '" + messageName + "', only first response was accepted.");
-  }
-  return result;
+  };
 }
 
 function getSender(origin)

[Erik, 2016/03/15 22:56:35]

I'm not quire sure when this would be used, since sent messages using
`MESSAGE_NAME` appear to only occur above and they appear to not use the
`message.target` attribute.

[Wladimir Palant, 2016/03/16 09:45:17]

This is something that the first-run page will need - it subscribes to
notifications, so messageResponder module needs a way to remember the sender of
the "subscriptions.listen" message.

 {
   if (origin instanceof Ci.nsIDOMXULElement)
     origin = origin.messageManager;
 
   if (origin instanceof Ci.nsIMessageSender)
     return new LightWeightPort(origin);
   else
     return null;
 }
 
@@ skipping 52 matching lines @@
   {
     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};
+    let response = {callbackID, payload};
     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")
+    if (isPromise(result))
     {
       // 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 => {
+      result.then(result =>
+      {
         this._sendResponse(sender, callbackID, result)
-      }, e => {
+      }, 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))
+    let callbackData = this._responseCallbacks.get(callbackID);
+    if (!callbackData)
       return;
 
-    let [callback, messageName, responses, expectedResponses] =
-        this._responseCallbacks.get(callbackID);
-    responses.push(payload);
-    if (responses.length == expectedResponses)
+    let [callback, processor, expectedResponses] = callbackData;
+
+    try
+    {
+      processor.add(payload);
+    }
+    catch (e)
+    {
+      Cu.reportError(e);
+    }
+
+    callbackData[2] = --expectedResponses;
+    if (expectedResponses <= 0)
     {
       this._responseCallbacks.delete(callbackID);
-      callback(flattenResponses(responses, messageName));
+      callback(processor.value);
     }
   },
 
   _dispatch: function(messageName, payload, sender)
   {
     let callbacks = this._callbacks.get(messageName);
     if (!callbacks)
       return undefined;
 
     callbacks = callbacks.slice();
-    let responses = [];
+    let processor = new ResponseProcessor(messageName);
     for (let callback of callbacks)
     {
       try
       {
-        responses.push(callback(payload, sender));
+        processor.add(callback(payload, sender));
       }
       catch (e)
       {
         Cu.reportError(e);
       }
     }
-    return flattenResponses(responses, messageName);
+    return processor.value;
   },
 
   /**
    * 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
    *      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).
    */
 
   /**
    * Adds a handler for the specified message.
    * @param {string} messageName message that would trigger the callback
    * @param {Port~messageHandler} callback
    */
   on: function(messageName, callback)
   {
-    if (!this._callbacks.has(messageName))
-      this._callbacks.set(messageName, []);
-
     let callbacks = this._callbacks.get(messageName);
-    if (callbacks.indexOf(callback) < 0)
+    if (callbacks)
       callbacks.push(callback);
+    else
+      this._callbacks.set(messageName, [callback]);
   },
 
   /**
    * Removes a handler for the specified message.
    * @param {string} messageName message that would trigger the callback
    * @param {Port~messageHandler} callback
    */
   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);

[Sebastian Noack, 2016/03/16 11:53:32]

If a listener is temporarily added we leak the key and empty array. But I'm not
sure either whether this is worth to be addressed.

[Wladimir Palant, 2016/03/16 13:12:54]

IMHO not really, so far we don't have a single call to .off() in the codebase -
our listeners are usually permanent. Should this change at some point, it might
be that the temporary listeners are always being added for the same message in
which case the current behavior is actually better.

   },
 
   /**
    * Sends a message.
    * @param {string} messageName message identifier
    * @param [payload] data to attach to the message
    */
   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) => {
+    return new Promise((resolve, reject) =>
+    {
       this._responseCallbacks.set(callbackID,
-          [resolve, messageName, [], expectedResponses]);
+          [resolve, new ResponseProcessor(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)
@@ skipping 12 matching lines @@
 catch (e)
 {
   // Parent
   messageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]
                      .getService(Ci.nsIMessageListenerManager);
 }
 
 let port = new Port(messageManager);
 onShutdown.add(() => port.disconnect());
 exports.port = port;