Clear SamplesWaitingForKey::mProxy in Shutdown()

RESOLVED FIXED in Firefox 66

Status

()

defect
P2
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug, {memory-leak})

unspecified
mozilla67
Points:
---

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox65 wontfix, firefox66 fixed, firefox67 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 months ago

SamplesWaitingForKey has a strong reference to a CDMProxy, via the mProxy field. CDMProxy has a strong reference to SamplesWaitingForKey, via the mWaitForKeys field. Under normal circumstances, eventually the wait comes to an end in CDMCaps::SetKeyStatus() and the cycle is broken. However, as seen in bug 1517595, in some cases the cycle is never broken, causing a leak.

One fix for this that seems to work is to clear the mProxy field when the two classes with mSamplesWaitingForKey fields clear that field. Only doing it for EMEDecryptor is needed to fix the leaks, but hopefully it makes sense to also do it for EMEMediaDataDecoderProxy.

Updated

3 months ago
Priority: -- → P2
(Assignee)

Comment 1

3 months ago

There's a strong cycle of references between SamplesWaitingForKey and
CDMProxy that does not get cleared unless keys actually come in. This
causes a leak. One way to avoid that leak is to clear the proxy
reference when the things holding the SamplesWaitingForKey is shut
down.

Comment 2

3 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9452ba2d9657
Clear SamplesWaitingForKey::mProxy in Shutdown(). r=cpearce

Comment 3

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Is this worth backport consideration or can it ride the trains?

Flags: needinfo?(continuation)
(Assignee)

Comment 5

3 months ago

Comment on attachment 9039870 [details]
Bug 1523013 - Clear SamplesWaitingForKey::mProxy in Shutdown().

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Leaks in some error cases. There's no evidence this actually happens to users.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

It just clears out some pointers in some error cases. If there were problems, they'd likely show up as null derefs, but I haven't seen any.

String changes made/needed

none

Flags: needinfo?(continuation)
Attachment #9039870 - Flags: approval-mozilla-beta?

Comment on attachment 9039870 [details]
Bug 1523013 - Clear SamplesWaitingForKey::mProxy in Shutdown().

Fix for possible memory leaks. Let's uplift for beta 6.

Attachment #9039870 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.