1027595 - TypeError: deriveHawkCredentials is not a function after bug 1020859

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

Bug 1020859 makes the Mobile ID API crash with:

*************************
A coding exception was thrown in a Promise resolution callback.

Full message: TypeError: deriveHawkCredentials is not a function
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full stack: this.MobileIdentityClient.prototype._deriveHawkCredentials@resource://gre/modules/MobileIdentityClient.jsm:109:1
this.MobileIdentityClient.prototype.sign@resource://gre/modules/MobileIdentityClient.jsm:75:9
MobileIdentityManager.getCertificate@resource://gre/modules/MobileIdentityManager.jsm:239:1
MobileIdentityManager.generateAssertion/<@resource://gre/modules/MobileIdentityManager.jsm:614:1
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:863:11
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:7

*************************

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8442775 [details] [diff] [review]
v1

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

::: services/common/hawkrequest.js
@@ +116,5 @@
>    *          key: the Hawk key (from bytes 32 to 64)
>    *          extra: size - 64 extra bytes (if size > 64)
>    *        }
>    */
> +this.deriveHawkCredentials = function deriveHawkCredentials(tokenHex,

Thanks for fixing this, it is really strange how this breaks on Mobile, but desktop is fine...

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

The reason is because on b2g we run with jsloader.reuseGlobal set to true (bug 798491).

I definitely need to write the tests for the Mobile ID API :\

Comment 5

[Mark Banner (:standard8)]

Setting as blocker to bug 1020859 to help people find this more easily (we normally use the blocker field when its a regression).

Comment 6

[Maria Angeles Oteo (:oteo)]

Jason we need it to be fixed in 2.0 version, can you set it with blocking-b2g:2.0+ instead of 2.1?
Thanks a lot!

Comment 7

[Jason Smith [:jsmith]]

(In reply to Maria Angeles Oteo (:oteo) from comment #6)
> Jason we need it to be fixed in 2.0 version, can you set it with
> blocking-b2g:2.0+ instead of 2.1?
> Thanks a lot!

Can you clarify why? Is the regressing patch planning to be uplifted to 2.0?

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8442775 [details] [diff] [review]
v1

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

Thanks for jumping in with the fix, Fernando!

(I haven't run the tests on b2g and desktop; I trust they are happy again with this fix.)

::: services/common/hawkrequest.js
@@ +116,5 @@
>    *          key: the Hawk key (from bytes 32 to 64)
>    *          extra: size - 64 extra bytes (if size > 64)
>    *        }
>    */
> +this.deriveHawkCredentials = function deriveHawkCredentials(tokenHex,

As :ferjm has pointed out, it's because of reuseGlobal.  spenrose has opened Bug 1027951 to make global assignments to exported symbols fail loudly, which, because it would easily prevent this situation, I totally support.

I'm the one who gave the r+ to Attachment 8441351 [details] [diff] in bug 1020859, and I didn't catch it.  So at the very least, for people like me, I would love to see Bug 1027951 land :)

@@ +125,5 @@
>    let out = CryptoUtils.hkdf(token, undefined, Credentials.keyWord(context), size);
>  
>    let result = {
>      algorithm: "sha256",
> +    key: hexKey ? CommonUtils.bytesAsHex(out.slice(32, 64)) : out.slice(32, 64),

It would be nice to have a test for this switch.  But since the fix in this patch is urgent, and the default behavior doesn't change for existing callers, I'd be happy to see it happen in a follow-up bug.

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Fernando, Jason asks in Comment 7 whether uplift for 2.0 is required.  What about the `hexKey` parameter - is that needed in 2.0?

Comment 10

[Sam Penrose]

Setting checkin-needed; sherriffs here is the try build:

https://tbpl.mozilla.org/?tree=Try&rev=0ff2d831b76b

Comment 11

[Mark Banner (:standard8)]

I've pushed this to inbound, I suspect it'll make tomorrow's (Sunday's) nightlies. I might have pushed to central, but I'm busy, so haven't got time to watch the tree:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2048ba3b8468

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #8)

> 
> @@ +125,5 @@
> >    let out = CryptoUtils.hkdf(token, undefined, Credentials.keyWord(context), size);
> >  
> >    let result = {
> >      algorithm: "sha256",
> > +    key: hexKey ? CommonUtils.bytesAsHex(out.slice(32, 64)) : out.slice(32, 64),
> 
> It would be nice to have a test for this switch.  But since the fix in this
> patch is urgent, and the default behavior doesn't change for existing
> callers, I'd be happy to see it happen in a follow-up bug.

Thanks Jed. Since this was already pushed to inbound, I'll file the bug and work on the test on Monday.

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #9)
> Fernando, Jason asks in Comment 7 whether uplift for 2.0 is required.  What
> about the `hexKey` parameter - is that needed in 2.0?

This should not be uplifted to 2.0 unless the bug causing the regression is uplifted.

(In reply to Mark Banner (:standard8) from comment #11)
> I've pushed this to inbound, I suspect it'll make tomorrow's (Sunday's)
> nightlies. I might have pushed to central, but I'm busy, so haven't got time
> to watch the tree:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2048ba3b8468

Thanks Mark!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2048ba3b8468

Comment 14

[Peter Bylenga [:PBylenga]]

Was able to verify this bug as fixed with the STRs used in the duplicate bug 1027874.  We are able to sign into fx accounts without encountering an infinite loop.

Environmental Variables:
Device: Flame Master
Build ID: 20140623095853
Gaia: 41cc1de26e4edbe12add0009cdc0bd292f2e94fe
Gecko: 31de1a84b27f
Version: 33.0a1 (Master)
Firmware Version: v122

Comment 15

[Adam Alldredge [:AdamA][Inactive]]

Verified as fixed in 2.1 Flame.

Environmental Variables:
Device: Flame 2.1
BuildID: 20141110001201
Gaia: 0ec1925fc37b7c71d129ae44e42516a0cfb013c4
Gecko: 97487a2d1ee6
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

User is able to create a Firefox account