Closed Bug 1142272 Opened 5 years ago Closed 4 years ago

[gmp] Leak of |platformAPI| in GMPChild::RecvStartPlugin

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: ltronghieu1989, Mentored)

Details

(Whiteboard: [MemShrink:P3][CID 1286606][lang=c++][good first bug])

Attachments

(3 files)

Whiteboard: [MemShrink][CID 1286606] → [MemShrink:P3][CID 1286606]
Mentor: erahm
Whiteboard: [MemShrink:P3][CID 1286606] → [MemShrink:P3][CID 1286606][lang=c++][good first bug]
Greetings,

I'd like to take this bug. Can you assign it to me? Thank you :)
Great! Please let me know if you need any guidance. This is also a good resource for new contributors: https://developer.mozilla.org/en-US/docs/Introduction
Assignee: nobody → ltronghieu1989
I have coded the fix. How can I put out a shelveset for your review?
Step 4 in the link that Eric Rahm posted talks about how to attach a patch.

(This version of the link will attempt to select a language based on your locale or something if you would prefer a language besides English to read documentation: https://developer.mozilla.org/docs/Introduction )
Specifically, this section of the wiki has good details on generating a patch: https://developer.mozilla.org/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Attached patch hieu.patchSplinter Review
the platformAPI is initialized as a "struct" with the all the members as "int" (GErr is a enum type). So to release the struct memory, I simply call the "delete" on platformAPI. 

P.S. To be safe, we can do "platformAPI = NULL" so that the system will know and discard it at the end of RecvStartPlugin().
Flags: needinfo?(erahm)
Attachment #8607909 - Flags: feedback?
Comment on attachment 8607909 [details] [diff] [review]
hieu.patch

Review of attachment 8607909 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a very minor ('nit') request for cleaning up the indentation.

::: dom/media/gmp/GMPChild.cpp
@@ +402,5 @@
>  
>    mGMPLoader = GMPProcessChild::GetGMPLoader();
>    if (!mGMPLoader) {
>      NS_WARNING("Failed to get GMPLoader");
> +	 delete platformAPI;

nit: our coding style uses 2 spaces ('  ') rather than tabs for indenting. Can you fix the indentation for these 3 changes?
Attachment #8607909 - Flags: feedback? → feedback+
Attached patch hieu.patchSplinter Review
Eric,
Please review this again. I have modified the indentations of the three lines.
Attachment #8608534 - Flags: feedback?
Comment on attachment 8608534 [details] [diff] [review]
hieu.patch

Review of attachment 8608534 [details] [diff] [review]:
-----------------------------------------------------------------

Just small misunderstanding. Can you please clean up the indentation again?

::: dom/media/gmp/GMPChild.cpp
@@ +402,5 @@
>  
>    mGMPLoader = GMPProcessChild::GetGMPLoader();
>    if (!mGMPLoader) {
>      NS_WARNING("Failed to get GMPLoader");
> +  delete platformAPI;

I'm sorry I wasn't more explicit! This should just match the indentation of |NS_WARNING|. So that would be 4 spaces total (but no tabs!). Same for the rest.
Attachment #8608534 - Flags: feedback? → feedback+
Flags: needinfo?(erahm)
Attached patch hieu.patchSplinter Review
Hi Eric, I have fixed the indentation. Please take another look at it.
Flags: needinfo?(erahm)
Attachment #8612125 - Flags: feedback?
Comment on attachment 8612125 [details] [diff] [review]
hieu.patch

Review of attachment 8612125 [details] [diff] [review]:
-----------------------------------------------------------------

Close enough, lets get this landed.
Attachment #8612125 - Flags: review+
Flags: needinfo?(erahm)
Attachment #8612125 - Flags: feedback?
Using an nsAutoPtr here instead of a manual delete would have been good, as then future code changes are less likely to regress this, as the GMPPlatform* will automatically be deleted.
(In reply to Chris Pearce (:cpearce) from comment #13)
> Using an nsAutoPtr here instead of a manual delete would have been good, as
> then future code changes are less likely to regress this, as the
> GMPPlatform* will automatically be deleted.

It's a little trickier than that as you'd have to transfer ownership in the call to |mGMPLoader->Load| [1], but still handle the failure case.

[1] https://hg.mozilla.org/mozilla-central/annotate/9eae3880b132/dom/media/gmp/GMPChild.cpp#l416
Sure, that's what nsAutoPtr::forget() is for.
https://hg.mozilla.org/mozilla-central/rev/135fb7986b60
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.