1522951 - ChromiumCDMProxy::mCrashHelper should be a strong reference

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

ChromiumCDMProxy::mCrashHelper has type GMPCrashHelper*. It gets assigned from the third argument to the ChromiumCDMProxy ctor, which is |new MediaKeysGMPCrashHelper(this)|. That means that a refcounted object gets created, but it is not immediately addrefed. If anything else addrefs and releases the object, then this pointer becomes a pointer to a dead object. So, this should become a strong pointer.

The purpose of this field seems to be to carry along the helper to the ChromiumCDMProxy::Init() method which assigns the raw pointer to a stack local strong reference, which finally addrefs the helper (the strong reference is then copied into a closure). However, if the Init method bails out earlier then we end up never addrefing or releasing the object, and it will end up leaking. I think the fix here is to move the assignment to the stack local to the top of the function, and make it into a move assignment. mCrashHelper is not used after the Init method, so we don't need to keep around a strong reference that might end up contributing to a leak.

I noticed this while investigating bug 1517595 but it doesn't seem to be contributing to that leak. This does affect that leak a little, because the GMPCrashHelper seems to never be addrefed, which means it does not show up in the XPCOM leak checker.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1522951 - Make ChromiumCDMProxy::mCrashHelper into a strong reference.

Refcounted objects should be put into refptrs when they are created.

This patch also moves the crash helper out of the object early in
Init, to maybe reduce the chance of a leak if it fails early.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d37ccbbcd36d
Get rid of ChromiumCDMProxy::mCrashHelper. r=jya

Comment 3

[Arthur Iakab [arthur_iakab]]

Backed out changeset d37ccbbcd36d (bug 1522951) for Android build bustages on MediaDrmCDMProxy.h CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/cc1c59ce9077f7dfc1be30282a09a6ba6d75b234

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=d37ccbbcd36df177e86dd3a6d7cdf5a355fa8245

Log failure https://treeherder.mozilla.org/logviewer.html#?job_id=225298077&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=225297972&repo=autoland

[:mccr8 can you please take a look?

Comment 4

[Andrew McCreight [:mccr8]]

Ah, looks like CDMProxy uses a different implementation on Android. I think I missed that because I was using SearchFox and that doesn't analyze Android code. MediaDrmCDMProxy.cpp doesn't mention GMPCrashHelper anywhere, so I guess I have to not have that argument at all on Android or something? That explains the original set up. Hopefully I can think of some way to fix this that isn't ugly.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1522951 - Get rid of ChromiumCDMProxy::mCrashHelper.

Refcounted objects should be put into refptrs when they are created.

This patch also moves the crash helper out of the object early in
Init, to maybe reduce the chance of a leak if it fails early.

This field is only used to pass in a value to the Init() method. It
can't be passed in directly because on Android there are two
implementations of CDMProxy, and MediaDrmCDMProxy doesn't take a crash
helper.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/945770882765
Make ChromiumCDMProxy::mCrashHelper into a strong reference. r=jya

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/945770882765

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 9039270 [details]
Bug 1522951 - Make ChromiumCDMProxy::mCrashHelper into a strong reference.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Leaks in some error cases. I haven't seen any reports from actual 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 changes how a pointer in passed around across just a few functions. The new code is a lot less questionable than the existing code.

String changes made/needed

none

Comment 10

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

Comment on attachment 9039270 [details]
Bug 1522951 - Make ChromiumCDMProxy::mCrashHelper into a strong reference.

Fix for potential memory leak, sound good to me.
Should land for beta 6.

[Triage Comment]

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/70ad33dab5d6