Bug 1089919 - Callback with an error if a 2nd registration for MozLoopPushHandler happens for the same channel ID. r=mikedeboer
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Tue, 28 Oct 2014 10:42:27 +0100
changeset 212701 f26373e62bcb66229526c07d5d2cc644c7abe4db
parent 212700 cf419403a2eb60b5444908c2633e35318e21ddab
child 212702 3679bc784d1a1469dc5b0659b1b3d5adea07abe2
push id51042
push userryanvm@gmail.com
push dateTue, 28 Oct 2014 20:25:03 +0000
treeherdermozilla-inbound@53d84829b2b8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1089919
milestone36.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1089919 - Callback with an error if a 2nd registration for MozLoopPushHandler happens for the same channel ID. r=mikedeboer
browser/components/loop/MozLoopPushHandler.jsm
browser/components/loop/test/xpcshell/test_looppush_initialize.js
--- a/browser/components/loop/MozLoopPushHandler.jsm
+++ b/browser/components/loop/MozLoopPushHandler.jsm
@@ -104,30 +104,35 @@ let MozLoopPushHandler = {
     */
   register: function(channelID, onRegistered, onNotification) {
     if (!channelID || !onRegistered || !onNotification) {
       throw new Error("missing required parameter(s):"
                       + (channelID ? "" : " channelID")
                       + (onRegistered ? "" : " onRegistered")
                       + (onNotification ? "" : " onNotification"));
     }
-    // Only register new channels
-    if (!(channelID in this.channels)) {
-      this.channels[channelID] = {
-        onRegistered: onRegistered,
-        onNotification: onNotification
-      };
+
+    // If the channel is already registered, callback with an error immediately
+    // so we don't leave code hanging waiting for an onRegistered callback.
+    if (channelID in this.channels) {
+      onRegistered("error: channel already registered: " + channelID);
+      return;
+    }
 
-      // If registration is in progress, simply add to the work list.
-      // Else, re-start a registration cycle.
-      if (this._registrationID) {
-        this._channelsToRegister.push(channelID);
-      } else {
-        this._registerChannels();
-      }
+    this.channels[channelID] = {
+      onRegistered: onRegistered,
+      onNotification: onNotification
+    };
+
+    // If registration is in progress, simply add to the work list.
+    // Else, re-start a registration cycle.
+    if (this._registrationID) {
+      this._channelsToRegister.push(channelID);
+    } else {
+      this._registerChannels();
     }
   },
 
   /**
    * Listener method, handles the start of the websocket stream.
    * Sends a hello message to the server.
    *
    * @param {nsISupports} aContext Not used
--- a/browser/components/loop/test/xpcshell/test_looppush_initialize.js
+++ b/browser/components/loop/test/xpcshell/test_looppush_initialize.js
@@ -41,18 +41,50 @@
         Assert.equal(mockWebSocket.protocol, "push-notification",
                      "Should have the protocol set to push-notifications");
         mockWebSocket.notify(15);
       },
       function(version, id) {
         Assert.equal(version, 15, "Should have version number 15");
         Assert.equal(id, "chan-1", "Should have channel id = chan-1");
         run_next_test();
+      });
+  });
+
+  add_test(function test_register_twice_same_channel() {
+    MozLoopPushHandler.register(
+      "chan-2",
+      function(err, url, id) {
+        Assert.equal(err, null, "Should return null for success");
+        Assert.equal(url, kEndPointUrl, "Should return push server application URL");
+        Assert.equal(id, "chan-2", "Should have channel id = chan-2");
+        Assert.equal(mockWebSocket.uri.prePath, kServerPushUrl,
+                     "Should have the url from preferences");
+        Assert.equal(mockWebSocket.origin, kServerPushUrl,
+                     "Should have the origin url from preferences");
+        Assert.equal(mockWebSocket.protocol, "push-notification",
+                     "Should have the protocol set to push-notifications");
+
+        // Register again for the same channel
+        MozLoopPushHandler.register(
+          "chan-2",
+          function(err, url, id) {
+            Assert.notEqual(err, null, "Should have returned an error");
+            // Notify the first registration to make sure that still works.
+            mockWebSocket.notify(16);
+          },
+          function(version, id) {
+            Assert.ok(false, "The 2nd onNotification callback shouldn't be called");
+        });
       },
-      mockWebSocket);
+      function(version, id) {
+        Assert.equal(version, 16, "Should have version number 16");
+        Assert.equal(id, "chan-2", "Should have channel id = chan-2");
+        run_next_test();
+      });
   });
 
   add_test(function test_reconnect_websocket() {
     MozLoopPushHandler.uaID = undefined;
     MozLoopPushHandler.registeredChannels = {}; //Do this to force a new registration callback.
     mockWebSocket.stop();
   });