1379580 - U2FTokenTransport promises should resolve to U2F data buffers

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, enhancement)

Description

[Tim Taubert [:ttaubert] (inactive)]

Currently, we call U2FTokenTransport::Register() and Sign() and pass stack references that we expect to be filled when the promise resolves. The then() callback itself copies the nsTArrays, so this probably works by accident?

Well, this is bad. We should have proper Register/Sign promises that return a result specific to the request type. That result then holds the u8 buffers returned by the token.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1379580-U2FTokenTransport-promises-should-resolv.patch

Comment 2

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

Comment on attachment 8884769 [details] [diff] [review]
0001-Bug-1379580-U2FTokenTransport-promises-should-resolv.patch

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

r+, though I do have one question.

::: dom/webauthn/U2FTokenManager.cpp
@@ +322,5 @@
>  }
>  
>  void
>  U2FTokenManager::MaybeConfirmSign(uint64_t aTransactionId,
> +                                  U2FSignResult&& aResult)

Just curious, why the move semantics here and in register? I think the call to SendConfirmSign/Register is just going to copy these elements into a message block for IPC, so afaik there's nothing after this that really needs the ownership? Doesn't really hurt anything and I suppose if you're following the Promise API, it's fine. Just want to make sure I'm not missing something.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kyle Machulis [:qdot] [:kmachulis] from comment #2)
> >  void
> >  U2FTokenManager::MaybeConfirmSign(uint64_t aTransactionId,
> > +                                  U2FSignResult&& aResult)
> 
> Just curious, why the move semantics here and in register? I think the call
> to SendConfirmSign/Register is just going to copy these elements into a
> message block for IPC, so afaik there's nothing after this that really needs
> the ownership? Doesn't really hurt anything and I suppose if you're
> following the Promise API, it's fine. Just want to make sure I'm not missing
> something.

You're right, I went a little overboard with the move semantics here. We actually just need to pass by reference from the promise->Then() callback.

Thanks!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/695e774df6c6
U2FTokenTransport promises should resolve to U2F data buffers r=qDot

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/695e774df6c6