Closed Bug 1074663 Opened 6 years ago Closed 6 years ago

Register with the server for push notification updates to rooms

Categories

(Hello (Loop) :: Client, enhancement)

enhancement
Not set
normal

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
Blocking Flags:
backlog Fx35+

People

(Reporter: standard8, Assigned: pkerr)

References

Details

(Whiteboard: [rooms])

User Story

* Extend the push notification service as per the specification:

https://wiki.mozilla.org/Loop/Architecture/Rooms#Room_Owner_Notification

* Update push handler to register multiple ids for rooms and calls, both FxA and Guest
* Update registration handling to handle multiple channels
* Update notification callbacks to handle multiple channels

* TBD: Split existing Guest/FxA Calls push notification into two separate channels (is the server changing the ids?)

Attachments

(1 file, 11 obsolete files)

93.29 KB, patch
jesup
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
Severity: normal → enhancement
Assignee: nobody → pkerr
Status: NEW → ASSIGNED
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?
Flags: needinfo?(adam)
(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.
Flags: needinfo?(adam)
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.)
Flags: needinfo?(adam)
(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.
Flags: needinfo?(adam)
Attached patch WIP (obsolete) — Splinter Review
MozLoopPushHanler will handles multiple notification channels and multiple calls to register channels. Integrated with LoopRooms to register for rooms notification channels.
Attached patch WIP (obsolete) — Splinter Review
MozLoopPushHandler will handles multiple notification channels and multiple calls to register channels. Integrated with LoopRooms to register for rooms notification channels.
Attachment #8503424 - Attachment is obsolete: true
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.
Attachment #8503437 - Flags: feedback?(standard8)
Attachment #8503437 - Flags: feedback?(standard8)
backlog: --- → Fx35+
Attached patch WIP (obsolete) — Splinter Review
Functional code. But, some tests are broken due to new registrations
Attachment #8503437 - Attachment is obsolete: true
Attachment #8504994 - Attachment is obsolete: true
Attachment #8504996 - Flags: review?(standard8)
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).
removed a missed DEBUG temp change.
Attachment #8504996 - Attachment is obsolete: true
Attachment #8504996 - Flags: review?(standard8)
Attachment #8505024 - Flags: review?(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.
Attachment #8505024 - Flags: review?(standard8)
Attachment #8505024 - Flags: review?(MattN+bmo)
Attachment #8505024 - Flags: feedback+
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
(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.
Attachment #8505024 - Attachment is obsolete: true
Attachment #8505024 - Flags: review?(MattN+bmo)
Attachment #8507069 - Attachment is obsolete: true
Attachment #8507599 - Flags: review?(MattN+bmo)
Blocks: 1074664
fixed some bit-rot
Attachment #8507599 - Attachment is obsolete: true
Attachment #8507599 - Flags: review?(MattN+bmo)
Attachment #8507599 - Flags: review?(MattN+bmo)
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.
Attachment #8508234 - Attachment is obsolete: true
Attachment #8508321 - Flags: review?(MattN+bmo)
Attachment #8507599 - Flags: review?(MattN+bmo)
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
fixed mochitests
Attachment #8508321 - Attachment is obsolete: true
Attachment #8508321 - Flags: review?(MattN+bmo)
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.
>::: 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.
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.
incorporate feedback
Attachment #8509318 - Attachment is obsolete: true
Attachment #8509673 - Flags: review?(MattN+bmo)
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.
(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 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?
Attachment #8509673 - Flags: review?(MattN+bmo) → review+
(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.
>::: 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.
::: 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
incorporate review comments and nits
Attachment #8509673 - Attachment is obsolete: true
::: 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.
Carrying forward r+ from MattN.
Keywords: checkin-needed
Attachment #8510059 - Attachment description: Register with PushServer for updates to rooms → [landed on fx-team] Register with PushServer for updates to rooms
Blocks: 1088230
https://hg.mozilla.org/mozilla-central/rev/5c8fc27aa8d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1089919
No longer blocks: 1074665
Iteration: --- → 36.1
What are the user-facing implications of this change? Does this need manual QE verification? Does this have in-testsuite coverage?
Flags: qe-verify?
Flags: in-testsuite?
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
Attachment #8510059 - Flags: review+
Attachment #8510059 - Flags: approval-mozilla-aurora?
Attachment #8510059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1102806
This is covered by unit tests.
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.