Bug 1108028 - Replace pushURL registered with LoopServer whenever PushServer does a re-assignment. r=dmose, a=sledru
authorPaul Kerr [:pkerr] <pkerr@mozilla.com>
Tue, 16 Dec 2014 14:14:06 -0800
changeset 242923 be5eee20bba5
parent 242922 ee09df3331d0
child 242924 2b2b697613eb
push id4341
push userryanvm@gmail.com
push date2015-01-20 15:33 +0000
treeherdermozilla-beta@595835cd60a0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdmose, sledru
bugs1108028
milestone36.0
Bug 1108028 - Replace pushURL registered with LoopServer whenever PushServer does a re-assignment. r=dmose, a=sledru
browser/components/loop/MozLoopPushHandler.jsm
browser/components/loop/MozLoopService.jsm
browser/components/loop/test/mochitest/browser_fxa_login.js
browser/components/loop/test/mochitest/head.js
browser/components/loop/test/xpcshell/head.js
browser/components/loop/test/xpcshell/test_looppush_initialize.js
browser/components/loop/test/xpcshell/test_loopservice_registration.js
browser/components/loop/test/xpcshell/test_loopservice_registration_retry.js
browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
--- a/browser/components/loop/MozLoopPushHandler.jsm
+++ b/browser/components/loop/MozLoopPushHandler.jsm
@@ -439,23 +439,26 @@ let MozLoopPushHandler = {
     // the websocket referencing it as an nsIWebSocketListener is released.
     this.channels.clear();
     this.uaID = undefined;
     this.pushUrl = undefined;
     this.pushServerUri = undefined;
   },
 
    /**
-    * Start registration of a PushServer notification channel.
-    * connection, it will automatically say hello and register the channel
-    * id with the server.
+    * Assign a channel to be registered with the PushServer
+    * This channel will be registered when a connection to the PushServer
+    * has been established or re-registered after a connection has been lost
+    * and re-established. Calling this more than once for the same channel
+    * has no additional effect.
     *
     * onRegistered callback parameters:
     * - {String|null} err: Encountered error, if any
     * - {String} url: The push url obtained from the server
+    * - {String} channelID The channelID on which the notification was sent.
     *
     * onNotification parameters:
     * - {String} version The version string received from the push server for
     *                    the notification.
     * - {String} channelID The channelID on which the notification was sent.
     *
     * @param {String} channelID Channel ID to use in registration.
     *
@@ -473,30 +476,55 @@ let MozLoopPushHandler = {
     if (!channelID || !onRegistered || !onNotification) {
       throw new Error("missing required parameter(s):" +
                       (channelID ? "" : " channelID") +
                       (onRegistered ? "" : " onRegistered") +
                       (onNotification ? "" : " onNotification"));
     }
 
     consoleLog.info("PushHandler: channel registration: ", channelID);
-    // If the channel is already registered, callback with an error immediately
-    // so we don't leave code hanging waiting for an onRegistered callback.
     if (this.channels.has(channelID)) {
-      consoleLog.error("PushHandler: channel already registered");
-      onRegistered("error: channel already registered: " + channelID);
+      // If this channel has an active registration with the PushServer
+      // call the onRegister callback with the URL.
+      if (this.registeredChannels[channelID]) {
+        onRegistered(null, this.registeredChannels[channelID], channelID);
+      }
+      // Update the channel record.
+      this.channels.set(channelID, {onRegistered: onRegistered,
+                        onNotification: onNotification});
       return;
     }
 
     this.channels.set(channelID, {onRegistered: onRegistered,
                                   onNotification: onNotification});
-
     this._channelsToRegister.push(channelID);
     this._registerChannels();
   },
+  
+  /**
+   * Un-register a notification channel.
+   *
+   * @param {String} channelID Notification channel ID.
+   */
+  unregister: function(channelID) {
+    consoleLog.info("MozLoopPushHandler: un-register channel ", channelID);
+    if (!this.channels.has(channelID)) {
+      return;
+    }
+
+    this.channels.delete(channelID);
+
+    if (this.registeredChannels[channelID]) {
+      delete this.registeredChannels[channelID];
+      if (this.connectionState === CONNECTION_STATE_OPEN) {
+        this._pushSocket.send({messageType: "unregister",
+                               channelID: channelID});
+      }
+    }
+  },
 
   /**
    * Handles the start of the websocket stream.
    * Sends a hello message to the server.
    *
    */
   _onStart: function() {
     consoleLog.info("PushHandler: websocket open, sending 'hello' to PushServer");
@@ -613,17 +641,17 @@ let MozLoopPushHandler = {
     // If a new uaID is received, then any previous channel registrations
     // are no longer valid and a Registration request is generated.
     if (this.uaID !== aMsg.uaid) {
       consoleLog.log("PushHandler: registering all channels");
       this.uaID = aMsg.uaid;
       // Re-register all channels.
       this._channelsToRegister = [...this.channels.keys()];
       this.registeredChannels = {};
-     }
+    }
     // Allow queued registrations to start (or all if cleared above).
     this._registerChannels();
   },
 
   /**
    * Handles notification message.
    *
    * This method will parse the Array of updates and trigger
@@ -698,28 +726,26 @@ let MozLoopPushHandler = {
                         msg.channelID);
         // retry the registration request after a suitable delay
         this._retryManager.retry(() => this._sendRegistration(msg.channelID));
         break;
 
       case 409:
         consoleLog.error("PushHandler: received a 409 response from the PushServer: ",
                          msg.channelID);
-        this.channels.get(this._pendingChannelID).onRegistered(
-          "error: PushServer ChannelID already in use: " + msg.channelID);
+        this.channels.get(this._pendingChannelID).onRegistered("409");
         // Remove this channel from the channel list.
         this.channels.delete(this._pendingChannelID);
         this._registerNext();
         break;
 
       default:
         consoleLog.error("PushHandler: received error ", msg.status,
                          " from the PushServer: ", msg.channelID);
-        this.channels.get(this._pendingChannelID).onRegistered(
-          "error: PushServer registration failure, status = " + msg.status);
+        this.channels.get(this._pendingChannelID).onRegistered(msg.status);
         this.channels.delete(this._pendingChannelID);
         this._registerNext();
         break;
     }
   },
 
   /**
    * Attempts to open a websocket.
--- a/browser/components/loop/MozLoopService.jsm
+++ b/browser/components/loop/MozLoopService.jsm
@@ -119,20 +119,20 @@ let gConversationWindowData = new Map();
  * Internal helper methods and state
  *
  * The registration is a two-part process. First we need to connect to
  * and register with the push server. Then we need to take the result of that
  * and register with the Loop server.
  */
 let MozLoopServiceInternal = {
   conversationContexts: new Map(),
+  pushURLs: new Map(),
 
   mocks: {
     pushHandler: undefined,
-    webSocket: undefined,
   },
 
   /**
    * The current deferreds for the registration processes. This is set if in progress
    * or the registration was successful. This is null if a registration attempt was
    * unsuccessful.
    */
   deferredRegistrations: new Map(),
@@ -320,111 +320,209 @@ let MozLoopServiceInternal = {
     }
   },
 
   get errors() {
     return gErrors;
   },
 
   /**
-   * Get endpoints with the push server and register for notifications.
-   * This should only be called from promiseRegisteredWithServers to prevent reentrancy.
+   * Create a notification channel between the LoopServer and this client
+   * via a PushServer. Once created, any subsequent changes in the pushURL
+   * assigned by the PushServer will be communicated to the LoopServer.
+   * with the Loop server. It will return early if already registered.
    *
+   * @param {String} channelID Unique identifier for the notification channel
+   *                 registered with the PushServer.
    * @param {LOOP_SESSION_TYPE} sessionType
-   * @return {Promise} resolves with all push endpoints
-   *                   rejects if any of the push registrations failed
+   * @param {String} serviceType Either 'calls' or 'rooms'.
+   * @param {Function} onNotification Callback function that will be associated
+   *                   with this channel from the PushServer.
+   * @returns {Promise} A promise that is resolved with no params on completion, or
+   *                    rejected with an error code or string.
    */
