Closed Bug 994151 Opened 6 years ago Closed 6 years ago

Loop desktop client should wait to have a valid generated call url to start the SimplePush registration

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33
Blocking Flags:
backlog mlp+

People

(Reporter: alexis+bugs, Assigned: standard8)

References

Details

(Whiteboard: [mlp-server?][est:1d, p=1])

User Story

Todo:

- Add a preference, e.g. loop.active and set it only once a url has been obtained (so that random opening of the panel doesn't activate).

- On startup, if loop.active is set, then initialize the service.
-- Increase delay for the push notification registration to something like 5 seconds (so it isn't doing network as soon as the UI is up).

- If loop.active is not set, don't initialize the service.

- When generating a url if the service hasn't yet been initialized/registered then
-- initialize & register
-- get the url

Also need to account for:

- Errors in registration when getting a url
- In-progress registration

Handling automatic recovery of errors and warnings will be a separate bug.

Attachments

(4 files, 8 obsolete files)

26.04 KB, patch
dmose
: review+
Details | Diff | Splinter Review
4.43 KB, patch
dmose
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
dmose
: review+
Details | Review
46 bytes, text/x-github-pull-request
dmose
: review+
Details | Review
In order to not overwhelm the Simple Push server with connections that would not be useful, we should start the registration on the Simple Push server in a lazy way.

Registering the simple push server can be done only when a previous call URL was generated and is still valid, otherwise there is no need to do that registration step, so we can avoid traffic on the Simple Push server.
We should wait for bug 976109 before doing this, as the architecture there will influence what we do here.
Depends on: 976109
Alexis - Do we need this for MLP?  Can you ask the Push folks if they need this done before we land it in Nightly?  Thanks.
Flags: needinfo?(alexis+bugs)
Whiteboard: [mlp-server?]
That depends the amount of load we would generate in nightly, how many users to we expect?

SP had been specified to handle 1M concurrent connected clients, but the less we send there the less we will need to handle.
Flags: needinfo?(alexis+bugs)
I've heard that 10K users on Nightly is a safe assumption (i.e. we'll very likely have less -- especially at the beginning).  RT -- Do we have a better estimated for Alexis (based on metrics)?
Flags: needinfo?(rtestard)
The specific issue here is that in our current state *all* nightly users will be registering with the push server (via websockets) on startup. They will also be registering with the loop server.

If we implement this bug, then we'll reduce from the number of nightly users to however many users try it out at least once.
Just so I understand, if we implement that bug a user is registered on both push and Loop servers so long as they generate a URL and for the expiration duration of the URL or forever? If the browser is closed and re-opened the user is re-registered?
Flags: needinfo?(rtestard)
Whiteboard: [mlp-server?] → [mlp-server?][est:?]
Whiteboard: [mlp-server?][est:?] → [mlp-server?][est:3d]
Priority: -- → P3
backlog: --- → mlp+
This should really be more like a P2.  The server piece needs us to land this for MLP.
Priority: P3 → P2
Depends on: 1000007
No longer depends on: 976109
Whiteboard: [mlp-server?][est:3d] → [mlp-server?][est:1d, p=1]
Target Milestone: --- → mozilla32
Blocks: 1002416
Depends on: 1005041
User Story: (updated)
Priority: P2 → P1
No longer depends on: 1005041
We need this so that every Nightly user doesn't register with the Loop server when MLP lands.
Assignee: nobody → standard8
Depends on: 1008122
This is the first step to allowing us to delay the registration. It implements an ensureRegistered API on mozLoop, which gets called before asking the server for call-urls. If we are not registered (as in the patch with the commented out auto-registration lines), then it attempts to go and register, calling back up the tree when complete, and then allowing the call-url to be obtained.

If registration fails for any reason, the failure gets passed back up the call tree via the callbacks, so at least the user gets notified about it when they next try to generate a url.

The second part (still to be written) will handle only registering if necessary (if a call-url has been requested recently), and delaying the initial registration after startup a little bit longer.

I'm currently looking at some xpshell tests for MozLoopService, hence putting up for feedback whilst I try and do those.
Attachment #8423841 - Flags: feedback?(dmose)
This is the shared part of the changes necessary for checking registration when requesting the call-url.
Attachment #8423844 - Flags: feedback?(dmose)
Attachment #8423844 - Attachment description: Ensure the client is registered before requesting a call url → Part 1. Ensure the client is registered before requesting a call url
This is larger than I originally expected, due to the additional complexity of needing to sort out the failure routes.
Whiteboard: [mlp-server?][est:1d, p=1] → [mlp-server?][est:1d, p=2]
This is the second part, that makes us only register with the servers if the user has a call url that hasn't expired yet. Call urls could have different expiry times, so we store the latest expiry time.

Docs, tidy-up & tests still to come.
Attachment #8424727 - Flags: feedback?(dmose)
Updated part 1 to include some xpcshell tests for the registration process. For this we mock the websocket interface (there's no apparent pre-written xpcshell helpers for websocket), and we use a http server to driver the loop server part of the interface.

The split of testing types seems a bit uneven, but seems the simplest ways to the individual parts.

There's also some doc cleanup in this patch.

I think this is real close to review worthy now, but I want to check over the code one more time for cases I should be testing.
Attachment #8423841 - Attachment is obsolete: true
Attachment #8423841 - Flags: feedback?(dmose)
Attachment #8424865 - Flags: feedback?(dmose)
Updated for bitrot
Attachment #8424727 - Attachment is obsolete: true
Attachment #8424727 - Flags: feedback?(dmose)
Attachment #8424866 - Flags: feedback?(dmose)
Attachment #8424729 - Attachment description: Part 2 Only register with the servers if the user has obtained call urls that haven't expired yet → Part 2 (loop-client) Only register with the servers if the user has obtained call urls that haven't expired yet
Comment on attachment 8423844 [details] [diff] [review]
Part 1. Ensure the client is registered before requesting a call url

Looks good.  One tweak: there are two places where the jsdoc declares a callback signature with two args, but the actual code only calls back with one arg.
Attachment #8423844 - Flags: feedback?(dmose) → feedback+
Comment on attachment 8424866 [details] [diff] [review]
Part 2 Only register with the servers if the user has obtained call urls that haven't expired yet. v2

Seems reasonable.  A couple of nits:

The "log" logCallUrlExpiry seems confusing.  Perhaps "noteCallUrlExpiry".

It would also be nice to name variable names and functions and log statments involving times to include the units, in order to help prevent future coding thinkos.
Attachment #8424866 - Flags: feedback?(dmose) → feedback+
Attachment #8424729 - Flags: feedback?(dmose) → feedback+
I would suggest using separate functions for converting between time units.  As it stands, the code in various functions in jumping across levels of abstractions, which makes it harder to read.
(In reply to Mark Banner (:standard8) from comment #9)
>
> If registration fails for any reason, the failure gets passed back up the
> call tree via the callbacks, so at least the user gets notified about it
> when they next try to generate a url.

Presumably, this wants a spin-off bug for MVP with better UX for the failures.
Comment on attachment 8424865 [details] [diff] [review]
Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v2

I _think_ this is OK, but the callback chains are getting a little complicated to keep in my head at once, which always concerns me.  One thing that's making stuff harder is that sometimes "register" or "registration" is used without a qualifier (eg "push" or "loopXhr"), so that it's easy to misread when one is new to the code.  If you somehow make the two processes more clearly distinct, perhaps just by always using a qualifier, I think that would help.
Attachment #8424865 - Flags: feedback?(dmose) → feedback+
(I'm mildly tempted to want to use promises here to improve readability, but with none of us having had experience doing that in a Gecko context, I'm not convinced that now's the time for that).
(In reply to Dan Mosedale (:dmose) from comment #19)
> (In reply to Mark Banner (:standard8) from comment #9)
> >
> > If registration fails for any reason, the failure gets passed back up the
> > call tree via the callbacks, so at least the user gets notified about it
> > when they next try to generate a url.
> 
> Presumably, this wants a spin-off bug for MVP with better UX for the
> failures.

Yep, bug 1013248.

(In reply to Dan Mosedale (:dmose) from comment #21)
> (I'm mildly tempted to want to use promises here to improve readability, but
> with none of us having had experience doing that in a Gecko context, I'm not
> convinced that now's the time for that).

Agreed, now isn't the time for that, and my additional thought is we may be able to re-use some of the existing push notification stuff to harden up the push service (bug 1013248).
Updated to address the various comments and fix a few more things. This is currently based against privs-landing, but I'll rebase it against loop-service once review is complete. I'll attach the loop-client part in a moment.

Part 2 is still in work - I want to see if I can write some sensible unit tests for it.
Attachment #8423844 - Attachment is obsolete: true
Attachment #8424865 - Attachment is obsolete: true
Attachment #8425450 - Flags: review?(dmose)
Now with tests, fixed the previous comments as well.
Attachment #8424729 - Attachment is obsolete: true
Attachment #8424866 - Attachment is obsolete: true
Attachment #8426190 - Flags: review?(dmose)
Whiteboard: [mlp-server?][est:1d, p=2] → [mlp-server?][est:1d, p=1]
Status: NEW → ASSIGNED
Comment on attachment 8425453 [details] [diff] [review]
[checked in] Part 1. Ensure the client is registered before requesting a call url. v2

r=dmose; one tweak suggested on github branch.
Attachment #8425453 - Flags: review?(dmose) → review+
Comment on attachment 8425450 [details] [diff] [review]
[checked in] Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3

r=dmose with the comments in github addressed.
Attachment #8425450 - Flags: review?(dmose) → review+
(In reply to Dan Mosedale (:dmose) from comment #28)
> Comment on attachment 8425450 [details] [diff] [review]
> Part 1. Handle registration errors better, with simple notifications to the
> user, and handle subsequent attempts to register with the server. v3
> 
> r=dmose with the comments in github addressed.

FTR comments here: https://github.com/adamroach/gecko-dev/commit/32eca6e455bae72bb9a9afc4244acc9e51ff8fef

(In reply to Dan Mosedale (:dmose) from comment #27)
> Comment on attachment 8425453 [details] [diff] [review]
> Part 1. Ensure the client is registered before requesting a call url. v2
> 
> r=dmose; one tweak suggested on github branch.

FTR comments here: https://github.com/mozilla/loop-client/commit/bf8860f4bc43782c81dbbdc166a9ffad72d7ce91
Comment on attachment 8425450 [details] [diff] [review]
[checked in] Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3

Landed on our integration branch:

https://github.com/adamroach/gecko-dev/commit/27ff9e42ac5af2c3ca118148a678d61586290929
Attachment #8425450 - Attachment description: Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3 → [checked in] Part 1. Handle registration errors better, with simple notifications to the user, and handle subsequent attempts to register with the server. v3
Comment on attachment 8425453 [details] [diff] [review]
[checked in] Part 1. Ensure the client is registered before requesting a call url. v2

Landed in loop-client:

https://github.com/mozilla/loop-client/commit/5d3bb4ba83e0eb1d56d9b970e60560a655eb97bf
Attachment #8425453 - Attachment description: Part 1. Ensure the client is registered before requesting a call url. v2 → [checked in] Part 1. Ensure the client is registered before requesting a call url. v2
Attachment #8426190 - Attachment is obsolete: true
Attachment #8426191 - Attachment is obsolete: true
Attachment #8426190 - Flags: review?(dmose)
Attachment #8426191 - Flags: review?(dmose)
Attachment #8427688 - Flags: review?(dmose)
Comment on attachment 8427689 [details] [review]
Part 2 (loop-client). Only register with the servers if the user has obtained call urls that haven't expired yet. v2

r=dmose, with nits noted in PR addressed.
Attachment #8427689 - Flags: review?(dmose) → review+
Comment on attachment 8427688 [details] [review]
Part 2. Only register with the servers if the user has obtained call urls that haven't expired yet. v2

r=dmose w/the comments from the github PR addressed as you see fit.  Thanks for the hard work!
Attachment #8427688 - Flags: review?(dmose) → review+
Landed on loop-service our integration branch, closing for tracking, will comment further when this is landed in mercurial:

https://github.com/adamroach/gecko-dev/commit/5f93f71be036e11a544d6985c8ba056b35fd837e
https://github.com/mozilla/loop-client/commit/26d70718f4508b4d4689d6f74c585b3f615fe82b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla32 → mozilla33
Does this need automated or manual testing?
Flags: needinfo?(alexis+bugs)
OS: Linux → All
Hardware: x86_64 → All
Clearing the needinfo for me as this is a client-side bug.
Flags: needinfo?(alexis+bugs)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #39)
> Does this need automated or manual testing?

Dan, do you know the answer to this question?
Flags: needinfo?(dmose)
I suspect the testing for this could be automated over the medium term, but we don't have functional tests going just yet.

It's not totally clear to me how one would manually test this.  Standard8 might have some thoughts on that...
Flags: needinfo?(dmose) → needinfo?(standard8)
You can easily test this today by starting up the client, and within 5 seconds, opening the panel. If you start up with -jsconsole, you can generally see the registration being forced through.

For automated testing we will be testing this once we get our functional tests done (bug 976114) - by default they create a new profile, and hence we won't be auto-registering on startup, so getting a url will force registration & obtaining the url together.

The harder bit is probably testing auto registration works, although we have unit tests which partially cover that scenario.
Flags: needinfo?(standard8)
(In reply to Mark Banner (:standard8) from comment #43)
> You can easily test this today by starting up the client, and within 5
> seconds, opening the panel. If you start up with -jsconsole, you can
> generally see the registration being forced through.

Can I please get clearer steps to reproduce? I tried starting Firefox with -jconsole and initiating a call in the latest Nightly but all I got in terminal output was the following:

> GMPInstallManager.simpleCheckAndInstall INFO Last check was: 11535 seconds ago, minimum seconds: 86400
> GMPInstallManager.simpleCheckAndInstall INFO Will not check for updates.
Mark, can you please answer comment 44? Sorry for not flagging you earlier.
Flags: needinfo?(standard8)
QA Contact: anthony.s.hughes
Whiteboard: [mlp-server?][est:1d, p=1] → [mlp-server?][est:1d, p=1][qa?]
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #44)
> (In reply to Mark Banner (:standard8) from comment #43)
> > You can easily test this today by starting up the client, and within 5
> > seconds, opening the panel. If you start up with -jsconsole, you can
> > generally see the registration being forced through.
> 
> Can I please get clearer steps to reproduce? I tried starting Firefox with
> -jconsole and initiating a call in the latest Nightly but all I got in
> terminal output was the following:

So you should be seeing something like:

POST http://localhost:5000/registration [HTTP/1.1 200 OK 2ms]

(different server obviously)

Sometimes I've found that you have to toggle the "Net" button (sometimes on and off) to get the network activity to show.

Having looked at this again, I should probably extend my STR anyway:

1) Start Firefox with a new profile with -jsconsole on the command line
2) Wait 10-20 seconds, examine the console.

=> the above registration message shouldn't appear (wait 10-20 seconds; 5 seconds is the 'normal' time)

3) Open the Loop panel

=> Registration message should appear in the console, and a url is shown in the window.

4) Copy the url (this is needed since bug 1060610 landed)

5) Restart Firefox
6) Watch the console.

=> After about 5 seconds the registration message should appear.
Flags: needinfo?(standard8)
Flags: qe-verify+
Whiteboard: [mlp-server?][est:1d, p=1][qa?] → [mlp-server?][est:1d, p=1]
Sorry this took so long for me to get to, it fell off my radar. The steps are not working for me. When I launch Firefox with -jconsole I get an error in Browser Console: "unrecognized command line flag -jconsole"
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #47)
> Sorry this took so long for me to get to, it fell off my radar. The steps
> are not working for me. When I launch Firefox with -jconsole I get an error
> in Browser Console: "unrecognized command line flag -jconsole"

That should be "-jsconsole"
Thanks for your help Mark. I confirm your steps in comment 46.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.