WebSocket Pepper API: error handling improvement

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 3fd708b11abab79a2937189ef59609dd835a8c45..a728f851d9d4a97589f6bc2a1ff463a2e4652d77 100644
--- a/webkit/plugins/ppapi/ppb_websocket_impl.cc
+++ b/webkit/plugins/ppapi/ppb_websocket_impl.cc
@@ -72,6 +72,7 @@ namespace ppapi {
 PPB_WebSocket_Impl::PPB_WebSocket_Impl(PP_Instance instance)
     : Resource(instance),
       state_(PP_WEBSOCKETREADYSTATE_INVALID_DEV),
+      error_was_received_(false),
       receive_callback_var_(NULL),
       wait_for_receive_(false),
       close_code_(0),
@@ -256,6 +257,11 @@ int32_t PPB_WebSocket_Impl::ReceiveMessage(PP_Var* message,
   if (!received_messages_.empty())
     return DoReceive();

[dmichael (off chromium), 2011/12/02 16:28:08]

I just realized there's a problem here; you're not handling the
'PP_COMPLETIONCALLBACK_FLAG_OPTIONAL' flag, if the callback has one. When you
fix this to return PP_ERROR_BLOCKS_MAIN_THREAD when appropriate, please also fix
this to handle the force-async case.

To do so, you'll want to update the thunk layer and use 'MayForceCallback' (you
can grep for it in ppapi/thunk for examples).

[Takashi Toyoshima, 2011/12/02 17:15:42]

Oh, I see.
I fix it.

 
+  // Returns PP_ERROR_FAILED after an error is received and received messages
+  // is exhausted.
+  if (error_was_received_)
+    return PP_ERROR_FAILED;
+
   // Or retain |message| as buffer to store and install |callback|.
   wait_for_receive_ = true;
   receive_callback_var_ = message;
@@ -360,6 +366,15 @@ void PPB_WebSocket_Impl::didConnect() {
 }
 
 void PPB_WebSocket_Impl::didReceiveMessage(const WebString& message) {
+  // Dispose packets after receiving an error.
+  if (error_was_received_)
+    return;
+
+  // Dispose packets in invalid state.
+  if (state_ != PP_WEBSOCKETREADYSTATE_OPEN_DEV &&
+      state_ != PP_WEBSOCKETREADYSTATE_CLOSING)

[dmichael (off chromium), 2011/12/01 17:09:34]

Shouldn't that be 'CLOSING_DEV'? In my local check-out, ppb_websocket_dev.h has
'PP_WEBSOCKETREADYSTATE_CLOSING_DEV'. 

[Takashi Toyoshima, 2011/12/02 05:51:28]

Yes!
Sorry, I missed compilation error?
Fixed.

+    return;
+
   // Append received data to queue.
   std::string string = message.utf8();
   PP_Var var = StringVar::StringToPPVar(
@@ -373,13 +388,36 @@ void PPB_WebSocket_Impl::didReceiveMessage(const WebString& message) {
 }
 
 void PPB_WebSocket_Impl::didReceiveBinaryData(const WebData& binaryData) {
-  DLOG(INFO) << "didReceiveBinaryData is not implemented yet.";
+  // Dispose packets after receiving an error.
+  if (error_was_received_)
+    return;
+
+  // Dispose packets in invalid state.
+  if (state_ != PP_WEBSOCKETREADYSTATE_OPEN_DEV &&
+      state_ != PP_WEBSOCKETREADYSTATE_CLOSING)

[dmichael (off chromium), 2011/12/01 17:09:34]

optional suggestion: You do this check a lot. It could be worth a quick function
'InValidStateToReceive' or something.

[Takashi Toyoshima, 2011/12/02 05:51:28]

Sounds nice.
Done.

+    return;
+
   // TODO(toyoshim): Support to receive binary data.
+  DLOG(INFO) << "didReceiveBinaryData is not implemented yet.";
 }
 
 void PPB_WebSocket_Impl::didReceiveMessageError() {
-  // TODO(toyoshim): Must implement.
-  DLOG(INFO) << "didReceiveMessageError is not implemented yet.";
+  if (state_ != PP_WEBSOCKETREADYSTATE_OPEN_DEV &&
+      state_ != PP_WEBSOCKETREADYSTATE_CLOSING_DEV)
+    return;
+
+  // Pepper binding has no event or callback mechanism for errors.

[dmichael (off chromium), 2011/12/01 17:09:34]

I'm not sure I understand your comment. The idea during the API review was that
you would use error codes from pp_errors.h and invoke the appropriate completion
callback. If you don't have the right error(s) defined there yet, you can define
new ones. You do appear to be calling the receive_callback_ below; what's
missing?

[Takashi Toyoshima, 2011/12/02 05:51:28]

didReceiveMessageError() callback was invoked from WebKit side implementation
when it detect errors on underlaying networks or WebSocket protocol stack itself
every time.
Also other didReceive* callbacks were invoked every time being independent from
calling pepper side APIs.
So didReceive* events could not be binded to some specific callback.

We just queue incoming frame messages and errors in order manner, then
pop a message from top of the queue when the pepper side APIs are called.

I try to revise this comment to be clearer.

+  // Just remember the error, and notice after reading the last message as
+  // returned error code.
+  error_was_received_ = true;
+
+  if (!wait_for_receive_)
+    return;
+
+  // |received_messages| is empty and receiving process is on going. Received
+  // messages after an error is just ignored. So we must abort here.
+  wait_for_receive_ = false;
+  PP_RunAndClearCompletionCallback(&receive_callback_, PP_ERROR_FAILED);

[dmichael (off chromium), 2011/12/01 17:09:34]

Are you guaranteed that you'll still get 'didClose' if we're in the process of
Closing the socket? Or should you also invoke the closing callback if it's
valid?

[Takashi Toyoshima, 2011/12/02 05:51:28]

all didReceiveMessageError() was followed by didClose().
This behavior is described in WebSocket API specification.
And in WebKit side, these did* callbacks is binded to JavaScript API's on*
events.

So, I can guarantee closing callback will be invoked in didClose().

 }
 
 void PPB_WebSocket_Impl::didUpdateBufferedAmount(
@@ -390,11 +428,10 @@ void PPB_WebSocket_Impl::didUpdateBufferedAmount(
 }
 
 void PPB_WebSocket_Impl::didStartClosingHandshake() {
-  // TODO(toyoshim): Must implement.
-  DLOG(INFO) << "didStartClosingHandshake is not implemented yet.";
+  state_ = PP_WEBSOCKETREADYSTATE_CLOSING_DEV;
 }
 
-void PPB_WebSocket_Impl::didClose(unsigned long buffered_amount,
+void PPB_WebSocket_Impl::didClose(unsigned long unhandled_buffered_amount,
                                   ClosingHandshakeCompletionStatus status,
                                   unsigned short code,
                                   const WebString& reason) {
@@ -404,26 +441,33 @@ void PPB_WebSocket_Impl::didClose(unsigned long buffered_amount,
   close_reason_ = new StringVar(
       PpapiGlobals::Get()->GetModuleForInstance(pp_instance()), reason_string);
 
-  // TODO(toyoshim): Set close_was_clean_.
+  // Set close_was_clean_.
+  bool was_clean =
+      state_ == PP_WEBSOCKETREADYSTATE_CLOSING_DEV &&
+      !unhandled_buffered_amount &&
+      status == WebSocketClient::ClosingHandshakeComplete;
+  close_was_clean_ = was_clean ? PP_TRUE : PP_FALSE;
+
+  // Update buffered_amount_.
+  buffered_amount_ = unhandled_buffered_amount;
 
   // Handle state transition and invoking callback.
   DCHECK_NE(PP_WEBSOCKETREADYSTATE_CLOSED_DEV, state_);
   PP_WebSocketReadyState_Dev state = state_;
   state_ = PP_WEBSOCKETREADYSTATE_CLOSED_DEV;
 
-  // Update buffered_amount_.
-  buffered_amount_ = buffered_amount;
-
   if (state == PP_WEBSOCKETREADYSTATE_CONNECTING_DEV)
     PP_RunAndClearCompletionCallback(&connect_callback_, PP_OK);
 
   if (state == PP_WEBSOCKETREADYSTATE_CLOSING_DEV)
     PP_RunAndClearCompletionCallback(&close_callback_, PP_OK);
+
+  // Disconnect.
+  if (websocket_.get())
+    websocket_->disconnect();
 }
 
 int32_t PPB_WebSocket_Impl::DoReceive() {
-  // TODO(toyoshim): Check state.
-
   if (!receive_callback_var_)
     return PP_OK;