Closed Bug 1522951 Opened 9 months ago Closed 9 months ago

ChromiumCDMProxy::mCrashHelper should be a strong reference

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, 1 obsolete file)

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.

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.

Priority: -- → P2
Attachment #9039270 - Attachment description: Bug 1522951 - ChromiumCDMProxy::mCrashHelper should be a strong reference. → Bug 1522951 - Get rid of ChromiumCDMProxy::mCrashHelper.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d37ccbbcd36d
Get rid of ChromiumCDMProxy::mCrashHelper. r=jya

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.

Flags: needinfo?(continuation)

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.

Attachment #9040594 - Attachment is obsolete: true
Attachment #9039270 - Attachment description: Bug 1522951 - Get rid of ChromiumCDMProxy::mCrashHelper. → Bug 1522951 - Make ChromiumCDMProxy::mCrashHelper into a strong reference.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/945770882765
Make ChromiumCDMProxy::mCrashHelper into a strong reference. r=jya
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: needinfo?(continuation)

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

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

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]

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