Currently, FxAccounts.jsm polls the auth server to check whether the user has verified their email address. The auth server is to be updated with support for push notifications in [1] so that the polling can be eliminated. This issue exists to cover the associated changes to the browser code. [1] https://github.com/mozilla/fxa-auth-server/issues/1141

This is a proof of concept patch to replace email polling with push notifications. I made it so we can test the full flow and to start the discussion. This depends on https://bugzilla.mozilla.org/show_bug.cgi?id=1227527, waiting for that to land first.

Comment on attachment 8707232 [details] [diff] [review] 0002-Bug-1235607-Eliminate-FxA-email-verification-polling.patch Review of attachment 8707232 [details] [diff] [review]: ----------------------------------------------------------------- Just a quick look, but I see nothing bad (except potentially the fact we no longer poll at all, but I don't have enough push context to know if that makes sense or not) ::: services/fxaccounts/FxAccounts.jsm @@ +1009,5 @@ > > + _waitForPushNotification: function () { > + return new Promise((resolve) => { > + // listen for push notifications for account status update > + Services.obs.addObserver(function observe(subject, topic, data) { I think this will be leaky if the push notification never comes (eg, the user disconnects) @@ +1021,4 @@ > }, > > + // XXX - pollEmailStatus should maybe be on the AccountState object? > + pollEmailStatus: function pollEmailStatus(currentState, sessionToken) { this is no longer polling, so should probably get a new name. OTOH though, we were discussing keeping polling in (with a greater interval) just incase the push turns out to be unreliable. Are we comfortable push will be reliable? (I've no pre-conceived answer to that!) Also, I wonder if there is a subtle race here - we determine the user's not verified, then open the push channel - but what if the user verified between these steps? ie, I wonder if we should also do a checkEmailStatus after opening the channel to mitigate this? @@ +1028,5 @@ > + // is yet to start up.) This might cause "A promise chain failed to > + // handle a rejection" messages, so add an error handler directly > + // on the promise to log the error. > + currentState.whenVerifiedDeferred.promise.then(null, err => { > + log.info("the wait for user verification was stopped: " + err); these days log messages like this should be: > log.info("the wait for user verification was stopped", err); so we get richer error reporting (eg, including a stack trace etc.) @@ +1037,5 @@ > + this._waitForPushNotification().then(() => { > + return this.checkEmailStatus(sessionToken); > + }) > + .then((response) => { > + log.debug("checkEmailStatus -> " + JSON.stringify(response)); This was almost certainly already there, but we should be careful with PII here - if it's possible the complete response contains sensitive info we should check |logPII| before logging it - and we can also skip the stringify. eg, if we decided it was potentially sensitive we could maybe write: if (logPII) { log.debug("checkEmailStatus full response", response); } log.debug("checkEmailStatus got verified flag as", response.verified); @@ +1054,5 @@ > + this.notifyObservers(ON_FXA_UPDATE_NOTIFICATION, ONVERIFIED_NOTIFICATION); > + }); > + } > + }, error => { > + // TODO: What do we do here now? probably just log.error("blah", error); - OTOH though, it would be safer to have a final .catch, as exceptions in the block above will not be caught here. @@ +1367,5 @@ > + _registerPushCallback: function () { > + let pushPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); > + > + return new Promise((resolve, reject) => { > + PushService.subscribe(FXA_PUSH_SCOPE_DEVICE, pushPrincipal, (result, subscription) => { do we need to unsubscribe? I wonder if we can avoid this and the observer leak by having the subscription and observer as globals (ie, only ever registered once and never removed), and having the global handlers do the right thing? @@ +1370,5 @@ > + return new Promise((resolve, reject) => { > + PushService.subscribe(FXA_PUSH_SCOPE_DEVICE, pushPrincipal, (result, subscription) => { > + if (Components.isSuccessCode(result)) { > + return resolve(subscription); > + } else { We avoid |else| after returns @@ +1391,5 @@ > } catch(ignore) {} > > return Promise.resolve().then(() => { > + return this._registerPushCallback(); > + }) most other code in this file has the .then on the same line as the closing })

If we're removing polling all together then we need some failsafes in place to handle cases we may have missed the push notification. E.g., 1) If we're unverified at browser start, check once to see if that's still the case. 2) If the user interacts with any browser UI that depends on the user being verified, check in again to see if that's still the case (in some reasonable debounced way). Thoughts?

Comment on attachment 8707232 [details] [diff] [review] 0002-Bug-1235607-Eliminate-FxA-email-verification-polling.patch Review of attachment 8707232 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccountsClient.jsm @@ +374,5 @@ > * { > * id: Device identifier > * createdAt: Creation time (milliseconds since epoch) > * name: Name of device > * type: Type of device (mobile|desktop) I'm probably stating the obvious but we should make sure these comments get updated for the new options argument.

One more thing missing from this: Check for empty strings in the push callback of the current devices and update with a new one if needed

Another thing to think about: users reconfiguring Push settings ( + Push server connection failure)

+1 to what Chris said in Comment 3. I suspect there could be a lot of potential complexity here in recovering from bad situations, e.g. detecting that a previously-registered push endpoint has been invalided by either the accounts server or the push server. It would be good to explicitly enumerate the ways in which this connection might break down.

As of January 22nd 2016: - the observer topic is now called "push-message," instead of "push-notification" - there's a new nsIPushMessageData interface

And some docs: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPushService

Dan's recent hacks article has some good advice re: handling changes to the subscription URL which are probably relevant to us here as well: https://hacks.mozilla.org/2016/01/web-push-arrives-in-firefox-44/

Renaming the issue to better reflect the milestone title.

Polling reduction landed in https://bugzilla.mozilla.org/show_bug.cgi?id=1247786#c33 Next step is https://bugzilla.mozilla.org/show_bug.cgi?id=1249029

Renaming this bug to make it clear that the work landed in bug 1247786 and bug 1249029.