Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Description • 11 years ago

      No description provided.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 1 • 11 years ago

This patch addresses various problems that cropped up in Bug 943521.

ferjm, Can I ask for your review of this change in particular:

- FxAccounts.jsm getKeys() was returning a null promise in cases like the one tested in test_accounts.js test_overlapping_signin().  This has been fixed in FxAccounts.jsm.

ttaubert, Can I ask for your approval of these changes to test_client.js:

- Expected exceptions were not being proved before; they are now
- Removed a few straggling 'run_next_test()' calls from within add_task functions

Thank you both!

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 2 • 11 years ago

Comment on attachment 8372871 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

Note - apply over patch for Bug 943521

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 3 • 11 years ago

Pushed to try, which seems to have shoved some other patches in there as well (not mine).  Not sure what that's about.  Hope it's ok.

https://tbpl.mozilla.org/?tree=Try&rev=21609cca9130

	Tim Taubert [:ttaubert] (inactive)
	Comment 4 • 11 years ago

Comment on attachment 8372871 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

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

Do you think we should still split the tests? I feel like it's a little more readable if we don't.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +25,5 @@
> +              "3031323334353637 38393a3b3c3d3e3f"),
> + 
> +  wrapKB:   h("4041424344454647 48494a4b4c4d4e4f"+
> +              "5051525354555657 58595a5b5c5d5e5f"),
> +};

Nit: There are some trailing/leading white spaces in here.

@@ +208,1 @@
>    }

If you expect something to fail I think it would be better to do:

try {
  lets_fail_here();
  do_throw("we shouldn't get here");
} catch (e) {
  do_check_eq(e, error)'
}

@@ +457,5 @@
> +      response.bodyOutputStream.write(existsMessage, existsMessage.length);
> +    },
> +  });
> + 
> +   let client = new FxAccountsClient(server.baseURI);

Nit: wrong indentation and a few leading white spaces around here.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 5 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Comment on attachment 8372871 [details] [diff] [review]
> Misc. FxAccounts test fixes, with one FxAccounts.jsm fix
> 
> Review of attachment 8372871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you think we should still split the tests? I feel like it's a little more
> readable if we don't.

I wasn't sure.  Since you think so, I've put it back how it was.

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +25,5 @@
> > +              "3031323334353637 38393a3b3c3d3e3f"),
> > + 
> > +  wrapKB:   h("4041424344454647 48494a4b4c4d4e4f"+
> > +              "5051525354555657 58595a5b5c5d5e5f"),
> > +};
> 
> Nit: There are some trailing/leading white spaces in here.

Sorry about that.  (Moot now, as this part has been restored to its previous location.)

> @@ +208,1 @@
> >    }
> 
> If you expect something to fail I think it would be better to do:
> 
> try {
>   lets_fail_here();
>   do_throw("we shouldn't get here");
> } catch (e) {
>   do_check_eq(e, error)'
> }

Oh of course.  That makes so much more sense.  Thanks.

> @@ +457,5 @@
> > +      response.bodyOutputStream.write(existsMessage, existsMessage.length);
> > +    },
> > +  });
> > + 
> > +   let client = new FxAccountsClient(server.baseURI);
> 
> Nit: wrong indentation and a few leading white spaces around here.

Sorry again.  (Again, moot; restored to its former state.)

Thanks, Tim!

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 6 • 11 years ago

Addressed issues in comment 4

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 7 • 11 years ago

Comment on attachment 8372887 [details] [diff] [review]
969892-misc-fixes.patch

oops

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 8 • 11 years ago

major patch merge snafu.  sorry about that.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 9 • 11 years ago

Tim, I've put the test vectors back at the top of the file as a const after all.

	Fernando Jiménez Moreno [:ferjm]
	Comment 10 • 11 years ago

Comment on attachment 8372897 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

LGTM

	Fernando Jiménez Moreno [:ferjm]
	Comment 11 • 11 years ago

Comment on attachment 8372897 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

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

::: services/fxaccounts/FxAccounts.jsm
@@ +333,1 @@
>        if (!this.whenKeysReadyPromise) {

You can remove this if

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 12 • 11 years ago

Oh.  Duh.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 13 • 11 years ago

r=ferjm

	Tim Taubert [:ttaubert] (inactive)
	Comment 14 • 11 years ago

Comment on attachment 8373447 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

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

The test changes look a lot better, thanks!

::: services/fxaccounts/FxAccounts.jsm
@@ +332,1 @@
>        }

For later/Off topic: we should probably rename this field to "whenKeysReadyDeferred" or something. Because a promise doesn't have a .promise property. We shouldn't do that here though :)

