1108028 - Replace push URL registered with LoopServer whenever the PushServer re-assigns a channel's URL

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Paul Kerr [:pkerr]]

For a variety of reasons, such as after a client re-connect operation or a PushServer internal issue, the PushServer can assign a new push URL to one of the notification channels created by the Loop client. The MozPushHandler module will handle the re-registration and then call the onRegistered callback for an effected channel. At this point, the MozLoopService module does nothing after the first callback and the PushServer continues to use the old, now non-functional, push URL. MozLoopService must be updated to register the new push URL with the LoopServer and remove the old URL.

Comment 1

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

WIP

Comment 2

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

WIP - bug fixes, clean up nits

Comment 3

[Paul Kerr [:pkerr]]

Comment on attachment 8532728 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Found a race condition when testing another bug. Will repost for review after I have a fix.

Comment 4

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Update: fixed unit tests and fixed bugs uncovered by functional tests

Comment 5

[Paul Kerr [:pkerr]]

Comment on attachment 8537450 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

I need one reviewer, whoever has the time first.

Comment 6

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

fixed bitrot against fx-team repo. Removed left-over debug prints, etc.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Since this is a big patch, I think both knowledge-sharing and the patch itself are likely to benefit from pair review, so pkerr and I are hoping that his voice will be in shape to do that tomorrow.  If not, we'll find an alternate strategy.

Comment 8

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

fix bit-rot

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8545510 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Review of attachment 8545510 [details] [diff] [review]:
-----------------------------------------------------------------

I suspect it'd be more clear to refer to the things that are currently named "callbacks" as "events" or "notification callbacks" something else that more explicitly connates that it's possible to have them called repeatedly.  If this is an easy change to make before landing, that'd be great to do, I think.  If not, an XXX comment suggesting a future such change would be great.

r=dmose with the appropriate nits fixed.  Thanks for all this great cleanup!

::: browser/components/loop/MozLoopPushHandler.jsm
@@ +446,4 @@
>     /**
>      * Start registration of a PushServer notification channel.
>      * connection, it will automatically say hello and register the channel
>      * id with the server.

Suggest mentioning that the method is idempotent, and is so because the channel list is an attribute set.

@@ +450,5 @@
>      *
>      * 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.

For future reference, usejsdoc.org defines a machine-parsable syntax for callback params, I think.

@@ +497,5 @@
>      this._registerChannels();
>    },
>  
> +   /**
> +   * Un-register a notification channel

Add a comment that this is here to make testing easier so that this doesn't get inadvertently removed.

::: browser/components/loop/MozLoopService.jsm
@@ +357,5 @@
>    },
>  
>    /**
>     * Starts registration of Loop with the push server, and then will register
>     * with the Loop server. It will return early if already registered.

Please clarify that early return only happens when all pending things have completed.

@@ +386,5 @@
> +        return this.createNotificationChannel(
> +          sessionType == LOOP_SESSION_TYPE.GUEST ?
> +            MozLoopService.channelIDs.roomsGuest :
> +            MozLoopService.channelIDs.roomsFxA,
> +          sessionType, "rooms", roomsPushNotification)});

If you could avoid inlining the ternary operator into the actual function parameters, I think that would improve readability by making avoiding requiring extra mental state.

Maybe also consider use the LOOP_SESSION_TYPE contants as the key into channelIds.

@@ +405,5 @@
> +
> +  /**
> +   * 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.

This last sentence is no longer true; if you could either remove it or fix it to be better, that'd be great.

@@ +409,5 @@
> +   * 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.

Add an XXX comment here or somewhere else relevant about the unclear stuff about this retry stuff.

@@ +410,5 @@
> +   *
> +   * @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}

Please document what the promise resolves to.

@@ +412,5 @@
> +   * @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, serviceType, pushURL, retry = true) {

serviceType needs documentation

@@ +420,5 @@
> +    }
> +
> +    let pushURLs = this.pushURLs.get(sessionType);
> +
> +    if (!pushURLs) {

Some sort of comment here would be helpful.

::: browser/components/loop/test/xpcshell/test_looppush_initialize.js
@@ +74,2 @@
>            },
> +          function(version, id) {return});

This would be readable if it were named.  Maybe even in the head.

::: browser/components/loop/test/xpcshell/test_loopservice_token_invalid.js
@@ +33,5 @@
>  
>    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, 4);

Please comment this number, and, if you want, compute it too.

Comment 10

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Comment 11

[Paul Kerr [:pkerr]]

Comment on attachment 8546131 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Incorporate review feedback, carry forward dmose r+

Comment 12

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Comment 13

[Paul Kerr [:pkerr]]

Comment on attachment 8546855 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Carry forward r+ dmose.

Comment 14

[Paul Kerr [:pkerr]]

Attachment: replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Update and remove bit-rot

Comment 15

[Paul Kerr [:pkerr]]

Comment on attachment 8549015 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Carry forward r+ dmose.

Comment 16

[Paul Kerr [:pkerr]]

https://hg.mozilla.org/integration/fx-team/rev/9496d6ec12c3

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9496d6ec12c3

Comment 18

[Paul Kerr [:pkerr]]

Comment on attachment 8549015 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Approval Request Comment
[Feature/regressing bug #]: this bug (1108028)

[User impact if declined]: When a service connection is re-established with the PushServer by the Hello client after a network disconnect, the PushServer can, and does after an internal expiration time, generate a new set of Push URLs for the client. If these new Push URLs are not registered with the LoopServer the client cannot be notified of new service information. This will leave the Hello appearing to be live to the user but unable to receive notifications from the LoopServer.

[Describe test coverage new/current, TBPL]: Patch is currently on mozilla-central. Automated tests exist to test PushServer handler regression in the client and to the new functionality added by this bug. Manual testing has been performed for various network issues that can be encountered.

[Risks and why]: Low. A full set of unit and functional tests already exist to test the PushServer handler module of the Hello client. If this patch fails to register the new set of Push URLs with the LoopServer, the user is in no worse shape than the current functionality and a browser restart will complete a PushURL registration and clear the useless URLs from the LoopServer.

[String/UUID change made/needed]: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/396932c3a4aa
https://hg.mozilla.org/releases/mozilla-beta/rev/be5eee20bba5

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Looks like this is covered well by automated testing, so setting as qe-verify-.