1517611 - Leaks in /webauthn/ WPTs

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andrew McCreight [:mccr8] (PTO 9/16-9/23)]

Leak sanitizer found some leaks in the web platform tests in the /webauthn/ directory.

The leaks detected are various JS gunk, plus this:

leak at mozilla::dom::DOMException::Create, mozilla::dom::CreateException, mozilla::dom::ToJSValue, MaybeSomething
leak at mozilla::dom::Navigator::Credentials, mozilla::dom::Navigator_Binding::get_credentials, mozilla::dom::binding_detail::GenericGetter, CallJSNative

The second line there seems to be a CredentialsContainer object, which is traversed but not unlinked by Navigator, so maybe that's part of the problem.

Comment 1

[Andrew McCreight [:mccr8] (PTO 9/16-9/23)]

I suspect that every test in this directory leaks. I tried webauthn/createcredential-excludecredentials.https.html. The leak keeps NSS alive, so you need to comment out the line "MOZ_CRASH("NSS_Shutdown failed");" in xpcom/build/XPCOMInit.cpp or it will just crash without giving you any crash information.

Anyways, I think the problem is that WebAuthnManager needs to be cycle collected, because its base class WebAuthnManagerBase contains a strong reference to a window. The other subclass of WebAuthnManagerBase, U2F, is already CCed. It'll probably end up making sense to make WebAuthnManagerBase cycle collected. At a glance, I don't see the two subclasses need their own implementation of refcounting.

CredentialsContainer has a field that is a strong reference to WebAuthnManager, but the cycle collector already knows about that.

Comment 2

[Andrew McCreight [:mccr8] (PTO 9/16-9/23)]

These leaks are pretty bad. I think we'll just leak the entire tab for any webpage that uses this. It looks like this API is enabled by default.

Comment 3

[J.C. Jones [:jcj] (he/him)]

Yes, it shipped in Firefox 60, but so far is not widely used.

Comment 4

[Andrew McCreight [:mccr8] (PTO 9/16-9/23)]

• WebAuthnManager::mParent needs to be cycle collected, as mentioned above. This is sufficient to fix the leak in the test.

There are some other fields in these classes that need to be cycle collected as far as I can tell:

• U2F::mTransaction needs to be cycle collected, because it is a wrapper around a strong pointer to a DOM callback, which is cycle collected. I added some new overloads for the cycle collector helpers.
• WebAuthnManager::mTransaction needs to be cycle collected. This is similar to U2F::mTransaction.
• WebAuthnManager::mFollowingSignal is inherited from AbortFollower and needs to be cycle collected.

Comment 5

[Andrew McCreight [:mccr8] (PTO 9/16-9/23)]

Attachment: Bug 1517611 - Cycle collect WebAuthnManager and U2F more.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4145884732ca
Cycle collect WebAuthnManager and U2F more. r=smaug

Comment 7

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/4145884732ca

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Feature isn't widely used and we're late in the cycle. Let's just let this ride the 66 train to release.