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

RESOLVED FIXED in Firefox 36

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pkerr, Assigned: pkerr)

Tracking

unspecified
mozilla38
Points:
3
Dependency tree / graph
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

5 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

Updated

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

Comment 2

5 years ago
WIP - bug fixes, clean up nits
Assignee

Updated

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

Updated

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

Updated

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

Comment 3

5 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

5 years ago
Update: fixed unit tests and fixed bugs uncovered by functional tests
Assignee

Updated

5 years ago
Attachment #8532728 - Attachment is obsolete: true
Assignee

Comment 5

5 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

5 years ago
fixed bitrot against fx-team repo. Removed left-over debug prints, etc.
Assignee

Updated

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

Updated

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

Updated

5 years ago
Depends on: 1028869

Updated

5 years ago
backlog: --- → Fx36+

Updated

5 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.
Attachment #8537496 - Flags: review?(MattN+bmo)
Assignee

Updated

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

Updated

5 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

Updated

5 years ago
Attachment #8545510 - Attachment is obsolete: true
Assignee

Comment 11

5 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

Updated

5 years ago
Attachment #8546131 - Attachment is obsolete: true
Assignee

Comment 13

5 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

Updated

5 years ago
Attachment #8546855 - Attachment is obsolete: true
Assignee

Comment 15

5 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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee

Updated

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

Comment 18

5 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?
Attachment #8549015 - Flags: approval-mozilla-beta?
Attachment #8549015 - Flags: approval-mozilla-beta+
Attachment #8549015 - Flags: approval-mozilla-aurora?
Attachment #8549015 - Flags: approval-mozilla-aurora+
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.