WebSocket Pepper API: allow to release in completion callbacks

webkit/plugins/ppapi/ppb_websocket_impl.cc

Index: webkit/plugins/ppapi/ppb_websocket_impl.cc
diff --git a/webkit/plugins/ppapi/ppb_websocket_impl.cc b/webkit/plugins/ppapi/ppb_websocket_impl.cc
index bc9fae483d460f51ce2309dd3e402afc97a5ea1f..6bdf0372570884533d3d97a10bd483762ac45874 100644
--- a/webkit/plugins/ppapi/ppb_websocket_impl.cc
+++ b/webkit/plugins/ppapi/ppb_websocket_impl.cc
@@ -502,17 +502,26 @@ void PPB_WebSocket_Impl::didClose(unsigned long unhandled_buffered_amount,
   PP_WebSocketReadyState state = state_;
   state_ = PP_WEBSOCKETREADYSTATE_CLOSED;
 
-  if (state == PP_WEBSOCKETREADYSTATE_CONNECTING)
+  const base::WeakPtr<PPB_WebSocket_Impl> weak_ptr = AsWeakPtr();

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

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]

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]

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.

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

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.

+  if (state == PP_WEBSOCKETREADYSTATE_CONNECTING) {
     TrackedCallback::ClearAndRun(&connect_callback_, PP_ERROR_FAILED);
+    if (!weak_ptr)
+      return;
+  }
 
   if (wait_for_receive_) {
     wait_for_receive_ = false;
     receive_callback_var_ = NULL;
     TrackedCallback::ClearAndRun(&receive_callback_, PP_ERROR_FAILED);
+    if (!weak_ptr)
+      return;
   }
 
-  if ((state == PP_WEBSOCKETREADYSTATE_CLOSING) && close_callback_.get())
+  if ((state == PP_WEBSOCKETREADYSTATE_CLOSING) && close_callback_.get()) {
     TrackedCallback::ClearAndRun(&close_callback_, PP_OK);
+    if (!weak_ptr)
+      return;
+  }
 
   // Disconnect.
   if (websocket_.get())