Closed Bug 1523013 Opened 1 year ago Closed 1 year ago

Clear SamplesWaitingForKey::mProxy in Shutdown()

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak)

Attachments

(1 file)

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.

Priority: -- → P2

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.

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9452ba2d9657
Clear SamplesWaitingForKey::mProxy in Shutdown(). r=cpearce
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: needinfo?(continuation)

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.