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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Eric Rahm [:erahm]]

In GMPChild::RecvStartPlugin |platformAPI| [1] is leaked [2] if |mGMPLoader| is null.

[1] https://hg.mozilla.org/mozilla-central/annotate/6686aacf006f/dom/media/gmp/GMPChild.cpp#l407
[2] https://hg.mozilla.org/mozilla-central/annotate/6686aacf006f/dom/media/gmp/GMPChild.cpp#l413

Comment 1

[Hieu Le]

Greetings,

I'd like to take this bug. Can you assign it to me? Thank you :)

Comment 2

[Eric Rahm [:erahm]]

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

Comment 3

[Hieu Le]

I have coded the fix. How can I put out a shelveset for your review?

Comment 4

[Andrew McCreight [:mccr8]]

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 )

Comment 5

[Eric Rahm [:erahm]]

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

Comment 6

[Hieu Le]

Attachment: hieu.patch

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().

Comment 7

[Eric Rahm [:erahm]]

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?

Comment 8

[Hieu Le]

Attachment: hieu.patch

Eric,
Please review this again. I have modified the indentations of the three lines.

Comment 9

[Eric Rahm [:erahm]]

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.

Comment 10

[Hieu Le]

Attachment: hieu.patch

Hi Eric, I have fixed the indentation. Please take another look at it.

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 8612125 [details] [diff] [review]
hieu.patch

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

Close enough, lets get this landed.

Comment 12

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/135fb7986b60

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 14

[Eric Rahm [:erahm]]

(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

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sure, that's what nsAutoPtr::forget() is for.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/135fb7986b60