Leaks in /webauthn/ WPTs
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Keywords: memory-leak, Whiteboard: [MemShrink:P1] [webauthn])
Attachments
(1 file)
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.
Updated•2 years ago
|
Assignee | ||
Comment 1•2 years ago
|
||
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.
Assignee | ||
Comment 2•2 years ago
|
||
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.
Assignee | ||
Updated•2 years ago
|
Comment 3•2 years ago
|
||
Yes, it shipped in Firefox 60, but so far is not widely used.
Assignee | ||
Comment 4•2 years ago
|
||
- 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.
Updated•2 years ago
|
Assignee | ||
Comment 5•2 years ago
|
||
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4145884732ca Cycle collect WebAuthnManager and U2F more. r=smaug
Comment 7•2 years ago
|
||
bugherder |
Comment 8•2 years ago
|
||
Feature isn't widely used and we're late in the cycle. Let's just let this ride the 66 train to release.
Description
•