@@ +332,3 @@
>        }
> +      this.whenKeysReadyPromise = Promise.defer();
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {

I don't understand, how could getKeys() previously return null here? Did you check that we don't break any expectations here by returning |this.fetchAndUnwrapKeys()| that might be resolved even when abortExistingFlow() is called?

@@ +333,5 @@
> +      this.whenKeysReadyPromise = Promise.defer();
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> +        if (this.whenKeysReadyPromise) {
> +          this.whenKeysReadyPromise.resolve(data);
> +        }

If we can't reject fetchAndUnwrapKeys() we should probably save a local reference to the deferred created on line 333 and compare that to this.whenKeysReadyPromise before resolving. It might be a key request for an old user that makes us resolve a newer promise here, no? I know this isn't caused by your patch but we should file a follow-up for this.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +291,2 @@
>          response.setStatusLine(request.httpVersion, 200, "OK");
>          return response.bodyOutputStream.write(emptyMessage, emptyMessage.length);

Nit: put the return on a separate line so that it doesn't seem like we return for anything else than control flow reasons

@@ +293,5 @@
>        }
>  
>        // Second call gets an error trying to query a nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
>        return response.bodyOutputStream.write(errorMessage, errorMessage.length);

Nit: remove the return here as well, please

@@ -412,5 @@
>  
>    yield deferredStop(server);
>  });
>  
> -add_task(function test_accountExists() {

Did you really want to remove the test_accountExists()? Is that covered elsewhere now and I missed it?

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 15 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #14)
> Comment on attachment 8373447 [details] [diff] [review]
> Misc. FxAccounts test fixes, with one FxAccounts.jsm fix
> 
> Review of attachment 8373447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The test changes look a lot better, thanks!

Thanks, Tim!

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +332,1 @@
> >        }
> 
> For later/Off topic: we should probably rename this field to
> "whenKeysReadyDeferred" or something. Because a promise doesn't have a
> .promise property. We shouldn't do that here though :)

Well, it's easy enough to fix the names!  Done for whenKeysVerifiedDeferred and whenKeysReadyDeferred.

> @@ +332,3 @@
> >        }
> > +      this.whenKeysReadyPromise = Promise.defer();
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> 
> I don't understand, how could getKeys() previously return null here? Did you
> check that we don't break any expectations here by returning
> |this.fetchAndUnwrapKeys()| that might be resolved even when
> abortExistingFlow() is called?

I think the problem was that there was no 'return' statement inside the 'if (!this.whenKeysReadyPromise)', so it was falling through to 'return this.whenKeysReadyPromise;', which was still null at that point.

I believe this is still doing the right thing for the success case: we want to return the fetchAndUnwrapKeys deferred so that down the line callers of getKeys can actually get kA etc.  (For example, test_keyFetch checks this.)

I believe if there were a problem, 'test_overlapping_signins' in test_accounts.js would catch it.  Finally, we've been doing manual testing (thank you ferjm and spenrose), and haven't encountered any problems.

> @@ +333,5 @@
> > +      this.whenKeysReadyPromise = Promise.defer();
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> > +        if (this.whenKeysReadyPromise) {
> > +          this.whenKeysReadyPromise.resolve(data);
> > +        }
> 
> If we can't reject fetchAndUnwrapKeys() we should probably save a local
> reference to the deferred created on line 333 and compare that to
> this.whenKeysReadyPromise before resolving. It might be a key request for an
> old user that makes us resolve a newer promise here, no? I know this isn't
> caused by your patch but we should file a follow-up for this.

That's a really good point.

I've filed Bug 971173

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +291,2 @@
> >          response.setStatusLine(request.httpVersion, 200, "OK");
> >          return response.bodyOutputStream.write(emptyMessage, emptyMessage.length);
> 
> Nit: put the return on a separate line so that it doesn't seem like we
> return for anything else than control flow reasons

There were a lot of these.  All fixed.

> @@ +293,5 @@
> >        }
> >  
> >        // Second call gets an error trying to query a nonexistent account
> >        response.setStatusLine(request.httpVersion, 400, "Bad request");
> >        return response.bodyOutputStream.write(errorMessage, errorMessage.length);
> 
> Nit: remove the return here as well, please

Ditto

