U2FTokenTransport::Register() and ::Sign() should return promises

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [webauthn])

Attachments

(1 attachment, 2 obsolete attachments)

The softtoken is sync, so we didn't need this until now. For USB tokens however we need an async API. Luckily, this isn't so hard to with MozPromises.
Summary: U2FTokenTransport::Register() and ::Sign() should async APIs and return promises → U2FTokenTransport::Register() and ::Sign() should return promises
Comment on attachment 8880787 [details] [diff] [review]
0004-Bug-1375828-U2FTokenTransport-Register-and-Sign-shou.patch

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

::: dom/webauthn/U2FTokenManager.cpp
@@ +211,5 @@
> +                                               reg,
> +                                               sig);
> +
> +  mResultPromise->Then(GetCurrentThreadSerialEventTarget(), __func__,
> +                       [this, aTransactionParent, reg, sig](nsresult rv) {

Here and below: I don't think you can capture 'this' in a closure (I think static analysis will get angry if you run this through try). We have no way to keep 'this' alive in a guaranteed way until the non-deterministic point that the closure's then clause runs. When I was still fetching prefs via promises in the child, I had to get around this by using singletons. Not sure that's really an easy option here though?
Attachment #8880787 - Flags: review?(kyle) → review-
(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #2)
> Here and below: I don't think you can capture 'this' in a closure (I think
> static analysis will get angry if you run this through try). We have no way
> to keep 'this' alive in a guaranteed way until the non-deterministic point
> that the closure's then clause runs. When I was still fetching prefs via
> promises in the child, I had to get around this by using singletons. Not
> sure that's really an easy option here though?

Good point, I didn't realize this when I wrote it.

I changed the callbacks to now also use the U2FTokenManager::Get() singleton, it's actually quite straightforward. As ::Sign() and ::Register() are always only called by the same thread, the PBackground thread in the parent process, I think that this should be safe?
Attachment #8880787 - Attachment is obsolete: true
Attachment #8881444 - Flags: review?(kyle)
Comment on attachment 8881444 [details] [diff] [review]
0002-Bug-1375828-U2FTokenTransport-Register-and-Sign-shou.patch, v2

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

::: dom/webauthn/U2FTokenManager.cpp
@@ +220,5 @@
> +                                               reg,
> +                                               sig);
> +
> +  mResultPromise->Then(GetCurrentThreadSerialEventTarget(), __func__,
> +                       [aTransactionParent, reg, sig](nsresult rv) {

Use counter, as discussed in meeting
Attachment #8881444 - Flags: review?(kyle) → review-
Use a transaction ID instead of capturing a raw pointer to the transaction parent.
Attachment #8881444 - Attachment is obsolete: true
Attachment #8882986 - Flags: review?(kyle)
Comment on attachment 8882986 [details] [diff] [review]
0001-Bug-1375828-U2FTokenTransport-Register-and-Sign-shou.patch, v3

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

::: dom/webauthn/U2FTokenManager.cpp
@@ +148,5 @@
>  void
> +U2FTokenManager::MaybeAbortTransaction(uint64_t aTransactionId,
> +                                       const nsresult& aError)
> +{
> +  if (mTransactionId == aTransactionId) {

nit: I usually exited early on code around this, so just to keep style here and below, maybe check negation and return early instead of just having one if-statement for the function?
Attachment #8882986 - Flags: review?(kyle) → review+
(In reply to Kyle Machulis [:qdot] [:kmachulis]  (if a patch has no decent commit message, automatic r-) from comment #6)
> >  void
> > +U2FTokenManager::MaybeAbortTransaction(uint64_t aTransactionId,
> > +                                       const nsresult& aError)
> > +{
> > +  if (mTransactionId == aTransactionId) {
> 
> nit: I usually exited early on code around this, so just to keep style here
> and below, maybe check negation and return early instead of just having one
> if-statement for the function?

Sure, will do. Thanks!
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b74325b71af5
U2FTokenTransport::Register() and ::Sign() should return promises r=qDot
https://hg.mozilla.org/mozilla-central/rev/b74325b71af5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.