1523013 - Clear SamplesWaitingForKey::mProxy in Shutdown()

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1523013 - Clear SamplesWaitingForKey::mProxy in Shutdown().

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9452ba2d9657
Clear SamplesWaitingForKey::mProxy in Shutdown(). r=cpearce

Comment 3

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9452ba2d9657

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Andrew McCreight [:mccr8]]

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

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

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

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

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1079d285dd40