> @@ -412,5 @@
> >  
> >    yield deferredStop(server);
> >  });
> >  
> > -add_task(function test_accountExists() {
> 
> Did you really want to remove the test_accountExists()? Is that covered
> elsewhere now and I missed it?

Good heavens, where did it go?  It must have been obliterated when I rebased my patch somehow.  Crikey, thanks for catching that.  Reincorporated.

I've also updated the _deriveHawkCredentials test.  The original test derived the correct results, but it was not using published test vectors.  I've added the vectors to the head of the file.

Thanks for your review, Tim!

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 16 • 11 years ago

Addresses issues in Comment 14

	Tim Taubert [:ttaubert] (inactive)
	Comment 17 • 11 years ago

Comment on attachment 8374334 [details] [diff] [review]
Misc. FxAccounts test fixes, with one FxAccounts.jsm fix

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

I tried applying the patch to test things locally but the patch didn't apply at all. Can you please rebase, Jed?

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +489,5 @@
>    let client = new FxAccountsClient(server.baseURI);
>    let result;
>  
> +  result = yield client.accountExists("i.exist@example.com");
> +  do_check_eq(result, true);

Do these not throw anymore? I thought the promises would still be reject?

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 18 • 11 years ago

Thanks for looking, Tim.

They actually didn't throw to begin with.  That was a huge error with the tests.  The accountExists() method normally yields true or false (it catches the 400 response internally and checks the errno code).

Sorry the patch didn't apply.  Rebasing now.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 19 • 11 years ago

Looks like Bug 972093 beat us to the removal of all the run_next_test() calls.

Rebased.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 20 • 11 years ago

rebased

	Tim Taubert [:ttaubert] (inactive)
	Comment 21 • 11 years ago

Comment on attachment 8379830 [details] [diff] [review]
969892-misc-fixes.patch

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

The tests look good to me but I'd like to get some feedback about the FxAccounts.jsm change again...

::: services/fxaccounts/FxAccounts.jsm
@@ +344,5 @@
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> +        if (this.whenKeysReadyDeferred) {
> +          this.whenKeysReadyDeferred.resolve(data);
> +        }
> +      });

Hmmm... I'm still not really convinced that it's the right thing to do to change that here and return .fetchAndUnwrapKeys().

.whenKeysReadyDeferred can be rejected by abortExistingFlow(). BrowserIDManager.initializeWithCurrentIdentity() can thus be aborted.

The promise returned by .fetchAndUnwrapKeys() however will not be aborted by abortExistingFlow() but will just "return null" when it encounters an unexpected generation. That would lead to initializeWithCurrentIdentity() completing the setup, no?

I don't really know the affected code too well but I don't see how the previous version could return null. Line 341 creates a deferred if there is none that then is returned on line 348. fetchAndUnwrapKeys() doesn't null it so I don't see how this function could return null in that case?

FxA.getKeys() does always return a promise, if whenKeysReadyDeferred would be null it would just resolve to "null" instead of deferring the resolution to another promise, no? Can you reproduce the error you mentioned with the old version? What is the exact error you get?

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +152,5 @@
>  
>        // Error trying to create same account a second time
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: The return here seems unneeded. Also that maybe just do:

if (created) {
  ...
} else {
  ...
}

@@ +192,5 @@
>  
>        // Error trying to sign in to nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: Same about the return here, and also if (jsonBody.email == "mé@example.com") {} else {}.

@@ +229,5 @@
>  
>        // Error trying to sign out of nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: remove return and if () {} else {}

@@ +267,5 @@
>  
>        // Second call gets an error trying to query a nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: same :)

@@ +304,5 @@
>  
>        // Second call gets an error trying to query a nonexistent account
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: same.

@@ +427,5 @@
>  
>        // Second attempt, trigger error
>        response.setStatusLine(request.httpVersion, 400, "Bad request");
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;

Nit: *cough*

@@ +489,5 @@
>    let client = new FxAccountsClient(server.baseURI);
>    let result;
>  
> +  result = yield client.accountExists("i.exist@example.com");
> +  do_check_eq(result, true);

Nit: do_check_true() would be better here.

	Sam Penrose
	Comment 22 • 11 years ago

Rereading Jed's patch carefully, as best I can tell he has not changed the logic in getKeys() at all, just variable names and if block ordering. Can we review the changes he has made here, and if necessary open a separate issue to discuss how getKeys() should be structured?

(In reply to Tim Taubert [:ttaubert] from comment #21)
> Comment on attachment 8379830 [details] [diff] [review]
> 969892-misc-fixes.patch
> 
> Review of attachment 8379830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests look good to me but I'd like to get some feedback about the
> FxAccounts.jsm change again...
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +344,5 @@
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> > +        if (this.whenKeysReadyDeferred) {
> > +          this.whenKeysReadyDeferred.resolve(data);
> > +        }
> > +      });
> 
> Hmmm... I'm still not really convinced that it's the right thing to do to
> change that here and return .fetchAndUnwrapKeys().

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 23 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #21)
> Comment on attachment 8379830 [details] [diff] [review]
> 969892-misc-fixes.patch

Thanks for your review, Tim!

> Review of attachment 8379830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The tests look good to me but I'd like to get some feedback about the
> FxAccounts.jsm change again...

