Fix the leak of the wrapper created by WrapperClass_Allocate().

Fix the leak of the wrapper created by WrapperClass_Allocate().

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277962

[Lei Zhang, 2014-06-14 01:22:23]

ping

[brettw, 2014-06-16 19:36:50]

https://codereview.chromium.org/324033002/diff/1/content/renderer/pepper/plug...
File content/renderer/pepper/plugin_object.cc (right):

https://codereview.chromium.org/324033002/diff/1/content/renderer/pepper/plug...
content/renderer/pepper/plugin_object.cc:50: delete np_object;
Sorry I don't have in my head how this is supposed to work, but this probably
means we need some more comments here. I don't understand why we need to delete
the np_object when FromNPObject fails, and not when it succeeds. And actually, I
don't understand why it might fail in the first place. Does this happen in
practice?

[Lei Zhang, 2014-06-16 20:37:30]

https://codereview.chromium.org/324033002/diff/1/content/renderer/pepper/plug...
File content/renderer/pepper/plugin_object.cc (right):

https://codereview.chromium.org/324033002/diff/1/content/renderer/pepper/plug...
content/renderer/pepper/plugin_object.cc:50: delete np_object;
On 2014/06/16 19:36:50, brettw wrote:
> Sorry I don't have in my head how this is supposed to work, but this probably
> means we need some more comments here. I don't understand why we need to
delete
> the np_object when FromNPObject fails, and not when it succeeds. And actually,
I
> don't understand why it might fail in the first place. Does this happen in
> practice?

I'm not deleting it when FromNPObject() fails. I'm always deleting it.

WrapperClass_Allocate() above calls PluginObject::AllocateObjectWrapper() which
allocates a NPObject*, but I don't see any place that's deleting it. It comes up
when I run browser_tests under LSAN locally, and it reports the object returned
by WrapperClass_Allocate() as being leaked.

[brettw, 2014-06-17 20:18:21]

LGTM. This is either a great find, or it's wrong. We'll find out which.

[Lei Zhang, 2014-06-17 20:39:27]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-06-17 20:40:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/324033002/1

[commit-bot: I haz the power, 2014-06-18 07:10:33]

Change committed as 277962