1065153 - Get Call URLs with the proper Hawk session

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Matthew N. [:MattN]]

After bug 1065052 lands, there are two hawk sessions in use when logged in. We need to make sure we use the proper session when a user is logged in so that call URLs are either associated with the guest device or with the account (and all devices on it).

My understanding is that initially logged in users won't be able to choose to generate a URL for the guest session so I don't think UI needs to change.

See the "sessionType" argument to hawkRequest.

Comment 1

[Rémy Hubscher (:natim)]

> My understanding is that initially logged in users won't be able to choose to generate a URL for the guest session so I don't think UI needs to change.

I totally agree, the point of keeping both session is to be able to receive calls from anonymously created URLs.

Comment 2

[Matthew N. [:MattN]]

My point was more that we're not currently going to let logged in users decide (via UI) whether to generate a call URL for the anonymous session or the FxA session. If they are logged in, the call URL will always be for the FxA session, not the anonymous one.

Comment 3

[Rémy Hubscher (:natim)]

Yes, I totally agree.

Comment 4

[Matthew N. [:MattN]]

Without looking at if/how we delete call URLs: If we are persisting them for the history view, I guess we will need to also persist whether it was an anonymous or FxA URL. The other option is to just delete on both sessions so we don't have to persist that info.

Comment 5

[Mark Banner (:standard8)]

The hardest part here is how to keep track of if a call URL is for a logged in user or not.

I guess, what we could do is:

- Extend MozLoopService#onHandleNotification to request /calls information for both anonymous session and FxA session (if appropriate).
- When saving the call data, tag the data with the sessionType
- This will then get passed to the conversation window
- The conversation window can then pass the information back to the DELETE request.

This would serve the current situation. If more come up later, we'd probably need to persist, but we can think about that scenario when it occurs.

Comment 6

[Mark Banner (:standard8)]

Ok, comment 5 was actually forgetting this bug was about getting & deleting, not just deleting.

I'm going to move the delete part to bug 1065155 as delete is reasonably closely coupled with the conversation window at the moment, so I think it makes sense to do that together.

I've updated the user story with what I think needs doing here.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

I've much of the code working and content side unit tests passing, and in theory it should work.  Except it has regressed the one functional test ("make a call"), so I need to figure out what's going on.  I've made it possible to do xpcshell tests on at least pieces of MozLoopAPI.jsm and LoopStorage.jsm, and I'm about half done with writing unit tests for the browser-side changes as well.

Comment 8

[Mark Banner (:standard8)]

I'm a bit surprised you needed xpcshell tests for MozLoopAPI, when we're already testing that in Mochitest. However, I won't object, though we might want to consider keeping those mainly in one place.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Auto-select and pass correct hawk sessionType through hawkRequest, WIP v1

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8489732 [details] [diff] [review]
Auto-select and pass correct hawk sessionType through hawkRequest, WIP v1

This patch makes mozLoop.hawkRequest pass a sessionType variable, and I suspect that code works.  It's based on top of jaws rebased patch bug 1047146.

I've done some tweaking to make it possible to test MozLoopAPI using XPCShell, and I'm trying to get it to test accessing the mozLoop.LOOP_SESSION_TYPE values.  I can make the tests session LOOP_SESSION_TYPE itself, but, interestingly enough, none of the properties.  Turning on xrays doesn't seem to help.

Could you take a look at this and see if you can see what I'm missing?  I'm happy to pair on it over Skype or Hangouts, if you wish.  Thanks!

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

To be more specific, when running "mach xpcshell-test --sequential browser/components/loop/test/xpcshell"

The Assert on line 20 of test_loopapi_hawk_request passes, but the Assert on line 24 fails with this message:

 0:04.44 TEST-UNEXPECTED-FAIL | /Users/dmose/r/gecko-dev/obj-x86_64-apple-darwin13.3.0/_tests/xpcshell/browser/components/loop/test/xpcshell/test_loopapi_hawk_request.js | "undefined" == 1 - See following stack:
 0:04.44     /Users/dmose/r/gecko-dev/obj-x86_64-apple-darwin13.3.0/_tests/xpcshell/browser/components/loop/test/xpcshell/test_loopapi_hawk_request.js:hawk_session_scope_constants:24
 0:04.44     _run_next_test@/Users/dmose/r/gecko-dev/testing/xpcshell/head.js:1308:9
 0:04.44     do_execute_soon/<.run@/Users/dmose/r/gecko-dev/testing/xpcshell/head.js:570:9
 0:04.44     _do_main@/Users/dmose/r/gecko-dev/testing/xpcshell/head.js:191:5
 0:04.44     _execute_test@/Users/dmose/r/gecko-dev/testing/xpcshell/head.js:405:5
 0:04.44     @-e:1:1

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8489732 [details] [diff] [review]
Auto-select and pass correct hawk sessionType through hawkRequest, WIP v1

Standard8 has led me to see the error of my ways.  New patch soonish... :-)

Comment 13

[Matthew N. [:MattN]]

