1017394 - MozSocialAPI initialization issue prevents hawk bits from working

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Over at <https://github.com/adamroach/gecko-dev/pull/29#issuecomment-44410268>, Niko noticed an issue which looks like it was introduced in bug 994151:

callback is not a function MozLoopService.jsm:286

He proposed a patch over there.  MarkH and I chatted about the patch, and MarkH pointed out that the real bug is that initialize can be called twice, which shouldn't be possible.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I should have caught this as the reviewer, but I think I'm getting a little too fried from pushing too hard.  In any case, MarkH wasn't comfortable stamping this with r+, and proposes that we should fix whatever's causing initialize to get called twice.

(MarkH, feel free to correct if I've mischaracterized anything here).

Comment 2

[Mark Banner (:standard8)]

The current requests are:

- Use promises instead of callbacks for register/registerWithServers
- Expose something to make testing not require a callback/promise on initialize, since the callback isn't needed for production code

Comment 3

[Mark Banner (:standard8)]

Oh, Mark has asked that we fix this before landing on nightly, hence the milestone & deps.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with Standard8, I've landed a bandaid fix <https://github.com/adamroach/gecko-dev/commit/ad07682bed13235e2fb2e7f4098e9bb44901f37f> onto loop-service so that we can have something that actually works on elm. 

This bug will then be fixed correctly on elm before MLP lands on mozilla-central.

Comment 5

[Mark Banner (:standard8)]

Temporary change landed in elm: https://hg.mozilla.org/projects/elm/rev/0b7c551f9b2f

Comment 6

[Mark Hammond [:markh] [:mhammond]]

FTR, something which is closer to what we want is at https://github.com/mhammond/gecko-dev/commit/e9b4deef563f9952524209e9a8dfd62c3e3fc9d5 - although I don't think we want a promise to be returned from initialize, instead we should create a new function _startTimer or similar, and mock it for the tests.  You'll then be able to call initialize many times via the tests and have the mocked _startTimer record if it was called or not, satisfying what the tests are trying to do.

Comment 7

[Mark Banner (:standard8)]

Attachment: In MozLoopService use promises rather than callbacks to make the code cleaner.

This moves us to use promises within the loop service.

The MozLoopAPI still transistions between callbacks and promises, as we can't pass a promise from Promise.jsm from privilaged to content. We could potentially use content-space promises, if we can create them, though I'd be tempted to do that in a separate patch, once we know the outcome of bug 1017902.

I've moved the guts of the initalize function from MozLoopServiceInternal to MozLoopService as this seemed better for exposing the start timer function.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8434124 [details] [diff] [review]
In MozLoopService use promises rather than callbacks to make the code cleaner.

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

Thanks!  Tests would look better as tasks, but I won't insist on that :)  r=me with the other comments addressed

::: browser/components/loop/MozLoopService.jsm
@@ +229,4 @@
>      }
>  
> +    this._registeredDeferred = Promise.defer();
> +    let result = this._registeredDeferred.promise;

pls add a comment saying that you grab the promise early incase .initialise sets it back to null on error

@@ +402,3 @@
>     */
> +  initialize: function() {
> +    if (this._initalizeTimer || MozLoopServiceInternal._registeredDeferred) {

still not that keen on this test-only code.  Can you please add a .reset method that is commented as being used only for tests and undo stuff there (eg, cancel the timer, then reject and reset the deferred)?  If tests call it as they finish via a cleanup function, it will also ensure a timer doesn't fire after the test is complete, which could upset things

::: browser/components/loop/test/xpcshell/test_loopservice_registration.js
@@ +72,5 @@
>      var hawkSessionPref;
>      try {
>        hawkSessionPref = Services.prefs.getCharPref("loop.hawk-session-token");
>      } catch (ex) {
>        // avoid slowing/hanging the tests if the pref isn't there

I know you didn't touch this, but it's suspect - either it should throw, or should be removed (or at least re-worded - it looks expected to me!)

Comment 9

[Mark Banner (:standard8)]

(In reply to Mark Hammond [:markh] from comment #8)
> > +    this._registeredDeferred = Promise.defer();
> > +    let result = this._registeredDeferred.promise;
> 
> pls add a comment saying that you grab the promise early incase .initialise
> sets it back to null on error

Added, also clarified when _registeredDeferred is set at its definition.

> > +  initialize: function() {
> > +    if (this._initalizeTimer || MozLoopServiceInternal._registeredDeferred) {
> 
> still not that keen on this test-only code.  Can you please add a .reset
> method that is commented as being used only for tests and undo stuff there
> (eg, cancel the timer, then reject and reset the deferred)?

As discussed on irc, I've removed this if statement.
Y
> ::: browser/components/loop/test/xpcshell/test_loopservice_registration.js
> @@ +72,5 @@
> >      var hawkSessionPref;
> >      try {
> >        hawkSessionPref = Services.prefs.getCharPref("loop.hawk-session-token");
> >      } catch (ex) {
> >        // avoid slowing/hanging the tests if the pref isn't there
> 
> I know you didn't touch this, but it's suspect - either it should throw, or
> should be removed (or at least re-worded - it looks expected to me!)

Yeah, that was just wrong, I've removed everything from the catch, the Assert will correctly handle the failure.

Comment 10

[Mark Banner (:standard8)]

Attachment: In MozLoopService use promises rather than callbacks to make the code cleaner. v2

Updated patch for Mark's comments.

Comment 11

[Mark Banner (:standard8)]

https://hg.mozilla.org/projects/elm/rev/f76b6f96fecd

Landed on Elm, closing for tracking purposes.

Comment 12

[Mark Banner (:standard8)]

https://hg.mozilla.org/mozilla-central/rev/0b7c551f9b2f
https://hg.mozilla.org/mozilla-central/rev/f76b6f96fecd

Comment 13

[u279076]

Dan, could you please advise whether this needs testing and what are the steps to reproduce this?

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

I don't believe this needs testing.

Comment 15

[u279076]

(In reply to Dan Mosedale (:dmose - needinfo? me for responses) from comment #14)
> I don't believe this needs testing.

Thanks, assuming verified fixed. Please needinfo me if you change your mind and decide this needs some testing.