WebSocket Pepper API: allow to release in completion callbacks

Allow to release WebSocket resource in completion callbacks invoked by Close().

 - Protect |this| around invoking plugin callbacks
 - Add unit tests to check the case

BUG=
TEST=browser_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144872

[Takashi Toyoshima, 2012-06-25 13:39:58]

Hi David,

This is a review request which is mentioned in my email.

Thanks.

[dmichael (off chromium), 2012-06-25 15:50:50]

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_ut...
File ppapi/tests/test_utils.h (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_ut...
ppapi/tests/test_utils.h:75: void SetDelegate(Delegate* delegate) { delegate_ =
delegate; }
Please add documentation for the method, including the fact that the "delegate"
will only be invoked once for each time SetDelegate is called. In other words,
it will be reset when "Reset" or "GetCallback" is called.

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
File ppapi/tests/test_websocket.cc (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
ppapi/tests/test_websocket.cc:582: // Release the resource before completion
callback invoked.
nit: before +the+ completion callback +is+ invoked

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
File ppapi/tests/test_websocket.h (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
ppapi/tests/test_websocket.h:19: public TestCompletionCallback::Delegate {
I think it would be cleaner to define a separate class to implement the
Delegate.

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
ppapi/tests/test_websocket.h:23: // TestCompletionCallback::Delegate interfaces.
interfaces->implementation

https://chromiumcodereview.appspot.com/10661026/diff/2007/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_websocket_impl.cc:505: const
base::WeakPtr<PPB_WebSocket_Impl> weak_ptr = AsWeakPtr();
Another option might be to just take a reference for the scope of this method,
to ensure the WebSocket resource isn't deleted until the end, at the latest. You
could do that with just:
scoped_refptr<PPB_WebSocket_Impl> retain_this(this);

Then you wouldn't need the SupportsWeakPtr stuff, and your logic might be
simpler.

[Takashi Toyoshima, 2012-06-26 09:19:10]

Thank you for looking this.
I have one question about a behavior as I replied in ppb_websocket_impl.cc.

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_ut...
File ppapi/tests/test_utils.h (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_ut...
ppapi/tests/test_utils.h:75: void SetDelegate(Delegate* delegate) { delegate_ =
delegate; }
On 2012/06/25 15:50:51, dmichael wrote:
> Please add documentation for the method, including the fact that the
"delegate"
> will only be invoked once for each time SetDelegate is called. In other words,
> it will be reset when "Reset" or "GetCallback" is called.

Done.

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
File ppapi/tests/test_websocket.cc (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
ppapi/tests/test_websocket.cc:582: // Release the resource before completion
callback invoked.
On 2012/06/25 15:50:51, dmichael wrote:
> nit: before +the+ completion callback +is+ invoked

Done.

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
File ppapi/tests/test_websocket.h (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
ppapi/tests/test_websocket.h:19: public TestCompletionCallback::Delegate {
OK, I will introduce class ReleaseResourceDelegate in test_websocket.cc.

https://chromiumcodereview.appspot.com/10661026/diff/2007/ppapi/tests/test_we...
ppapi/tests/test_websocket.h:23: // TestCompletionCallback::Delegate interfaces.
On 2012/06/25 15:50:51, dmichael wrote:
> interfaces->implementation

Done.

https://chromiumcodereview.appspot.com/10661026/diff/2007/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

https://chromiumcodereview.appspot.com/10661026/diff/2007/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_websocket_impl.cc:505: const
base::WeakPtr<PPB_WebSocket_Impl> weak_ptr = AsWeakPtr();
If I took a reference of this here, I can continue to run through this function.
At that time, it could be possible that it invokes another CompletionCallback
despite the fact that this object was already released in the previous
completion callback. I'm not sure this behavior is consistent to other PPAPIs.
If we should omit subsequent callbacks after destruction, WeakPtr provide a good
interface to check if the |this| was released.

If we should not omit them, I agreed your suggestion. To tell the truth, I
didn't notice that ppapi::Resource is already refcounted:-)

[dmichael (off chromium), 2012-06-26 16:01:40]

http://codereview.chromium.org/10661026/diff/2007/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/10661026/diff/2007/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:505: const
base::WeakPtr<PPB_WebSocket_Impl> weak_ptr = AsWeakPtr();
On 2012/06/26 09:19:10, toyoshim wrote:
> If I took a reference of this here, I can continue to run through this
function.
> At that time, it could be possible that it invokes another CompletionCallback
> despite the fact that this object was already released in the previous
> completion callback. I'm not sure this behavior is consistent to other PPAPIs.
> If we should omit subsequent callbacks after destruction, WeakPtr provide a
good
> interface to check if the |this| was released.
> 
> If we should not omit them, I agreed your suggestion. To tell the truth, I
> didn't notice that ppapi::Resource is already refcounted:-)
Are you sure? We keep 2 separate reference counts; the one available to the
plugin is distinct from the one we use internally to RefCounted. When the plugin
Releases the last [external] reference on the Resource, it should trigger
aborting all pending callbacks for the Resource. (See
ppapi/shared_impl/resource_tracker.cc:ReleaseResource and
LastPluginRefWasDeleted). Then, when you call ClearAndRun, TrackedCallback knows
that the callback has a pending abort, and will send PP_ERROR_ABORTED
immediately (and cancel the pending aborts). The behavior should look right to
the plugin, I think. Please let me know if it doesn't, because I'll want to fix
it.

http://codereview.chromium.org/10661026/diff/10/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/10661026/diff/10/ppapi/tests/test_websocket.cc...
ppapi/tests/test_websocket.cc:92: virtual void OnCallback(void* user_data,
int32_t result) {
nit: OVERRIDE

[Takashi Toyoshima, 2012-06-27 08:09:05]

Thanks.
Your advice is helpful to understand deeply.

http://codereview.chromium.org/10661026/diff/2007/webkit/plugins/ppapi/ppb_we...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/10661026/diff/2007/webkit/plugins/ppapi/ppb_we...
webkit/plugins/ppapi/ppb_websocket_impl.cc:505: const
base::WeakPtr<PPB_WebSocket_Impl> weak_ptr = AsWeakPtr();
Thank you.

I didn't understand TrackedCallback correctly. But, now I understand related
codes you pointed out and add a new unit test to ensure this understanding.

Yes, actually following callbacks are invoked with PP_ERROR_ABORTED immediately
if the user handler of previous callback releases the PP_Resource.

These callbacks will be invoked after PP_Resource was released. But this
behavior looks consistent in all APIs. So I agreed this is reasonable.

http://codereview.chromium.org/10661026/diff/10/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/10661026/diff/10/ppapi/tests/test_websocket.cc...
ppapi/tests/test_websocket.cc:92: virtual void OnCallback(void* user_data,
int32_t result) {
This code is for NaCl targeted compilers, so I think we couldn't use OVERRIDE
macro here.

http://codereview.chromium.org/10661026/diff/10/ppapi/tests/test_websocket.cc...
ppapi/tests/test_websocket.cc:596: // Release the resource before the completion
callback is invoked.
I notice that we already have this first case in TestAbortCall().
I'll remove this first case.

Also I move the second one into TestAbortCalls().

[dmichael (off chromium), 2012-06-27 19:52:17]

http://codereview.chromium.org/10661026/diff/15001/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/10661026/diff/15001/ppapi/tests/test_websocket...
ppapi/tests/test_websocket.cc:123: CallbackDelegate*
GetDelegateFor(WebSocketEvent::EventType type,
I don't think you need CallbackChecker (see below). But if you keep it, this
should be named something more like "CreateDelegateFor"

http://codereview.chromium.org/10661026/diff/15001/ppapi/tests/test_websocket...
ppapi/tests/test_websocket.cc:1024: ASSERT_EQ(PP_OK, close_events[0].result);
I'm having trouble understanding why you wanted the "checker"? You're already
seeing whether the callback was fired with the right value when you do
close_callback.WaitForResult(result);
ASSERT_EQ(PP_OK, close_callback.result());

It seems overly complicated to check for it two different ways. Same comment for
the receive_checker below. Can you get rid of the CallbackChecker and just have
a Delegate that releases the resource?

[Takashi Toyoshima, 2012-06-28 08:05:37]

Thanks.
I removed CallbackChecker. Also update the CL description.

http://codereview.chromium.org/10661026/diff/15001/ppapi/tests/test_websocket.cc
File ppapi/tests/test_websocket.cc (right):

http://codereview.chromium.org/10661026/diff/15001/ppapi/tests/test_websocket...
ppapi/tests/test_websocket.cc:1024: ASSERT_EQ(PP_OK, close_events[0].result);
Ah... you are right.
Firstly, I'd like to check when the second callback is invoked. That's the
original reason why I try to introduce this CallbackChecker mechanism.
Using this, I checked its behavior locally, but I stopped to apply the check
because I think it will introduce flakiness. As a result, only meaningless
redundant checks remains here.

I remove CallbackChecker.

[dmichael (off chromium), 2012-06-28 16:08:22]

lgtm

[commit-bot: I haz the power, 2012-06-29 03:37:16]

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

[commit-bot: I haz the power, 2012-06-29 04:41:43]

Change committed as 144872