Pepper WebSocket API: Fix memory leak issue

Pepper WebSocket API: Fix memory leak issue
 - Browser callback should release PP_Var resource after serialization.
 - Fix pp::Var to release injected refcounted object.
 - Fix example to release resources before reusing it.

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

[Takashi Toyoshima, 2012-09-07 17:18:26]

I fixed memory leak issues.
Could you review this change?

The leak of browser_callback is obvious.
But, the leak inside NativeClient looks difficult to solve properly.

These leaks happen in ppapi tests, too. But valgrind could not catch them.
Is it difficult to detect PP_Var leaks in unit tests?

http://codereview.chromium.org/10905128/diff/7001/native_client_sdk/src/examp...
File native_client_sdk/src/examples/websocket/websocket.cc (right):

http://codereview.chromium.org/10905128/diff/7001/native_client_sdk/src/examp...
native_client_sdk/src/examples/websocket/websocket.cc:100: pp::Var
release(pp::PASS_REF, receive_var_.Detach());
If we want to reuse the same pp::Var to receive ref-counted objects, we should
release internal PP_Var before passing it again. But I'm not sure this code is
good example for developers.
Should we release existing internal resource at ppapi/cpp/websocket.cc?

http://codereview.chromium.org/10905128/diff/7001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

http://codereview.chromium.org/10905128/diff/7001/ppapi/cpp/var.cc#newcode58
ppapi/cpp/var.cc:58: 
ppapi/cpp/websocket.cc uses Var::pp_var() to get internal PP_Var, then pass it
to C interface with const_cast. This is not expected for Var, but it looks
difficult to take another approach without any performance loss. I think it
needs another completion callback.

http://codereview.chromium.org/10905128/diff/7001/ppapi/cpp/var.cc#newcode62
ppapi/cpp/var.cc:62: needs_release_ = true;
NeedsRefcounting() tell us if the PP_Var requires refcounting.
So needs_release_ should means that the PP_Var resource should be managed by
this class instance.
It means that constructor Var(DontManage, PP_Var) is the only case which set
needs_release_ as false.

http://codereview.chromium.org/10905128/diff/7001/ppapi/cpp/var.cc#newcode116
ppapi/cpp/var.cc:116: if (needs_release_ && NeedsRefcounting(var_) &&
has_interface<PPB_Var_1_0>())
PP_Var leaked when PP_Var was updated after construction.
WebSocket ReceiveMessage completion callback did so.

http://codereview.chromium.org/10905128/diff/7001/ppapi/native_client/src/sha...
File ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc (right):

http://codereview.chromium.org/10905128/diff/7001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc:92:
PPBVarInterface()->Release(remote_callback->read_var);
This is a renderer process side memory leak.

[dmichael (off chromium), 2012-09-07 18:36:58]

http://codereview.chromium.org/10905128/diff/7001/native_client_sdk/src/examp...
File native_client_sdk/src/examples/websocket/websocket.cc (right):

http://codereview.chromium.org/10905128/diff/7001/native_client_sdk/src/examp...
native_client_sdk/src/examples/websocket/websocket.cc:100: pp::Var
release(pp::PASS_REF, receive_var_.Detach());
What if we just make ReceiveMessage release the var immediately? If plugins are
forced to do this, they'll get it wrong. I.e., we can just make ReceiveMessage
set the out-param to Undefined:
*message = Var();


