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

RESOLVED FIXED in Firefox 36

Status

Hello (Loop)
Client
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pkerr, Assigned: pkerr)

Tracking

unspecified
mozilla38
Points:
3
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

(Whiteboard: [p=2, gecko][loop-uplift])

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8532634 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

WIP
(Assignee)

Updated

3 years ago
Attachment #8532634 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 2

3 years ago
Created attachment 8532728 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

WIP - bug fixes, clean up nits
(Assignee)

Updated

3 years ago
Attachment #8532634 - Attachment is obsolete: true
Attachment #8532634 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8532728 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8532728 - Flags: review?(MattN+bmo)
Attachment #8532728 - Flags: feedback?(MattN+bmo)
Attachment #8532728 - Flags: feedback?
(Assignee)

Comment 3

3 years ago
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.
Attachment #8532728 - Flags: review?(MattN+bmo)
Attachment #8532728 - Flags: feedback?
(Assignee)

Comment 4

3 years ago
Created attachment 8537450 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Update: fixed unit tests and fixed bugs uncovered by functional tests
(Assignee)

Updated

3 years ago
Attachment #8532728 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
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.
Attachment #8537450 - Flags: review?(dmose)
Attachment #8537450 - Flags: review?(MattN+bmo)
(Assignee)

Comment 6

3 years ago
Created attachment 8537496 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

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

Updated

3 years ago
Attachment #8537450 - Attachment is obsolete: true
Attachment #8537450 - Flags: review?(dmose)
Attachment #8537450 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Attachment #8537496 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Depends on: 1028869

Updated

3 years ago
backlog: --- → Fx36+

Updated

3 years ago
Attachment #8537496 - Flags: review?(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.

Updated

3 years ago
Attachment #8537496 - Flags: review?(MattN+bmo)
(Assignee)

Comment 8

3 years ago
Created attachment 8545510 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

fix bit-rot
(Assignee)

Updated

3 years ago
Attachment #8537496 - Attachment is obsolete: true
Attachment #8537496 - Flags: review?(dmose)
(Assignee)

Updated

3 years ago
Attachment #8545510 - Flags: review?(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.
Attachment #8545510 - Flags: review?(dmose) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8546131 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment
(Assignee)

Updated

3 years ago
Attachment #8545510 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
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+
Attachment #8546131 - Flags: review+
(Assignee)

Comment 12

3 years ago
Created attachment 8546855 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment
(Assignee)

Updated

3 years ago
Attachment #8546131 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Carry forward r+ dmose.
Attachment #8546855 - Flags: review+
(Assignee)

Comment 14

3 years ago
Created attachment 8549015 [details] [diff] [review]
replace pushURL registered with LoopServer whenever PushServer does a re-assignment

Update and remove bit-rot
(Assignee)

Updated

3 years ago
Attachment #8546855 - Attachment is obsolete: true
(Assignee)

Comment 15

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

Carry forward r+ dmose.
Attachment #8549015 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9496d6ec12c3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Updated

3 years ago
Whiteboard: [p=2, gecko] → [p=2, gecko][loop-uplift]
(Assignee)

Comment 18

3 years ago
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
Attachment #8549015 - Flags: approval-mozilla-beta?
Attachment #8549015 - Flags: approval-mozilla-aurora?
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
Attachment #8549015 - Flags: approval-mozilla-beta?
Attachment #8549015 - Flags: approval-mozilla-beta+
Attachment #8549015 - Flags: approval-mozilla-aurora?
Attachment #8549015 - Flags: approval-mozilla-aurora+
Keywords: qawanted
https://hg.mozilla.org/releases/mozilla-aurora/rev/396932c3a4aa
https://hg.mozilla.org/releases/mozilla-beta/rev/be5eee20bba5
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Flags: in-testsuite+
Looks like this is covered well by automated testing, so setting as qe-verify-.
Flags: qe-verify-
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.