-  promiseRegisteredWithPushServer: function(sessionType) {
-    if (!this.deferredRegistrations.has(sessionType)) {
-      return Promise.reject(new Error("promiseRegisteredWithPushServer must be called while there is a " +
-                            "deferred in deferredRegistrations in order to prevent reentrancy"));
-    }
-    // Wrap push notification registration call-back in a Promise.
-    function registerForNotification(channelID, onNotification) {
-      log.debug("registerForNotification", channelID);
-      return new Promise((resolve, reject) => {
-        function onRegistered(error, pushUrl) {
-          log.debug("registerForNotification onRegistered:", error, pushUrl);
-          if (error) {
-            reject(Error(error));
-          } else {
-            resolve(pushUrl);
-          }
+  createNotificationChannel: function(channelID, sessionType, serviceType, onNotification) {
+    log.debug("createNotificationChannel", channelID, sessionType, serviceType);
+    // Wrap the push notification registration callback in a Promise.
+    return new Promise((resolve, reject) => {
+      let onRegistered = (error, pushURL, channelID) => {
+        log.debug("createNotificationChannel onRegistered:", error, pushURL, channelID);
+        if (error) {
+          reject(Error(error));
+        } else {
+          resolve(this.registerWithLoopServer(sessionType, serviceType, pushURL));
         }
+      }
 
-        // If we're already registered, resolve with the existing push URL
-        let pushURL = MozLoopServiceInternal.pushHandler.registeredChannels[channelID];
-        if (pushURL) {
-          log.debug("Using the existing push endpoint for channelID:", channelID);
-          resolve(pushURL);
-          return;
-        }
-
-        MozLoopServiceInternal.pushHandler.register(channelID, onRegistered, onNotification);
-      });
-    }
-
-    let options = this.mocks.webSocket ? { mockWebSocket: this.mocks.webSocket } : {};
-    this.pushHandler.initialize(options);
-
-    if (sessionType == LOOP_SESSION_TYPE.GUEST) {
-      let callsRegGuest = registerForNotification(MozLoopService.channelIDs.callsGuest,
-                                                  LoopCalls.onNotification);
-
-      let roomsRegGuest = registerForNotification(MozLoopService.channelIDs.roomsGuest,
-                                                  roomsPushNotification);
-      return Promise.all([callsRegGuest, roomsRegGuest]);
-    } else if (sessionType == LOOP_SESSION_TYPE.FXA) {
-      let callsRegFxA = registerForNotification(MozLoopService.channelIDs.callsFxA,
-                                                LoopCalls.onNotification);
-
-      let roomsRegFxA = registerForNotification(MozLoopService.channelIDs.roomsFxA,
-                                                roomsPushNotification);
-      return Promise.all([callsRegFxA, roomsRegFxA]);
-    }
-
-    return Promise.reject(new Error("promiseRegisteredWithPushServer: Invalid sessionType"));
+      this.pushHandler.register(channelID, onRegistered, onNotification);
+    });
   },
 
   /**
-   * Starts registration of Loop with the push server, and then will register
-   * with the Loop server. It will return early if already registered.
+   * Starts registration of Loop with the PushServer and the LoopServer.
+   * Successful PushServer registration will automatically trigger the registration
+   * of the PushURL returned by the PushServer with the LoopServer. If the registration
+   * chain has already been set up, this function will simply resolve.
    *
    * @param {LOOP_SESSION_TYPE} sessionType
    * @returns {Promise} a promise that is resolved with no params on completion, or
    *          rejected with an error code or string.
    */
   promiseRegisteredWithServers: function(sessionType = LOOP_SESSION_TYPE.GUEST) {
+    if (sessionType !== LOOP_SESSION_TYPE.GUEST && sessionType !== LOOP_SESSION_TYPE.FXA) {
+      return Promise.reject(new Error("promiseRegisteredWithServers: Invalid sessionType"));
+    }
+
     if (this.deferredRegistrations.has(sessionType)) {
-      log.debug("promiseRegisteredWithServers: registration already completed or in progress:", sessionType);
-      return this.deferredRegistrations.get(sessionType).promise;
+      log.debug("promiseRegisteredWithServers: registration already completed or in progress:",
+                sessionType);
+      return this.deferredRegistrations.get(sessionType);
     }
 
-    let result = null;
-    let deferred = Promise.defer();
-    log.debug("assigning to deferredRegistrations for sessionType:", sessionType);
-    this.deferredRegistrations.set(sessionType, deferred);
+    let options = this.mocks.webSocket ? { mockWebSocket: this.mocks.webSocket } : {};
+    this.pushHandler.initialize(options); // This can be called more than once.
 
-    // We grab the promise early in case one of the callers below delete it from the map.
-    result = deferred.promise;
+    let callsID = sessionType == LOOP_SESSION_TYPE.GUEST ?
+          MozLoopService.channelIDs.callsGuest :
+          MozLoopService.channelIDs.callsFxA,
+        roomsID = sessionType == LOOP_SESSION_TYPE.GUEST ?
+          MozLoopService.channelIDs.roomsGuest :
+          MozLoopService.channelIDs.roomsFxA;
 
-    this.promiseRegisteredWithPushServer(sessionType).then(() => {
-      return this.registerWithLoopServer(sessionType);
-    }).then(() => {
-      deferred.resolve("registered to status:" + sessionType);
-      // No need to clear the promise here, everything was good, so we don't need
-      // to re-register.
-    }, error => {
-      log.error("Failed to register with Loop server with sessionType " + sessionType, error);
-      deferred.reject(error);
+    let regPromise = this.createNotificationChannel(
+      callsID, sessionType, "calls", LoopCalls.onNotification).then(() => {
+        return this.createNotificationChannel(
+          roomsID, sessionType, "rooms", roomsPushNotification)});
+
+    log.debug("assigning to deferredRegistrations for sessionType:", sessionType);
+    this.deferredRegistrations.set(sessionType, regPromise);
+
+    // Do not return the new Promise generated by this catch() invocation.
+    // This will be called along with any other onReject function attached to regPromise.
+    regPromise.catch((error) => {
+      log.error("Failed to register with Loop server with sessionType ", sessionType, error);
       this.deferredRegistrations.delete(sessionType);
       log.debug("Cleared deferredRegistration for sessionType:", sessionType);
     });
 
-    return result;
+    return regPromise;
+  },
+
+  /**
+   * Registers with the Loop server either as a guest or a FxA user.
+   *
+   * @private
+   * @param {LOOP_SESSION_TYPE} sessionType The type of session e.g. guest or FxA
+   * @param {String} serviceType: "rooms" or "calls"
+   * @param {Boolean} [retry=true] Whether to retry if authentication fails.
+   * @return {Promise} resolves to pushURL or rejects with an Error
+   */
+  registerWithLoopServer: function(sessionType, serviceType, pushURL, retry = true) {
+    log.debug("registerWithLoopServer with sessionType:", sessionType, serviceType, retry);
+    if (!pushURL || !sessionType || !serviceType) {
+      return Promise.reject(new Error("Invalid or missing parameters for registerWithLoopServer"));
+    }
+
+    let pushURLs = this.pushURLs.get(sessionType);
+
+    // Create a blank URL record set if none exists for this sessionType.
+    if (!pushURLs) {
+      pushURLs = {calls: undefined, rooms: undefined};
+      this.pushURLs.set(sessionType, pushURLs);
+    }
+
+    if (pushURLs[serviceType] == pushURL) {
+      return Promise.resolve(pushURL);
+    }
+
+    let newURLs = {calls: pushURLs.calls,
+                   rooms: pushURLs.rooms};
+    newURLs[serviceType] = pushURL;
+
+    return this.hawkRequestInternal(sessionType, "/registration", "POST",
+                                    {simplePushURLs: newURLs}).then(
+      (response) => {
+        // If this failed we got an invalid token.
+        if (!this.storeSessionToken(sessionType, response.headers)) {
+          throw new Error("session-token-wrong-size");
+        }
+
+        // Record the new push URL
+        pushURLs[serviceType] = pushURL;
+        log.debug("Successfully registered with server for sessionType", sessionType);
+        this.clearError("registration");
+        return pushURL;
+      }, (error) => {
+        // There's other errors than invalid auth token, but we should only do the reset
+        // as a last resort.
+        if (error.code === 401) {
+          // Authorization failed, invalid token, we need to try again with a new token.
+          // XXX (pkerr) - Why is there a retry here? This will not clear up a hawk session
+          // token problem at this level.
+          if (retry) {
+            return this.registerWithLoopServer(sessionType, serviceType, pushURL, false);
+          }
+        }
+
+        log.error("Failed to register with the loop server. Error: ", error);
+        throw error;
+      }
+    );
+  },
+
+  /**
+   * Unregisters from the Loop server either as a guest or a FxA user.
+   *
+   * This is normally only wanted for FxA users as we normally want to keep the
+   * guest session with the device.
+   *
+   * NOTE: It is the responsibiliy of the caller the clear the session token
+   * after all of the notification classes: calls and rooms, for either
+   * Guest or FxA have been unregistered with the LoopServer.
+   *
+   * @param {LOOP_SESSION_TYPE} sessionType The type of session e.g. guest or FxA
+   * @return {Promise} resolving when the unregistration request finishes
+   */
+  unregisterFromLoopServer: function(sessionType) {
+    let prefType = Services.prefs.getPrefType(this.getSessionTokenPrefName(sessionType));
+    if (prefType == Services.prefs.PREF_INVALID) {
+      log.debug("already unregistered from LoopServer", sessionType);
+      return Promise.resolve("already unregistered");
+    }
+
+    let error,
+        pushURLs = this.pushURLs.get(sessionType),
+        callsPushURL = pushURLs ? pushURLs.calls : null,
+        roomsPushURL = pushURLs ? pushURLs.rooms : null;
+    this.pushURLs.delete(sessionType);
+
+    let unregister = (sessionType, pushURL) => {
+      if (!pushURL) {
+        return Promise.resolve("no pushURL of this type to unregister");
+      }
+
+      let unregisterURL = "/registration?simplePushURL=" + encodeURIComponent(pushURL);
+      return this.hawkRequestInternal(sessionType, unregisterURL, "DELETE").then(
+        () => {
+          log.debug("Successfully unregistered from server for sessionType = ", sessionType);
+          return "unregistered sessionType " + sessionType;
+        },
+        error => {
+          if (error.code === 401) {
+            // Authorization failed, invalid token. This is fine since it may mean we already logged out.
+            log.debug("already unregistered - invalid token", sessionType);
+            return "already unregistered, sessionType = " + sessionType;
+          }
+
+          log.error("Failed to unregister with the loop server. Error: ", error);
+          throw error;
+        });
+    }
+
+    return Promise.all([unregister(sessionType, callsPushURL), unregister(sessionType, roomsPushURL)]);
   },
 
   /**
    * Performs a hawk based request to the loop server - there is no pre-registration
    * for this request, if this is required, use hawkRequest.
    *
    * @param {LOOP_SESSION_TYPE} sessionType The type of session to use for the request.
    *                                        This is one of the LOOP_SESSION_TYPE members.
@@ -435,16 +533,17 @@ let MozLoopServiceInternal = {
    * @param {Boolean} [retryOn401=true] Whether to retry if authentication fails.
    * @returns {Promise}
    *        Returns a promise that resolves to the response of the API call,
    *        or is rejected with an error.  If the server response can be parsed
    *        as JSON and contains an 'error' property, the promise will be
    *        rejected with this JSON-parsed response.
    */
   hawkRequestInternal: function(sessionType, path, method, payloadObj, retryOn401 = true) {
+    log.debug("hawkRequestInternal: ", sessionType, path, method);
     if (!gHawkClient) {
       gHawkClient = new HawkClient(this.loopServerUri);
     }
 
     let sessionToken, credentials;
     try {
       sessionToken = Services.prefs.getCharPref(this.getSessionTokenPrefName(sessionType));
     } catch (x) {
@@ -468,25 +567,27 @@ let MozLoopServiceInternal = {
           newPayloadObj[property] = payloadObj[property];
         }
       };
       payloadObj = newPayloadObj;
     }
 
     let handle401Error = (error) => {
       if (sessionType === LOOP_SESSION_TYPE.FXA) {
-        MozLoopService.logOutFromFxA().then(() => {
+        return MozLoopService.logOutFromFxA().then(() => {
           // Set a user-visible error after logOutFromFxA clears existing ones.
           this.setError("login", error);
+          throw error;
         });
       } else if (this.urlExpiryTimeIsInFuture()) {
         // If there are no Guest URLs in the future, don't use setError to notify the user since
         // there isn't a need for a Guest registration at this time.
         this.setError("registration", error);
       }
+      throw error;
     };
 
     return gHawkClient.request(path, method, credentials, payloadObj).then(
       (result) => {
         this.clearError("network");
         return result;
       },
       (error) => {
@@ -494,22 +595,21 @@ let MozLoopServiceInternal = {
         this.clearSessionToken(sessionType);
         if (retryOn401 && sessionType === LOOP_SESSION_TYPE.GUEST) {
           log.info("401 and INVALID_AUTH_TOKEN - retry registration");
           return this.registerWithLoopServer(sessionType, false).then(
             () => {
               return this.hawkRequestInternal(sessionType, path, method, payloadObj, false);
             },
             () => {
-              handle401Error(error); //Process the original error that triggered the retry.
-              throw error;
+              return handle401Error(error); //Process the original error that triggered the retry.
             }
           );
         }
-        handle401Error(error);
+        return handle401Error(error);
       }
       throw error;
     });
   },
 
   /**
    * Performs a hawk based request to the loop server, registering if necessary.
    *
@@ -554,17 +654,16 @@ let MozLoopServiceInternal = {
       case LOOP_SESSION_TYPE.GUEST:
         suffix = "";
         break;
       case LOOP_SESSION_TYPE.FXA:
         suffix = ".fxa";
         break;
       default:
         throw new Error("Unknown LOOP_SESSION_TYPE");
-        break;
     }
     return "loop.hawk-session-token" + suffix;
   },
 
   /**
    * Used to store a session token from a request if it exists in the headers.
    *
    * @param {LOOP_SESSION_TYPE} sessionType The type of session to use for the request.
@@ -599,122 +698,16 @@ let MozLoopServiceInternal = {
    *                                        One of the LOOP_SESSION_TYPE members.
    */
   clearSessionToken: function(sessionType) {
     Services.prefs.clearUserPref(this.getSessionTokenPrefName(sessionType));
     log.debug("Cleared hawk session token for sessionType", sessionType);
   },
 
   /**
-   * Registers with the Loop server either as a guest or a FxA user. This method should only be
-   * called by promiseRegisteredWithServers since it prevents calling this while a registration is
-   * already in progress.
-   *
-   * @private
-   * @param {LOOP_SESSION_TYPE} sessionType The type of session e.g. guest or FxA
-   * @param {Boolean} [retry=true] Whether to retry if authentication fails.
-   * @return {Promise}
-   */
-  registerWithLoopServer: function(sessionType, retry = true) {
-    log.debug("registerWithLoopServer with sessionType:", sessionType);
-
-    let callsPushURL, roomsPushURL;
-    if (sessionType == LOOP_SESSION_TYPE.FXA) {
-      callsPushURL = this.pushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA];
-      roomsPushURL = this.pushHandler.registeredChannels[MozLoopService.channelIDs.roomsFxA];
-    } else if (sessionType == LOOP_SESSION_TYPE.GUEST) {
-      callsPushURL = this.pushHandler.registeredChannels[MozLoopService.channelIDs.callsGuest];
-      roomsPushURL = this.pushHandler.registeredChannels[MozLoopService.channelIDs.roomsGuest];
-    }
-
-    if (!callsPushURL || !roomsPushURL) {
-      return Promise.reject(new Error("Invalid sessionType or missing push URLs for registerWithLoopServer: " + sessionType));
-    }
-
-    // create a registration payload with a backwards compatible attribute (simplePushURL)
-    // that will register only the calls notification.
-    let msg = {
-        simplePushURL: callsPushURL,
-        simplePushURLs: {
-          calls: callsPushURL,
-          rooms: roomsPushURL,
-        },
-    };
-    return this.hawkRequestInternal(sessionType, "/registration", "POST", msg, false)
-      .then((response) => {
-        // If this failed we got an invalid token.
-        if (!this.storeSessionToken(sessionType, response.headers)) {
-          return Promise.reject(new Error("session-token-wrong-size"));
-        }
-
-        log.debug("Successfully registered with server for sessionType", sessionType);
-        this.clearError("registration");
-        return undefined;
-      }, (error) => {
-        // There's other errors than invalid auth token, but we should only do the reset
-        // as a last resort.
-        if (error.code === 401) {
-          // Authorization failed, invalid token, we need to try again with a new token.
-          if (retry) {
-            return this.registerWithLoopServer(sessionType, false);
-          }
-        }
-
-        log.error("Failed to register with the loop server. Error: ", error);
-        let deferred = Promise.defer();
-        deferred.promise.then(() => {
-          log.debug("registration retry succeeded");
-        },
-        error => {
-          log.debug("registration retry failed");
-        });
-        this.setError("registration", error, () => MozLoopService.delayedInitialize(deferred));
-        throw error;
-      }
-    );
-  },
-
-  /**
-   * Unregisters from the Loop server either as a guest or a FxA user.
-   *
-   * This is normally only wanted for FxA users as we normally want to keep the
-   * guest session with the device.
-   *
-   * NOTE: It is the responsibiliy of the caller the clear the session token
-   * after all of the notification classes: calls and rooms, for either
-   * Guest or FxA have been unregistered with the LoopServer.
-   *
-   * @param {LOOP_SESSION_TYPE} sessionType The type of session e.g. guest or FxA
-   * @param {String} pushURL The push URL previously given by the push server.
-   *                         This may not be necessary to unregister in the future.
-   * @return {Promise} resolving when the unregistration request finishes
-   */
-  unregisterFromLoopServer: function(sessionType, pushURL) {
-    let prefType = Services.prefs.getPrefType(this.getSessionTokenPrefName(sessionType));
-    if (prefType == Services.prefs.PREF_INVALID) {
-      return Promise.resolve("already unregistered");
-    }
-
-    let unregisterURL = "/registration?simplePushURL=" + encodeURIComponent(pushURL);
-    return this.hawkRequestInternal(sessionType, unregisterURL, "DELETE")
-      .then(() => {
-        log.debug("Successfully unregistered from server for sessionType", sessionType);
-      },
-      error => {
-        if (error.code === 401) {
-          // Authorization failed, invalid token. This is fine since it may mean we already logged out.
-          return;
-        }
-
-        log.error("Failed to unregister with the loop server. Error: ", error);
-        throw error;
-      });
-  },
-
-  /**
    * A getter to obtain and store the strings for loop. This is structured
    * for use by l10n.js.
    *
    * @returns {Map} a map of element ids with localized string values
    */
   get localizedStrings() {
     if (gLocalizedStrings.size)
       return gLocalizedStrings;
@@ -1286,31 +1279,16 @@ this.MozLoopService = {
   /**
    * Returns a new GUID (UUID) in curly braces format.
    */
   generateUUID: function() {
     return uuidgen.generateUUID().toString();
   },
 
   /**
-   * Returns a new non-global id
-   *
-   * @param {Function} notUnique [optional] This function will be
-   *                   applied to test the generated id for uniqueness
-   *                   in the callers domain.
-   */
-  generateLocalID: function(notUnique = ((id) => {return false})) {
-    do {
-      var id = Date.now().toString(36) + Math.floor((Math.random() * 4096)).toString(16);
-    }
-    while (notUnique(id));
-    return id;
-  },
-
-  /**
    * Retrieves MozLoopService "do not disturb" value.
    *
    * @return {Boolean}
    */
   get doNotDisturb() {
     return MozLoopServiceInternal.doNotDisturb;
   },
 
@@ -1489,34 +1467,31 @@ this.MozLoopService = {
    * Logs the user out from FxA.
    *
    * Gracefully handles if the user is already logged out.
    *
    * @return {Promise} that resolves when the FxA logout flow is complete.
    */
   logOutFromFxA: Task.async(function*() {
     log.debug("logOutFromFxA");
-    let pushHandler = MozLoopServiceInternal.pushHandler;
-    let callsPushUrl = pushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA];
-    let roomsPushUrl = pushHandler.registeredChannels[MozLoopService.channelIDs.roomsFxA];
     try {
-      if (callsPushUrl) {
-        yield MozLoopServiceInternal.unregisterFromLoopServer(LOOP_SESSION_TYPE.FXA, callsPushUrl);
-      }
-      if (roomsPushUrl) {
-        yield MozLoopServiceInternal.unregisterFromLoopServer(LOOP_SESSION_TYPE.FXA, roomsPushUrl);
-      }
-    } catch (error) {
-      throw error;
-    } finally {
+      yield MozLoopServiceInternal.unregisterFromLoopServer(LOOP_SESSION_TYPE.FXA);
+    }
+    catch (err) {
+      throw err
+    }
+    finally {
       MozLoopServiceInternal.clearSessionToken(LOOP_SESSION_TYPE.FXA);
-
       MozLoopServiceInternal.fxAOAuthTokenData = null;
       MozLoopServiceInternal.fxAOAuthProfile = null;
       MozLoopServiceInternal.deferredRegistrations.delete(LOOP_SESSION_TYPE.FXA);
+      // Unregister with PushHandler so these push channels will not get re-registered
+      // if the connection is re-established by the PushHandler.
+      MozLoopServiceInternal.pushHandler.unregister(MozLoopService.channelIDs.callsFxA);
+      MozLoopServiceInternal.pushHandler.unregister(MozLoopService.channelIDs.roomsFxA);
 
       // Reset the client since the initial promiseFxAOAuthParameters() call is
       // what creates a new session.
       gFxAOAuthClient = null;
       gFxAOAuthClientPromise = null;
 
       // clearError calls notifyStatusChanged so should be done last when the
       // state is clean.
--- a/browser/components/loop/test/mochitest/browser_fxa_login.js
+++ b/browser/components/loop/test/mochitest/browser_fxa_login.js
@@ -37,26 +37,22 @@ function* checkFxA401() {
   loopPanel.hidePopup();
 }
 
 add_task(function* setup() {
   Services.prefs.setBoolPref("loop.gettingStarted.seen", true);
   MozLoopServiceInternal.mocks.pushHandler = mockPushHandler;
   // Normally the same pushUrl would be registered but we change it in the test
   // to be able to check for success on the second registration.
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA] = "https://localhost/pushUrl/fxa-calls";
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.roomsFxA] = "https://localhost/pushUrl/fxa-rooms";
 
   registerCleanupFunction(function* () {
     info("cleanup time");
     yield promiseDeletedOAuthParams(BASE_URL);
     Services.prefs.clearUserPref("loop.gettingStarted.seen");
     MozLoopServiceInternal.mocks.pushHandler = undefined;
-    delete mockPushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA];
-    delete mockPushHandler.registeredChannels[MozLoopService.channelIDs.roomsFxA];
 
     yield resetFxA();
     Services.prefs.clearUserPref(MozLoopServiceInternal.getSessionTokenPrefName(LOOP_SESSION_TYPE.GUEST));
   });
 });
 
 add_task(function* checkOAuthParams() {
   let params = {
@@ -159,17 +155,16 @@ add_task(function* invalidState() {
   yield loginPromise.catch((error) => {
     ok(error, "The login promise should be rejected due to invalid state");
   });
 });
 
 add_task(function* basicRegistrationWithoutSession() {
   yield resetFxA();
   yield promiseDeletedOAuthParams(BASE_URL);
-
   let caught = false;
   yield MozLoopServiceInternal.promiseFxAOAuthToken("code1", "state").catch((error) => {
     caught = true;
     is(error.code, 401, "Should have returned a 401");
   });
   ok(caught, "Should have caught the error requesting /token without a hawk session");
   yield checkFxA401();
 });
@@ -354,51 +349,44 @@ add_task(function* loginWithParams401() 
   });
 
   yield checkFxA401();
 });
 
 add_task(function* logoutWithIncorrectPushURL() {
   yield resetFxA();
   let pushURL = "http://www.example.com/";
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA] = pushURL;
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.roomsFxA] = pushURL;
-
   // Create a fake FxA hawk session token
   const fxASessionPref = MozLoopServiceInternal.getSessionTokenPrefName(LOOP_SESSION_TYPE.FXA);
   Services.prefs.setCharPref(fxASessionPref, "X".repeat(HAWK_TOKEN_LENGTH));
-
-  yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA);
+  yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, "calls", pushURL);
   let registrationResponse = yield promiseOAuthGetRegistration(BASE_URL);
   ise(registrationResponse.response.simplePushURLs.calls, pushURL, "Check registered push URL");
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA] = "http://www.example.com/invalid";
+  MozLoopServiceInternal.pushURLs.get(LOOP_SESSION_TYPE.FXA).calls = "http://www.example.com/invalid";
   let caught = false;
   yield MozLoopService.logOutFromFxA().catch((error) => {
     caught = true;
   });
   ok(caught, "Should have caught an error logging out with a mismatched push URL");
   checkLoggedOutState();
   registrationResponse = yield promiseOAuthGetRegistration(BASE_URL);
   ise(registrationResponse.response.simplePushURLs.calls, pushURL, "Check registered push URL wasn't deleted");
 });
 
 add_task(function* logoutWithNoPushURL() {
   yield resetFxA();
   let pushURL = "http://www.example.com/";
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA] = pushURL;
-
   // Create a fake FxA hawk session token
   const fxASessionPref = MozLoopServiceInternal.getSessionTokenPrefName(LOOP_SESSION_TYPE.FXA);
   Services.prefs.setCharPref(fxASessionPref, "X".repeat(HAWK_TOKEN_LENGTH));
 
-  yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA);
+  yield MozLoopServiceInternal.registerWithLoopServer(LOOP_SESSION_TYPE.FXA, "calls", pushURL);
   let registrationResponse = yield promiseOAuthGetRegistration(BASE_URL);
   ise(registrationResponse.response.simplePushURLs.calls, pushURL, "Check registered push URL");
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.callsFxA] = null;
-  mockPushHandler.registeredChannels[MozLoopService.channelIDs.roomsFxA] = null;
+  MozLoopServiceInternal.pushURLs.delete(LOOP_SESSION_TYPE.FXA);
   yield MozLoopService.logOutFromFxA();
   checkLoggedOutState();
   registrationResponse = yield promiseOAuthGetRegistration(BASE_URL);
   ise(registrationResponse.response.simplePushURLs.calls, pushURL, "Check registered push URL wasn't deleted");
 });
 
 add_task(function* loginWithRegistration401() {
   yield resetFxA();
--- a/browser/components/loop/test/mochitest/head.js
+++ b/browser/components/loop/test/mochitest/head.js
@@ -9,16 +9,17 @@ const {
 const {LoopCalls} = Cu.import("resource:///modules/loop/LoopCalls.jsm", {});
 const {LoopRooms} = Cu.import("resource:///modules/loop/LoopRooms.jsm", {});
 
 // Cache this value only once, at the beginning of a
 // test run, so that it doesn't pick up the offline=true
 // if offline mode is requested multiple times in a test run.
 const WAS_OFFLINE = Services.io.offline;
 
+
 var gMozLoopAPI;
 
 function promiseGetMozLoopAPI() {
   return new Promise((resolve, reject) => {
     let loopPanel = document.getElementById("loop-notification-panel");
     let btn = document.getElementById("loop-button");
 
     // Wait for the popup to be shown if it's not already, then we can get the iframe and
@@ -109,17 +110,16 @@ function promiseOAuthParamsSetup(baseURL
   });
 }
 
 function* resetFxA() {
   let global = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
   global.gHawkClient = null;
   global.gFxAOAuthClientPromise = null;
   global.gFxAOAuthClient = null;
-  MozLoopServiceInternal.deferredRegistrations.delete(LOOP_SESSION_TYPE.FXA);
   MozLoopServiceInternal.fxAOAuthProfile = null;
   MozLoopServiceInternal.fxAOAuthTokenData = null;
   const fxASessionPref = MozLoopServiceInternal.getSessionTokenPrefName(LOOP_SESSION_TYPE.FXA);
   Services.prefs.clearUserPref(fxASessionPref);
   MozLoopService.errors.clear();
   let notified = promiseObserverNotified("loop-status-changed");
   MozLoopServiceInternal.notifyStatusChanged();
   yield notified;
@@ -184,33 +184,45 @@ function getLoopString(stringID) {
  * This is used to fake push registration and notifications for
  * MozLoopService tests. There is only one object created per test instance, as
  * once registration has taken place, the object cannot currently be changed.
  */
 let mockPushHandler = {
   // This sets the registration result to be returned when initialize
   // is called. By default, it is equivalent to success.
   registrationResult: null,
-  registrationPushURL: null,
+  registrationPushURLs: {},
   notificationCallback: {},
   registeredChannels: {},
 
   /**
    * MozLoopPushHandler API
    */
   initialize: function(options = {}) {
     if ("mockWebSocket" in options) {
       this._mockWebSocket = options.mockWebSocket;
     }
+    this.registrationPushURLs[MozLoopService.channelIDs.callsGuest] =
+      "https://localhost/pushUrl/guest-calls";
+    this.registrationPushURLs[MozLoopService.channelIDs.roomsGuest] =
+      "https://localhost/pushUrl/guest-rooms";
+    this.registrationPushURLs[MozLoopService.channelIDs.callsFxA] =
+      "https://localhost/pushUrl/fxa-calls";
+    this.registrationPushURLs[MozLoopService.channelIDs.roomsFxA] =
+      "https://localhost/pushUrl/fxa-rooms";
   },
 
   register: function(channelId, registerCallback, notificationCallback) {
     this.notificationCallback[channelId] = notificationCallback;
-    this.registeredChannels[channelId] = this.registrationPushURL;
-    setTimeout(registerCallback(this.registrationResult, this.registrationPushURL, channelId), 0);
+    this.registeredChannels[channelId] = this.registrationPushURLs[channelId];
+    setTimeout(registerCallback(this.registrationResult, this.registeredChannels[channelId], channelId), 0);
+  },
+
+  unregister: function(channelID) {
+    return;
   },
 
   /**
    * Test-only API to simplify notifying a push notification result.
    */
   notify: function(version, chanId) {
     this.notificationCallback[chanId](version, chanId);
   }
--- a/browser/components/loop/test/xpcshell/head.js
+++ b/browser/components/loop/test/xpcshell/head.js
@@ -97,16 +97,20 @@ let mockPushHandler = {
   },
 
   register: function(channelId, registerCallback, notificationCallback) {
     this.notificationCallback[channelId] = notificationCallback;
     this.registeredChannels[channelId] = this.registrationPushURL;
     registerCallback(this.registrationResult, this.registrationPushURL, channelId);
   },
 
+  unregister: function(channelID) {
+    return;
+  },
+
   /**
    * Test-only API to simplify notifying a push notification result.
    */
   notify: function(version, chanId) {
     this.notificationCallback[chanId](version, chanId);
   }
 };
 
--- a/browser/components/loop/test/xpcshell/test_looppush_initialize.js
+++ b/browser/components/loop/test/xpcshell/test_looppush_initialize.js
@@ -63,29 +63,25 @@
                      "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);
+            Assert.equal(err, null, "Should return null for success");
+            Assert.equal(id, "chan-2", "Should have channel id = chan-2");
+            run_next_test();
           },
-          function(version, id) {
-            Assert.ok(false, "The 2nd onNotification callback shouldn't be called");
-        });
+          dummyCallback
+        );
       },
-      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();
-      });
+      dummyCallback
+    );
   });
 
   // Test that the PushHander will re-connect after the near-end disconnect.
   // The uaID is cleared to force re-registration of all notification channels.
   add_test(function test_reconnect_websocket() {
     MozLoopPushHandler.uaID = undefined;
     mockWebSocket.stop();
     // Previously registered onRegistration callbacks will fire and be checked (see above).
@@ -117,19 +113,18 @@
       function(err, url, id) {
         Assert.equal(++regCnt, 1, "onRegistered should only be called once");
         Assert.equal(err, null, "err should be null to indicate success");
         Assert.equal(url, kEndPointUrl, "Should return push server application URL");
         Assert.equal(id, "test-chan", "Should have channel id = test-chan");
         mockWebSocket.stop();
         setTimeout(run_next_test(), 0);
       },
-      function(version, id) {
-        return;
-      });
+      dummyCallback
+    );
   });
 
   add_test(function test_ping_websocket() {
     let pingReceived = false,
         socketClosed = false;
     mockWebSocket.defaultMsgHandler = (msg) => {
       pingReceived = true;
       // Do not send a ping response.
@@ -149,19 +144,18 @@
             run_next_test();
           }, () => {
             do_throw("should have closed the websocket");
           });
         }, () => {
           do_throw("should have sent ping");
         });
       },