The "new, cool" way to do this is with a CompletionCallbackWithOutput. It
shouldn't change the C interface, but would make the C++ interface look
different. ReceiveMessage's signature would become:
int32_t ReceiveMessage(const CompletionCallbackWithOutput<pp::Var>& callback)
And calling it would look like this:
pp::CompletionCallbackWithOutput<pp::Var> callback(OnReceiveCompletionCallback);
websocket_->ReceiveMessage(callback);
(note that you don't have to provide a Var)

OnReceiveCompletionCallback would look like this:
void OnReceiveCompletionCallback(int32_t result, const pp::Var& message);

The callback takes care of allocating & passing the var. Unfortunately, this
didn't exist when you did Websocket. But we should be able to use it now,
without breaking the ABI. I suspect we'd want to support both forms in parallel
for a while, to avoid breaking source compatibility. What do you think?

http://codereview.chromium.org/10905128/diff/7001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

http://codereview.chromium.org/10905128/diff/7001/ppapi/cpp/var.cc#newcode62
ppapi/cpp/var.cc:62: needs_release_ = true;
On 2012/09/07 17:18:27, toyoshim wrote:
> NeedsRefcounting() tell us if the PP_Var requires refcounting.
Currently, it only tells you if the Var's *type* requires reference counting. So
you're changing its meaning...

I think this is a reasonable way to go, but for me, it reinforces that we need
to get rid of DontManage. Then, we wouldn't need to store any data beyond the
PP_Var, and we would always just use NeedsRefcounting() to decide whether to
release.

http://codereview.chromium.org/10905128/diff/7001/ppapi/native_client/src/sha...
File ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc (right):

http://codereview.chromium.org/10905128/diff/7001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc:92:
PPBVarInterface()->Release(remote_callback->read_var);
On 2012/09/07 17:18:27, toyoshim wrote:
> This is a renderer process side memory leak.
Nice catch!

I have automatic checking for the plugin side for all tests (see test_case.h and
test_case.cc). I'm surprised this isn't showing up in the tests? Maybe we just
aren't reusing a var there?

In any case, I definitely should extend it to check the renderer side...

[Takashi Toyoshima, 2012-09-10 13:21:27]

Thanks. I revised my CL as follows.

- Release reused Var::pp_var not in example side, but in cpp/websocket.cc
- rename needs_release_ to needs_management_, and change the variable meaning.

https://chromiumcodereview.appspot.com/10905128/diff/7001/native_client_sdk/s...
File native_client_sdk/src/examples/websocket/websocket.cc (right):

https://chromiumcodereview.appspot.com/10905128/diff/7001/native_client_sdk/s...
native_client_sdk/src/examples/websocket/websocket.cc:100: pp::Var
release(pp::PASS_REF, receive_var_.Detach());
OK. I'll do '*message = Var();' in ReceiveMessage internally.

Also, the CompletionCallbackWithOutput looks nice.
I want to introduce this new interface to C++ API.

But this CL might be merged to other branches.
So, I'll separate it into another CL and BUG.
https://code.google.com/p/chromium/issues/detail?id=147544

https://chromiumcodereview.appspot.com/10905128/diff/7001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://chromiumcodereview.appspot.com/10905128/diff/7001/ppapi/cpp/var.cc#ne...
ppapi/cpp/var.cc:62: needs_release_ = true;
I meant that we can change the meaning of |needs_release_|. I can keep the
original meaning of NeedsRefcounting() here.

Current implementation, |needs_release_| means that the instance should manage
the internal PP_Var resource, AND the PP_Var is refcounted object. But the
latter condition looks redundant because we can always know it via
NeedsRefcounting().

So my suggestion is to change the meaning of |needs_release_| to only the former
meaning that the PP_Var is passed via DontManage or not. Maybe it's good to
change its name from needs_release_ to needs_manage_.

So 'needs_manage_ && NeedsRefcounting(var_)' is a right condition to release the
internal PP_Var as shown in the revised destructor.

[dmichael (off chromium), 2012-09-10 16:21:13]

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.cc#newcode106
ppapi/cpp/var.cc:106: needs_management_ = other.needs_management_;
Shouldn't this always just be true, with the new meaning of needs_management_?
At present, "needs_release_" is not copied, so if you copy an Unmanaged Var, the
new copy still takes a reference and later needs to release it.

(You're getting leaks right now in the few tests that use DontManage vars, and I
think this is why.)

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.cc#newcode131
ppapi/cpp/var.cc:131: bool old_needs_management_ = needs_management_;
no trailing underscore for local variables

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.h#newcode286
ppapi/cpp/var.h:286: bool needs_management_;
Good idea on the rename. How about is_managed_? A little shorter, maybe a little
clearer?

[Takashi Toyoshima, 2012-09-11 08:38:15]

Thank you.
I fix the leak.

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.cc#newcode106
ppapi/cpp/var.cc:106: needs_management_ = other.needs_management_;
Oops, this was wrong.
I revert this line to be always true.

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.cc#newcode131
ppapi/cpp/var.cc:131: bool old_needs_management_ = needs_management_;
On 2012/09/10 16:21:14, dmichael wrote:
> no trailing underscore for local variables

Done.

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

http://codereview.chromium.org/10905128/diff/6005/ppapi/cpp/var.h#newcode286
ppapi/cpp/var.h:286: bool needs_management_;
Agreed.

[dmichael (off chromium), 2012-09-11 15:11:14]

lgtm

[commit-bot: I haz the power, 2012-09-11 16:14:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10905128/2011

[commit-bot: I haz the power, 2012-09-11 16:14:24]

Presubmit check for 10905128-2011 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
TODOs found in stable public PPAPI files:

***************
ppapi/cpp/var.h
***************

Presubmit checks took 4.0s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Takashi Toyoshima, 2012-09-11 16:20:56]

Oops.
That happened also in cl upload.
OK, I'll use dcommit tomorrow morning.