TypeError: deriveHawkCredentials is not a function after bug 1020859

VERIFIED FIXED in Firefox OS v2.1

Status

()

Core
DOM: Device Interfaces
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

({regression, smoketest})

unspecified
mozilla33
regression, smoketest
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.1 verified)

Details

(Whiteboard: ft:loop)

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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

*************************
(Assignee)

Updated

4 years ago
Assignee: nobody → ferjmoreno
Component: Client → DOM: Device Interfaces
Product: Loop → Core
(Assignee)

Updated

4 years ago
Blocks: 1021594
(Assignee)

Comment 1

4 years ago
Created attachment 8442775 [details] [diff] [review]
v1
Attachment #8442775 - Flags: review?(jparsons)
Whiteboard: ft:loop
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...
(Assignee)

Comment 3

4 years ago
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 :\
Duplicate of this bug: 1027874
Setting as blocker to bug 1020859 to help people find this more easily (we normally use the blocker field when its a regression).
Blocks: 1020859
No longer depends on: 1020859

Updated

4 years ago
blocking-b2g: --- → 2.1+
Keywords: regression, smoketest
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!
Flags: needinfo?(jsmith)

Updated

4 years ago
blocking-b2g: 2.1+ → 2.0+
Flags: needinfo?(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?
Flags: needinfo?(oteo)
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.
Attachment #8442775 - Flags: review?(jparsons) → review+
Fernando, Jason asks in Comment 7 whether uplift for 2.0 is required.  What about the `hexKey` parameter - is that needed in 2.0?
Flags: needinfo?(ferjmoreno)

Comment 10

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

https://tbpl.mozilla.org/?tree=Try&rev=0ff2d831b76b
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 12

4 years ago
(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!
Flags: needinfo?(oteo)
Flags: needinfo?(ferjmoreno)
(Assignee)

Updated

4 years ago
blocking-b2g: 2.0+ → 2.1?

Updated

4 years ago
blocking-b2g: 2.1? → 2.1+
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/2048ba3b8468
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
status-b2g-v2.1: --- → fixed
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
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.1: fixed → verified
Flags: needinfo?(pbylenga)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
You need to log in before you can comment on or make changes to this bug.