1074663 - Register with the server for push notification updates to rooms

Status: RESOLVED FIXED

(Hello (Loop) :: Client, enhancement)

Comment 1

[Paul Kerr [:pkerr]]

Do you expect that the current usage of one channelID for the "calls" based Loop client to be replaced with using a separate channelID for FxA and Guest in this patch update? Or, is the change to be made to add the new "rooms" notification only?

Comment 2

[Adam Roach [:abr]]

(In reply to Paul Kerr [:pkerr] from comment #1)
> Do you expect that the current usage of one channelID for the "calls" based
> Loop client to be replaced with using a separate channelID for FxA and Guest
> in this patch update? Or, is the change to be made to add the new "rooms"
> notification only?

tl;dr: Both.

We're not going to turn off the unaccounted direct calling immediately when rooms rolls out; we're just going to make it impossible to make new URLs. As the old ones age out, the feature will gradually remove itself from the system. This implies that the direct call handling logic for a logged in user will still have to check "GET /calls" for both the logged-in user and the browser instance. I'm concerned about the load impact on the Loop server of doing so.

Comment 3

[Paul Kerr [:pkerr]]

Actually, the pair of channelIDs I was asking about are the FXA_CALLS and GUEST_CALLS. The current code only uses one channelID for operation and checks both FxA and Guest accounts when a notification is received. (The channelID is the same as the one listed as GUEST_CALLS.) Is it your intention that this bug also change the "calls" behavior to use separate channelIDs for FXA and GUEST? (Besides the new "rooms" functions.)

Comment 4

[Adam Roach [:abr]]

(In reply to Paul Kerr [:pkerr] from comment #3)
> Actually, the pair of channelIDs I was asking about are the FXA_CALLS and
> GUEST_CALLS. The current code only uses one channelID for operation and
> checks both FxA and Guest accounts when a notification is received. (The
> channelID is the same as the one listed as GUEST_CALLS.) Is it your
> intention that this bug also change the "calls" behavior to use separate
> channelIDs for FXA and GUEST? (Besides the new "rooms" functions.)

Yes. That's what I was trying to say above. I don't want to have to hit the Loop server twice for every direct call.

Comment 5

[Paul Kerr [:pkerr]]

Attachment: WIP

MozLoopPushHanler will handles multiple notification channels and multiple calls to register channels. Integrated with LoopRooms to register for rooms notification channels.

Comment 6

[Paul Kerr [:pkerr]]

Attachment: WIP

MozLoopPushHandler will handles multiple notification channels and multiple calls to register channels. Integrated with LoopRooms to register for rooms notification channels.

Comment 7

[Paul Kerr [:pkerr]]

Comment on attachment 8503437 [details] [diff] [review]
WIP

Refer to bug 1074664 for the registration performed by the LoopRooms code. I will need to integrate the rooms registration with the delay timer that registers the MozLoopService. Since LoopRooms needs to call MozLoopService, it needs to be outside of MozLoopService.

Comment 8

[Paul Kerr [:pkerr]]

Attachment: WIP

Functional code. But, some tests are broken due to new registrations

Comment 9

[Paul Kerr [:pkerr]]

Attachment: Part 1: Register rooms notification channels. Register new calls notification channels

Comment 10

[Paul Kerr [:pkerr]]

Part 2 patch will fix the remainder of the broken xpcshell tests. I want to get feedback on the refactored code that handles multiple registrations and breaks out the calls handling code in a new module called LoopCalls.jsm (parallel to the new LoopRooms.jsm).

Comment 11

[Paul Kerr [:pkerr]]

Attachment: Part 1: Register rooms notification channels. Register new calls notification channels

removed a missed DEBUG temp change.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8505024 [details] [diff] [review]
Part 1: Register rooms notification channels. Register new calls notification channels

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

The general idea behind this looks reasonable to me. Splitting out LoopCalls/LoopRooms is definitely a good idea.

I'm going to see if Matt has time to review this, as I need to look at other things today, and I know Matt was interested in improving the registration flow so may have some comments here.

::: browser/components/loop/LoopCalls.jsm
@@ +191,5 @@
> + * The registration is a two-part process. First we need to connect to
> + * and register with the push server. Then we need to take the result of that
> + * and register with the Loop server.
> + */
> +let LoopCallsInternal = {

Based on previous discussions with testing, I think we should probably not have a split of LoopCallInternal vs LoopCalls here, and just go with a single version, and clear indication (e.g. _) of private methods.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8505024 [details] [diff] [review]
Part 1: Register rooms notification channels. Register new calls notification channels

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

I haven't had time to look at the registration changes yet.

::: browser/components/loop/LoopCalls.jsm
@@ +21,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "LOOP_SESSION_TYPE",
> +                                  "resource:///modules/loop/MozLoopService.jsm");
> +
> +// Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
> +XPCOMUtils.defineLazyGetter(this, "log", () => {

You can just access the logger from MozLoopService like MozLoopAPI does so you don't need to copy this code.

::: browser/components/loop/MozLoopService.jsm
@@ +24,1 @@
>  const PREF_LOG_LEVEL = "loop.debug.loglevel";

Want to |hg rename| this file to LoopService.jsm for consistency? :) I personally wouldn't mind.

@@ +311,4 @@
>      if (gRegisteredDeferred) {
>        return gRegisteredDeferred.promise;
>      }
> + 

Nit: whitespace

Comment 14

[Matthew N. [:MattN]]

(In reply to Mark Banner (:standard8) from comment #12)
> ::: browser/components/loop/LoopCalls.jsm
> @@ +191,5 @@
> > + * The registration is a two-part process. First we need to connect to
> > + * and register with the push server. Then we need to take the result of that
> > + * and register with the Loop server.
> > + */
> > +let LoopCallsInternal = {
> 
> Based on previous discussions with testing, I think we should probably not
> have a split of LoopCallInternal vs LoopCalls here, and just go with a
> single version, and clear indication (e.g. _) of private methods.

I agree. A separation can also be made with a comment to keep the public method in one part of the object and private _-prefixed ones in another half.

Comment 15

[Paul Kerr [:pkerr]]

Attachment: Register with PushServer for updates to rooms. Modify calls notification registration to use two channelIDs where it once used only one. Add notification of two notifications channels for rooms.

Comment 16

[Paul Kerr [:pkerr]]

Attachment: Register with PushServer for updates to rooms

Comment 17

[Paul Kerr [:pkerr]]

Attachment: Register with PushServer for updates to rooms

fixed some bit-rot

Comment 18

[Paul Kerr [:pkerr]]

Attachment: Register with PushServer for updates to rooms

I had made the MozLoopPushHandler.jsm response to having Services.io.offline set to true to be harsher than the original code: threw an exception. Reverted back to pringing a warning and returning.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8508321 [details] [diff] [review]
Register with PushServer for updates to rooms

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

Some initial review comments

::: browser/components/loop/LoopCalls.jsm
@@ +7,5 @@
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm", this);

What non-exported symbols are you using from osfile.jsm? i.e. why are you using the second argument?

@@ +178,5 @@
> + * The registration is a two-part process. First we need to connect to
> + * and register with the push server. Then we need to take the result of that
> + * and register with the Loop server.
> + */
> +let LoopCallsInternal = {

Please fold this into LoopCalls as mentioned earlier and prefix these members with "_"

::: browser/components/loop/MozLoopAPI.jsm
@@ +43,5 @@
>   */
>  const cloneErrorObject = function(error, targetWindow) {
>    let obj = new targetWindow.Error();
>    for (let prop of Object.getOwnPropertyNames(error)) {
> +    obj[prop] = String(error[prop]);

Note that this will convert numbers to strings too whereas the old code avoided that.

@@ -48,5 @@
> -      value = String(value);
> -    }
> -
> -    Object.defineProperty(Cu.waiveXrays(obj), prop, {
> -      configurable: false,

I'm not really sure about whether this code should be removed.

::: browser/components/loop/MozLoopPushHandler.jsm
@@ +24,5 @@
>    // This is the uri of the push server.
>    pushServerUri: undefined,
> +  // Records containing the registration and notification callbacks indexed by channelID.
> +  // Each channel will be registered with the PushServer.
> +  channels: {},

JS |Map| objects should be used instead of JS objects as the API is made for this use case and it avoids the default keys. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared

@@ +73,3 @@
>      *                                 the websocket to be mocked for tests.
>      */
> +  initialize: function(options = {}) {

I think the JSDoc is out-of-date.

@@ +91,5 @@
>      this._openSocket();
> +    return true;
> +  },
> +
> +  register: function(channelID, onRegistered, onNotification) {

Could you add a JSDoc comment?

@@ +127,5 @@
>      // If a uaID has already been assigned, assume this is a re-connect
>      // and send the uaID in order to re-synch with the
>      // PushServer. If a registration has been completed, send the channelID.
> +    let helloMsg =
> +        { messageType: "hello",

Nit: keep the { on the same line as the = and align the }; with the beginning of the |let|.

::: browser/components/loop/test/xpcshell/head.js
@@ +77,5 @@
>    // This sets the registration result to be returned when initialize
>    // is called. By default, it is equivalent to success.
>    registrationResult: null,
> +  registrationPushURL: null,
> +  notificationCallback: {},

Use a map here too

Comment 20

[Paul Kerr [:pkerr]]

Attachment: Register with PushServer for updates to rooms

fixed mochitests

Comment 21

[Paul Kerr [:pkerr]]

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f4412091172
Multi-platform run of xpcshell, marionette and mochi tests.
One orange for video control test on e10s.

Comment 22

[Paul Kerr [:pkerr]]

>::: browser/components/loop/MozLoopPushHandler.jsm
>@@ +24,5 @@
>>    // This is the uri of the push server.
>>    pushServerUri: undefined,
>> +  // Records containing the registration and notification callbacks indexed by channelID.
>> +  // Each channel will be registered with the PushServer.
>> +  channels: {},
>
>JS |Map| objects should be used instead of JS objects as the API is made for this use case and it avoids >the default keys. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Objects_and_maps_compared

If I change this to Map now, I will need to make changes in many locations in the browser_fxa_login.js mochitest that uses direct access to this property on the mockPushHandler.

Comment 23

[Paul Kerr [:pkerr]]

I suggest that I create a bug to address the transformation of the Object to Map implementation and move forward. Several rooms bugs are blocked waiting for functional registration code to land.

Comment 24

[Paul Kerr [:pkerr]]

Attachment: Register with PushServer for updates to rooms

incorporate feedback

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8509673 [details] [diff] [review]
Register with PushServer for updates to rooms

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

Here are some comments on MozLoopService:

I thought this patch was going to cleanup registration while adding more stuff to it.

The main change I think that should be made to the flow is rename promiseRegisteredWithServers to promiseRegisteredForPush and have it stop at setting gFxAOPushUrls. MozLoopService.register would then do the Loop server registration after promiseRegisteredForPush resolves.

::: browser/components/loop/MozLoopService.jsm
@@ +319,5 @@
>  
> +    this._mocks.webSocket = mockWebSocket;
> +    this._mocks.pushHandler = mockPushHandler;
> +
> +    // Wrap push notification regisration call-back in a Promise.

typo: s/regisration/registration/

@@ +320,5 @@
> +    this._mocks.webSocket = mockWebSocket;
> +    this._mocks.pushHandler = mockPushHandler;
> +
> +    // Wrap push notification regisration call-back in a Promise.
> +    let registerForNotification = (channelID, onNotification) => {

You can use traditional function syntax here to make it more clear this is a function when skimming:
function registerForNotification(channelID, onNotification) {

I wonder if it would be cleaner outside here and perhaps tests want to use it

@@ +328,5 @@
> +            reject(Error(error));
> +          } else {
> +            resolve(pushUrl);
> +          }
> +        }

Nit: missing semicolon

@@ +331,5 @@
> +          }
> +        }
> +        gPushHandler.register(channelID, onRegistered, onNotification);
> +      });
> +    }

Nit: missing semicolon

@@ +353,5 @@
> +                                              LoopCalls.onNotification);
> +
> +    let roomsRegFxA = registerForNotification(LoopRooms.channelIDs.FxA,
> +                                              LoopRooms.onNotification);
> +    Promise.all([callsRegGuest, roomsRegGuest, callsRegFxA, roomsRegFxA])

I like the simplicity of Promise.all but it does mean that one failure takes the whole ship down with it. I guess it's fine since I don't know why some would fail but not others (other than intermittent failures).

@@ +357,5 @@
> +    Promise.all([callsRegGuest, roomsRegGuest, callsRegFxA, roomsRegFxA])
> +    .then((pushUrls) => {
> +      gFxAOPushUrls = {calls: pushUrls[2], rooms: pushUrls[3]};
> +      return this.registerWithLoopServer(LOOP_SESSION_TYPE.GUEST,
> +                                         {calls: pushUrls[0], rooms: pushUrls[1]})})

Nit: Put spaces on the inside of { and } for objects (x2) like https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

Also, you're missing a semicolon after the return value.

@@ -695,5 @@
>      let unregisterURL = "/registration?simplePushURL=" + encodeURIComponent(pushURL);
>      return this.hawkRequest(sessionType, unregisterURL, "DELETE")
>        .then(() => {
>          log.debug("Successfully unregistered from server for sessionType", sessionType);
> -        MozLoopServiceInternal.clearSessionToken(sessionType);

I'll have to look at the removal of these two clearSessionToken calls more closely.

Comment 26

[Paul Kerr [:pkerr]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #25)
> 
> @@ -695,5 @@
> >      let unregisterURL = "/registration?simplePushURL=" + encodeURIComponent(pushURL);
> >      return this.hawkRequest(sessionType, unregisterURL, "DELETE")
> >        .then(() => {
> >          log.debug("Successfully unregistered from server for sessionType", sessionType);
> > -        MozLoopServiceInternal.clearSessionToken(sessionType);
> 
> I'll have to look at the removal of these two clearSessionToken calls more
> closely.

unregisterFromLoopServer() is only called with a sessionType of FXA. Since the pushUrl is encoded as a query parameter, only one registration can be canceled per call. (The DELETE /registration does not use the new {simplePushURLs: {calls: ... rooms: ...}} format used by registration.) In the new multi-channel scheme, unregisterFromLoopServer needs to be called twice. In its old form, it would clear the session token on the first call and the second call would fail with a 400 Bad Request. Moving the clearSessionToken to the logoutFromFxA function wraps up the processing whether it succeeds or fails, which is the expected result in the mochitest.

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8509673 [details] [diff] [review]
Register with PushServer for updates to rooms

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +47,5 @@
>      let value = error[prop];
>      if (typeof value != "string" && typeof value != "number") {
>        value = String(value);
>      }
> +    

Nit: whitespace

::: browser/components/loop/MozLoopService.jsm
@@ +112,5 @@
>  let gInitializeTimer = null;
>  let gFxAEnabled = true;
>  let gFxAOAuthClientPromise = null;
>  let gFxAOAuthClient = null;
> +let gFxAOPushUrls = {};

This is now unused

@@ +1325,5 @@
> +          LOOP_SESSION_TYPE.FXA, roomsPushUrl);
> +      }
> +    } 
> +    catch (error) {throw error} 
> +    finally {

} catch (error) {
  throw error;
} finally {

::: browser/components/loop/test/mochitest/browser_fxa_login.js
@@ +121,5 @@
>        "Check FxA hawk token is not set");
>  });
>  
>  add_task(function* params_nonJSON() {
> +  Services.prefs.setCharPref("loop.server", "https://localhost:3000/invalid");

Is localhost:3000 a live server? If so, this is changing the test and I'm not sure why

@@ +256,5 @@
>  
>    // Normally the same pushUrl would be registered but we change it in the test
>    // to be able to check for success on the second registration.
> +  mockPushHandler.registeredChannels[LoopCalls.channelIDs.FxA] = "https://localhost/pushUrl/fxa";
> +  mockPushHandler.registeredChannels[LoopRooms.channelIDs.FxA] = "https://localhost/pushUrl/fxa";

Ideally these should be two different URLs so we are testing they don't get mixed up.

@@ +283,5 @@
>  
>    let registrationResponse = yield promiseOAuthGetRegistration(BASE_URL);
> +  ise(registrationResponse.response.simplePushURLs.calls, "https://localhost/pushUrl/fxa",
> +      "Check registered push URL");
> +  ise(registrationResponse.response.simplePushURLs.rooms, "https://localhost/pushUrl/fxa",

Smae here. Use different URLs

::: browser/components/loop/test/mochitest/head.js
@@ +6,5 @@
>    LOOP_SESSION_TYPE,
>    MozLoopServiceInternal,
>  } = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
> +const {LoopCalls} = Cu.import("resource:///modules/loop/LoopCalls.jsm", {});
> +const {LoopRooms} = Cu.import("resource:///modules/loop/LoopRooms.jsm", {});

Why not the simpler version?:
Cu.import("resource:///modules/loop/LoopCalls.jsm");

::: browser/components/loop/test/xpcshell/test_loopservice_registration.js
@@ +23,3 @@
>      // 404 is an expected failure indicated by the lack of route being set
>      // up on the Loop server mock. This is added in the next test.
> +    Assert.equal(err.message, "404", "Expected no errors in websocket registration");

Why the change from errno?

Comment 28

[Paul Kerr [:pkerr]]

(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #19)
> Comment on attachment 8508321 [details] [diff] [review]
> @@ +178,5 @@
> > + * The registration is a two-part process. First we need to connect to
> > + * and register with the push server. Then we need to take the result of that
> > + * and register with the Loop server.
> > + */
> > +let LoopCallsInternal = {
> 
> Please fold this into LoopCalls as mentioned earlier and prefix these
> members with "_"
> 
Per our discussion, I am going to leave this split as-is for now pending further discussion within the group. No test code currently uses direct access to LoopCallsInternal.

Comment 29

[Paul Kerr [:pkerr]]

>::: browser/components/loop/test/mochitest/browser_fxa_login.js
>@@ +121,5 @@
>>        "Check FxA hawk token is not set");
>>  });
>>  
>>  add_task(function* params_nonJSON() {
>> +  Services.prefs.setCharPref("loop.server", "https://localhost:3000/invalid");
>
>Is localhost:3000 a live server? If so, this is changing the test and I'm not sure whyFATAL ERROR: >Non-local network connections are disabled and a connection attempt to loop.invalid (198.105.244.228) was m
ade.

Matt: when loop.server is set to http://loop.invalid I get the following on my system when running the tests:
"You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific
 httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server."
The hawkRequest is not going to the mock server and a fatal error is thrown. The localhost is not live and serves the purpose as a null sink but does not trigger the error.

Comment 30

[Paul Kerr [:pkerr]]

::: browser/components/loop/test/mochitest/head.js
>@@ +6,5 @@
>>    LOOP_SESSION_TYPE,
>>    MozLoopServiceInternal,
>>  } = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
>> +const {LoopCalls} = Cu.import("resource:///modules/loop/LoopCalls.jsm", {});
>> +const {LoopRooms} = Cu.import("resource:///modules/loop/LoopRooms.jsm", {});
>
>Why not the simpler version?:
>Cu.import("resource:///modules/loop/LoopCalls.jsm");

This form avoids triggering a test the explicitly looks for global properties.

146 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_CardDavImporter.js | leaked window property: LOOP_SESSION_TYPE - expected PASS
147 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_CardDavImporter.js | leaked window property: LoopCalls - expected PASS
148 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/loop/test/mochitest/browser_CardDavImporter.js | leaked window property: LoopRooms - expected PASS

Comment 31

[Paul Kerr [:pkerr]]

Attachment: [landed on fx-team] Register with PushServer for updates to rooms

incorporate review comments and nits

Comment 32

[Paul Kerr [:pkerr]]

::: browser/components/loop/test/xpcshell/test_loopservice_registration.js
@@ +23,3 @@
>      // 404 is an expected failure indicated by the lack of route being set
>      // up on the Loop server mock. This is added in the next test.
> +    Assert.equal(err.message, "404", "Expected no errors in websocket registration");

Why the change from errno?

Promise versions of new pushserver registration callbacks now pass an Error object with the message set to 404.

Comment 33

[Paul Kerr [:pkerr]]

Carrying forward r+ from MattN.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/5c8fc27aa8d4

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c8fc27aa8d4

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/358002151c87

Comment 37

[u279076]

What are the user-facing implications of this change? Does this need manual QE verification? Does this have in-testsuite coverage?

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8510059 [details] [diff] [review]
[landed on fx-team] Register with PushServer for updates to rooms

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 39

[Mark Banner (:standard8)]

This is covered by unit tests.