Closed Bug 1517611 Opened 9 months ago Closed 8 months ago

Leaks in /webauthn/ WPTs

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

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.
Whiteboard: [MemShrink] → [MemShrink:P2]

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: nobody → continuation

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.

Whiteboard: [MemShrink:P2] → [MemShrink:P1]

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

Whiteboard: [MemShrink:P1] → [MemShrink:P1] [webauthn]
  • 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.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4145884732ca
Cycle collect WebAuthnManager and U2F more. r=smaug
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

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

You need to log in before you can comment on or make changes to this bug.