This will fix it:

     LOOP_SESSION_TYPE: {
       enumerable: true,
-      value: function() {
-        var contentObj = Cu.cloneInto({GUEST: 1, FXA: 2}, targetWindow);
+      get: function() {
+        let contentObj = Cu.cloneInto(LOOP_SESSION_TYPE, targetWindow);
         return contentObj;
       },
     },

It was a stupid mistake on my part since there was no consumer of the getter. I figured it out when I saw that typeof was outputting "function" and then I looked at the result of toSource() and saw the body of the "value" function above.

Note I also changed |var| to |let| and referenced the original const again.

Other feedback:
* Use JS default argument syntax for keepUnsealedForTesting: "…, keepUnsealedForTesting = false)"

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Support call URLs with both guest and FxA hawk session types

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8491081 [details] [diff] [review]
Support call URLs with both guest and FxA hawk session types

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

r=me with comments

Woohoo!

::: browser/components/loop/LoopStorage.jsm
@@ +10,5 @@
> +  Cu.importGlobalProperties(["indexedDB"]);
> +} catch (ex) {
> +  if (typeof window !== 'undefined' && "console" in window) {
> +    console.log("Failed to import indexedDB; if this isn't a unit test," +
> +                " something is wrong", ex);

This is kinda gross.
Cu.reportError would allow you to avoid the window and console checks.

::: browser/components/loop/content/js/client.js
@@ +109,5 @@
>       */
>      _requestCallUrlInternal: function(nickname, cb) {
> +      var sessionType;
> +      if (!this.mozLoop.userProfile) {
> +        console.log("this.mozLoop.LOOP_SESSION_TYPE: ",

Nit: I would remove the ! and reverse the two bodies just to avoid the negation

@@ +110,5 @@
>      _requestCallUrlInternal: function(nickname, cb) {
> +      var sessionType;
> +      if (!this.mozLoop.userProfile) {
> +        console.log("this.mozLoop.LOOP_SESSION_TYPE: ",
> +          this.mozLoop.LOOP_SESSION_TYPE);

I'm guessing the console.log was just leftover from debugging.

@@ +154,5 @@
>        }.bind(this));
>      },
>  
>      _deleteCallUrlInternal: function(token, cb) {
> +      this.mozLoop.hawkRequest(this.mozLoop.LOOP_SESSION_TYPE.GUEST,

Add XXX with a bug #?

@@ +156,5 @@
>  
>      _deleteCallUrlInternal: function(token, cb) {
> +      this.mozLoop.hawkRequest(this.mozLoop.LOOP_SESSION_TYPE.GUEST,
> +        "/call-url/" + token, "DELETE", null, function (error, responseText) {
> +          if (error) {

Nit: the previous indentation aligning arguments on a new line with the first argument is more readable IMO.

Nit: The inline function should probably be defined at the top of _deleteCallUrlInternal and referenced by name since it's so long and makes this hard to read.

::: browser/components/loop/test/xpcshell/test_loopapi_hawk_request.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Test the toolbar button states.

Stale description

@@ +67,5 @@
> +});
> +
> +function run_test()
> +{
> +  run_next_test();

Nit: brace on the same line

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Matthew N. [:MattN] from comment #15)
> r=me with comments
> 
> Woohoo!
> 

Thanks for the quick review and encouragement!

> ::: browser/components/loop/LoopStorage.jsm
> @@ +10,5 @@
> > +  Cu.importGlobalProperties(["indexedDB"]);
> > +} catch (ex) {
> > +  if (typeof window !== 'undefined' && "console" in window) {
> > +    console.log("Failed to import indexedDB; if this isn't a unit test," +
> > +                " something is wrong", ex);
> 
> This is kinda gross.
> Cu.reportError would allow you to avoid the window and console checks.

Comment added instead, as per IRC discussion.
 
> ::: browser/components/loop/content/js/client.js
> @@ +109,5 @@
> >       */
> >      _requestCallUrlInternal: function(nickname, cb) {
> > +      var sessionType;
> > +      if (!this.mozLoop.userProfile) {
> > +        console.log("this.mozLoop.LOOP_SESSION_TYPE: ",
> 
> Nit: I would remove the ! and reverse the two bodies just to avoid the
> negation
> 
> @@ +110,5 @@
> >      _requestCallUrlInternal: function(nickname, cb) {
> > +      var sessionType;
> > +      if (!this.mozLoop.userProfile) {
> > +        console.log("this.mozLoop.LOOP_SESSION_TYPE: ",
> > +          this.mozLoop.LOOP_SESSION_TYPE);
> 
> I'm guessing the console.log was just leftover from debugging.

Good catches, both.  Fixed.

All remaining nits fixed as well.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Support call URLs with guest or FxA hawk session types

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8491637 [details] [diff] [review]
Support call URLs with guest or FxA hawk session types

Review comments addressed; carrying forward r+.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Adding [loop-uplift], as I'm sure we want to uplift the Fx Accounts stuff.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b541be2a5459

Comment 22

[u279076]

Matt, I'm unclear how to test this using the latest Nightly. Can you please clarify steps to reproduce?

Comment 23

[Matthew N. [:MattN]]

Two aspects to test which require 2 profiles:

A) Verify that an FxA call URL is generated when logged in:
1) Login with FxA on profile A
2) Login with FxA on profile B
3) Copy (FxA) call URL from profile A.
4) Logout in profile A.
5) Load the call URL and initiate the call.

Expected result:
* The call should ring on profile B but not on profile A.

B) Verify that a non-FxA call URL is generated when logged out:
1) Login with FxA on profile A
2) Login with FxA on profile B
3) Logout on profile A.
4) Close the panel (this is a workaround for bug 1069750) on profile A.
5) Open the panel and copy the (non-FxA) call URL on profile A.
6) Load the call URL and initiate the call.

Expected result:
* The call should ring on profile A but not on profile B.

Comment 24

[u279076]

Thanks Matt.

Paul, can you please test this in the latest Nightly as per Matt's instructions?

Comment 25

[Paul Silaghi, QA [:pauly]]

Verified fixed 35.0a1 (2014-10-01) and http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352

Comment 26

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

Comment on attachment 8491637 [details] [diff] [review]
Support call URLs with guest or FxA hawk session types

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 27

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/1277219be1ca

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8491637 [details] [diff] [review]
Support call URLs with guest or FxA hawk session types

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.