Closed Bug 1384623 Opened 7 years ago Closed 7 years ago

WebAuthn objects marked [SameObject] must cache those objects

Categories

(Core :: DOM: Device Interfaces, enhancement, P2)

55 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jcj, Assigned: jcj)

References

()

Details

(Whiteboard: [webauthn])

Attachments

(1 file)

Peter points out [1] that I made assumptions that [SameObject] would handle caching at the JS-layer, but it does not. This bug is to cache those objects [2] properly.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1382888#c6
[2] https://hg.mozilla.org/mozilla-central/rev/811510fdb51a
(In reply to Peter Van der Beken [:peterv] from comment #2)
> There's also
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached and
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#StoreInSlot,
> which cache on the JS object.

Indeed, I figured [SameObject] would have the same kind of semantics. Since WebAuthn went with [SameObject], I think this is the right move - stay in-line with the spec. Thanks again for the catch, Peter. Please comment if you see any issues with the attached patch.
Comment on attachment 8890444 [details]
Bug 1384623 - WebAuthn [SameObject] attributes must cache those objects

https://reviewboard.mozilla.org/r/161576/#review170968

::: dom/webauthn/AuthenticatorAssertionResponse.h:50
(Diff revision 1)
>    nsresult
>    SetSignature(CryptoBuffer& aBuffer);
>  
>  private:
>    CryptoBuffer mAuthenticatorData;
> +  JS::Heap<JSObject*> mAuthenticatorDataCachedObj;

So here and pretty much everywhere else, you're missing cycle collection which means we could possibly leak these JS::Heap members. Since we're holding JS objects both here in C++ and in JS, it makes the code way more FUN (in as much as lots of all caps macros is FUN). We need to let the cycle collector know that it might need to clean these up. This means you'll need to add

NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMEBER_CALLBACK

calls for all of your JS::Heap members here and in other files. 

You'll also need to null out all of your members on unlink. This is just done by setting 

tmp->[member] = nullptr;

in your UNLINK macro.

I realize this is probably an oversimplified explanation. Luckily, there's lots of examples of how this should work scattered around the code, for instance in dom/base/Pose.h|cpp (lots of JS::Heap<Object*>'s there).
Attachment #8890444 - Flags: review?(kyle) → review-
Comment on attachment 8890444 [details]
Bug 1384623 - WebAuthn [SameObject] attributes must cache those objects

https://reviewboard.mozilla.org/r/161576/#review170968

> So here and pretty much everywhere else, you're missing cycle collection which means we could possibly leak these JS::Heap members. Since we're holding JS objects both here in C++ and in JS, it makes the code way more FUN (in as much as lots of all caps macros is FUN). We need to let the cycle collector know that it might need to clean these up. This means you'll need to add
> 
> NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMEBER_CALLBACK
> 
> calls for all of your JS::Heap members here and in other files. 
> 
> You'll also need to null out all of your members on unlink. This is just done by setting 
> 
> tmp->[member] = nullptr;
> 
> in your UNLINK macro.
> 
> I realize this is probably an oversimplified explanation. Luckily, there's lots of examples of how this should work scattered around the code, for instance in dom/base/Pose.h|cpp (lots of JS::Heap<Object*>'s there).

Thanks for the review! I've put together a patch to do this, in monkey see/do fashion, which I have no confidence in. I'll ping on IRC for additional help.
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Keywords: stale-bug
Priority: P1 → P2
Kyle: I read through http://blog.kylehuey.com/post/27564411715/cycle-collection and think I applied it correctly. This patch appears to .. work? It also needs a rebase before it can land due to bug 1391005, but that's pretty simple and will clog up the review a bit, so I'll upload the rebase after you take a look here.

Thanks!
Comment on attachment 8890444 [details]
Bug 1384623 - WebAuthn [SameObject] attributes must cache those objects

https://reviewboard.mozilla.org/r/161576/#review181484

LGTM with the cleanup. Not really hurting anything as is though.

::: dom/webauthn/AuthenticatorAssertionResponse.cpp:17
(Diff revision 2)
>  
> +NS_IMPL_CYCLE_COLLECTION_CLASS(AuthenticatorAssertionResponse)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(AuthenticatorAssertionResponse,
> +                                                AuthenticatorResponse)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mAuthenticatorData)
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK(mSignature)

Here and below:

You just need the tmp->[whatever] = nullptr calls, in order to drop the references to the JS::Heap objects. 

Unlinking the nsTArray calls Clear, and a Cryptobuffer holds uint8_t, which isn't going to be tracked, so this will just be done automatically for you on destruction. Unlinking an nsTArray in the way you've done it is handy for if you have an nsTArray of JS::Heap objects or something. In the ToUint8Array called later there's a memcpy, so the memory isn't shared between your CryptoBuffer and your heap object.
Attachment #8890444 - Flags: review?(kyle) → review+
Comment on attachment 8890444 [details]
Bug 1384623 - WebAuthn [SameObject] attributes must cache those objects

https://reviewboard.mozilla.org/r/161576/#review181484

> Here and below:
> 
> You just need the tmp->[whatever] = nullptr calls, in order to drop the references to the JS::Heap objects. 
> 
> Unlinking the nsTArray calls Clear, and a Cryptobuffer holds uint8_t, which isn't going to be tracked, so this will just be done automatically for you on destruction. Unlinking an nsTArray in the way you've done it is handy for if you have an nsTArray of JS::Heap objects or something. In the ToUint8Array called later there's a memcpy, so the memory isn't shared between your CryptoBuffer and your heap object.

Thanks, qdot. Question: Should I still be calling TRAVERSE on those CryptoBuffer objects?
Comment on attachment 8890444 [details]
Bug 1384623 - WebAuthn [SameObject] attributes must cache those objects

Kyle - Thanks for the help. Can you take another look, with the correct constructor/destructor semantics?

(Apologies on the test changes -- those are from the rebase.)
Attachment #8890444 - Flags: review+ → review?(kyle)
Comment on attachment 8890444 [details]
Bug 1384623 - WebAuthn [SameObject] attributes must cache those objects

https://reviewboard.mozilla.org/r/161576/#review181564
Attachment #8890444 - Flags: review?(kyle) → review+
Target Milestone: mozilla56 → ---
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53ef4a4139b9
WebAuthn [SameObject] attributes must cache those objects r=qdot
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53ef4a4139b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.