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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1375828-U2FTokenTransport-Register-and-Sign-shou.patch

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1375828-U2FTokenTransport-Register-and-Sign-shou.patch, v2

(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?

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1375828-U2FTokenTransport-Register-and-Sign-shou.patch, v3

Use a transaction ID instead of capturing a raw pointer to the transaction parent.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(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!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b74325b71af5
U2FTokenTransport::Register() and ::Sign() should return promises r=qDot

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b74325b71af5