-      function(version) {
-        return;
-      });
+      dummyCallback
+    );
   });
 
   add_test(function test_retry_pushurl() {
     MozLoopPushHandler.shutdown();
     loopServer.registerPathHandler("/push-server-config", (request, response) => {
       // The PushHandler should retry the request for the push-server-config for
       // each of these cases without throwing an error.
       let n = 0;
--- a/browser/components/loop/test/xpcshell/test_loopservice_registration.js
+++ b/browser/components/loop/test/xpcshell/test_loopservice_registration.js
@@ -35,20 +35,24 @@ add_test(function test_register_websocke
 
 add_test(function test_register_success() {
   mockPushHandler.registrationPushURL = kEndPointUrl;
   mockPushHandler.registrationResult = null;
 
   loopServer.registerPathHandler("/registration", (request, response) => {
     let body = CommonUtils.readBytesFromInputStream(request.bodyInputStream);
     let data = JSON.parse(body);
-    Assert.equal(data.simplePushURLs.calls, kEndPointUrl,
-                 "Should send correct calls push url");
-    Assert.equal(data.simplePushURLs.rooms, kEndPointUrl,
-                 "Should send correct rooms push url");
+    if (data.simplePushURLs.calls) {
+      Assert.equal(data.simplePushURLs.calls, kEndPointUrl,
+                   "Should send correct calls push url");
+    }
+    if (data.simplePushURLs.rooms) {
+      Assert.equal(data.simplePushURLs.rooms, kEndPointUrl,
+                   "Should send correct rooms push url");
+    }
 
     response.setStatusLine(null, 200, "OK");
     response.processAsync();
     response.finish();
   });
   MozLoopService.promiseRegisteredWithServers().then(() => {
     run_next_test();
   }, err => {
--- a/browser/components/loop/test/xpcshell/test_loopservice_registration_retry.js
+++ b/browser/components/loop/test/xpcshell/test_loopservice_registration_retry.js
@@ -25,37 +25,41 @@ add_test(function test_retry_after_faile
 
     // Remove the error
     mockPushHandler.registrationResult = null;
     mockPushHandler.registrationPushURL = kEndPointUrl;
 
     yield regError.friendlyDetailsButtonCallback();
     Assert.strictEqual(MozLoopService.errors.size, 0, "Check that the errors are gone");
     let deferredRegistrations = MozLoopServiceInternal.deferredRegistrations;
-    yield deferredRegistrations.get(LOOP_SESSION_TYPE.GUEST).promise.then(() => {
+    yield deferredRegistrations.get(LOOP_SESSION_TYPE.GUEST).then(() => {
       Assert.ok(true, "The retry of registration succeeded");
     },
     (error) => {
       Assert.ok(false, "The retry of registration should have succeeded");
     });
 
     run_next_test();
   }));
 });
 
 function run_test() {
   setupFakeLoopServer();
 
   loopServer.registerPathHandler("/registration", (request, response) => {
     let body = CommonUtils.readBytesFromInputStream(request.bodyInputStream);
     let data = JSON.parse(body);
-    Assert.equal(data.simplePushURLs.calls, kEndPointUrl,
-                 "Should send correct calls push url");
-    Assert.equal(data.simplePushURLs.rooms, kEndPointUrl,
-                 "Should send correct rooms push url");
+    if (data.simplePushURLs.calls) {
+      Assert.equal(data.simplePushURLs.calls, kEndPointUrl,
+                   "Should send correct calls push url");
+    }
+    if (data.simplePushURLs.rooms) {
+      Assert.equal(data.simplePushURLs.rooms, kEndPointUrl,
+                   "Should send correct rooms push url");
+    }
 
     response.setStatusLine(null, 200, "OK");
   });
 
   let nowSeconds = Date.now() / 1000;
   Services.prefs.setIntPref("loop.urlsExpiryTimeSeconds", nowSeconds + 60);
 
   do_register_cleanup(function() {
--- a/browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
+++ b/browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
@@ -29,17 +29,17 @@ add_test(function test_registration_inva
     }
     response.processAsync();
     response.finish();
   });
 
   MozLoopService.promiseRegisteredWithServers().then(() => {
     // Due to the way the time stamp checking code works in hawkclient, we expect a couple
     // of authorization requests before we reset the token.
-    Assert.equal(authorizationAttempts, 2);
+    Assert.equal(authorizationAttempts, 4); //hawk will repeat each registration attemtp twice: calls and rooms.
     Assert.equal(Services.prefs.getCharPref(LOOP_HAWK_PREF), fakeSessionToken2);
     run_next_test();
   }, err => {
     do_throw("shouldn't be a failure result: " + err);
   });
 });