Since Bug 972070 landed, the landscape has changed and I no longer feel any modification to this part of the code is necessary.  But for the record, let me respond with what I was thinking at the time.

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +344,5 @@
> > +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> > +        if (this.whenKeysReadyDeferred) {
> > +          this.whenKeysReadyDeferred.resolve(data);
> > +        }
> > +      });
> 
> Hmmm... I'm still not really convinced that it's the right thing to do to
> change that here and return .fetchAndUnwrapKeys().
> 
> .whenKeysReadyDeferred can be rejected by abortExistingFlow().
> BrowserIDManager.initializeWithCurrentIdentity() can thus be aborted.
> 
> The promise returned by .fetchAndUnwrapKeys() however will not be aborted by
> abortExistingFlow() but will just "return null" when it encounters an
> unexpected generation. That would lead to initializeWithCurrentIdentity()
> completing the setup, no?
> 
> I don't really know the affected code too well but I don't see how the
> previous version could return null. Line 341 creates a deferred if there is
> none that then is returned on line 348. fetchAndUnwrapKeys() doesn't null it
> so I don't see how this function could return null in that case?
> 
> FxA.getKeys() does always return a promise, if whenKeysReadyDeferred would
> be null it would just resolve to "null" instead of deferring the resolution
> to another promise, no? Can you reproduce the error you mentioned with the
> old version? What is the exact error you get?

I think the source of the problem I was wrestling with is that the promise could become nulled halfway through the spawned task in fetchAndUnwrapKeys, with the result that after falling through a test for whether the deferred did not exist, by the time the next line of code was executed, in the expectation that it existed, it suddenly did *not* exist!  This would cause test_getKeys to explode.

But all of this is moot now as markh has fixed the problem for me!  (Thanks, Mark :)

My latest patch will not have any changes to this part.

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +152,5 @@
> >  
> >        // Error trying to create same account a second time
> >        response.setStatusLine(request.httpVersion, 400, "Bad request");
> > +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> > +      return;
> 
> Nit: The return here seems unneeded. Also that maybe just do:
> 
> if (created) {
>   ...
> } else {
>   ...
> }

For this and all the others you cite, the return is purely for flow control.

The useless return at the end of the block saves me from strict warnings that "anonymous function does not always return a value."  Given the choice, I personally prefer the final return to an if ... else ... construct, because it ensures that no extra code will inadvertently be tagged onto the end someday. 

But if you still prefer that I replace the 'return' statements, I'm happy to do so.  I'm not wedded to them :)

> @@ +489,5 @@
> >    let client = new FxAccountsClient(server.baseURI);
> >    let result;
> >  
> > +  result = yield client.accountExists("i.exist@example.com");
> > +  do_check_eq(result, true);
> 
> Nit: do_check_true() would be better here.

Righto.  Thanks.

Thanks again for all your time on this.  j

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 24 • 11 years ago

Rebased

	Tim Taubert [:ttaubert] (inactive)
	Comment 25 • 11 years ago

Comment on attachment 8385537 [details] [diff] [review]
Misc. FxAccounts test fixes

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

::: services/fxaccounts/FxAccounts.jsm
@@ +334,5 @@
> +      return this.fetchAndUnwrapKeys(data.keyFetchToken).then(data => {
> +        if (this.whenKeysReadyDeferred) {
> +          this.whenKeysReadyDeferred.resolve(data);
> +        }
> +      });

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #23)
> My latest patch will not have any changes to this part.

:O

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +489,5 @@
>    let client = new FxAccountsClient(server.baseURI);
>    let result;
>  
> +  result = yield client.accountExists("i.exist@example.com");
> +  do_check_eq(result, true);

do_check_true()

@@ +494,3 @@
>  
> +  result = yield client.accountExists("i.also.exist@example.com");
> +  do_check_eq(result, true);

do_check_true()

@@ +497,3 @@
>  
> +  result = yield client.accountExists("i.dont.exist@example.com");
> +  do_check_eq(result, false);

do_check_false()

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 26 • 11 years ago

Thanks for taking a look, Tim.

You canceled the review - are there other problems?  Or could this be r=ttaubert if I make those three changes to test_client.js?

Thanks
j

	Tim Taubert [:ttaubert] (inactive)
	Comment 27 • 11 years ago

Please see the part where you said you will not include certain changes anymore :) Basically, r=me with those changes reverted.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 28 • 11 years ago

Aha!  I think I messed up in my patch export!  Thank you for your review, Tim.  Fixes forthcoming.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 29 • 11 years ago

r=ttaubert

Sorry about the mix-up, Tim.  I must up uploaded the last patch without doing 'hg export' first!  I can imagine you must have been wondering what I was thinking.

Cheers
j

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

Needs rebasing.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 31 • 11 years ago

r=ttaubert

rebased

	Tim Taubert [:ttaubert] (inactive)
	Comment 32 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/945ddca04d49