WebSocket Pepper API: error handling improvement

WebSocket Pepper API: error handling improvement

 - didReceiveMessageError() handling
 - didStartClosingHandshake() handling
 - didReceive* state_ checking
 - MayForceCallback handling

BUG=87310
TEST=ui_tests --gtest_filter='PPAPITest.WebSocket*'

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

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

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:254: return PP_ERROR_BADARGUMENT;
Whoops, shouldn't this function also check that the completion callback is not a
blocking one and return PP_ERROR_BLOCKS_MAIN_THREAD if it is?

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:375: state_ !=
PP_WEBSOCKETREADYSTATE_CLOSING)
Shouldn't that be 'CLOSING_DEV'? In my local check-out, ppb_websocket_dev.h has
'PP_WEBSOCKETREADYSTATE_CLOSING_DEV'.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:397: state_ !=
PP_WEBSOCKETREADYSTATE_CLOSING)
optional suggestion: You do this check a lot. It could be worth a quick function
'InValidStateToReceive' or something.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:409: // Pepper binding has no event
or callback mechanism for errors.
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?

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:420:
PP_RunAndClearCompletionCallback(&receive_callback_, PP_ERROR_FAILED);
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]

Thank you for review.
I uploaded the revised change.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:254: return PP_ERROR_BADARGUMENT;
Oh, right!
I fix in the next change.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:375: state_ !=
PP_WEBSOCKETREADYSTATE_CLOSING)
Yes!
Sorry, I missed compilation error?
Fixed.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:397: state_ !=
PP_WEBSOCKETREADYSTATE_CLOSING)
Sounds nice.
Done.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:409: // Pepper binding has no event
or callback mechanism for errors.
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.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:420:
PP_RunAndClearCompletionCallback(&receive_callback_, PP_ERROR_FAILED);
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().

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

lgtm

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:254: return PP_ERROR_BADARGUMENT;
On 2011/12/02 05:51:28, toyoshim wrote:
> Oh, right!
> I fix in the next change.
Okay. By the way, this one will be a little more subtle than the others. If
received_messages_ is non-empty, then you can still return DoReceive() (as you
have below). But if it's empty, that's when you need PP_ERROR_BLOCKS_MAIN_THREAD

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:258: return DoReceive();
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).

http://codereview.chromium.org/8772001/diff/2002/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8772001/diff/2002/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:71: return true;
suggestion/nit: could also have just been:
return state == PP_WEBSOCKETREADYSTATE_OPEN_DEV ||
       state == PP_WEBSOCKETREADYSTATE_CLOSING_DEV;

That seems more readable to me.

http://codereview.chromium.org/8772001/diff/2002/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:414: // But, if no messages is queued
and ReceiveMessage() is now on going.
nit: is -> are

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

Thank you for review.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:254: return PP_ERROR_BADARGUMENT;
OK, I verify callbacks just before install them to variables.

http://codereview.chromium.org/8772001/diff/2001/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:258: return DoReceive();
Oh, I see.
I fix it.

http://codereview.chromium.org/8772001/diff/2002/webkit/plugins/ppapi/ppb_web...
File webkit/plugins/ppapi/ppb_websocket_impl.cc (right):

http://codereview.chromium.org/8772001/diff/2002/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:71: return true;
Sounds good.
Fixed.

http://codereview.chromium.org/8772001/diff/2002/webkit/plugins/ppapi/ppb_web...
webkit/plugins/ppapi/ppb_websocket_impl.cc:414: // But, if no messages is queued
and ReceiveMessage() is now on going.
On 2011/12/02 16:28:09, dmichael wrote:
> nit: is -> are

Done.

[dmichael (off chromium), 2011-12-02 17:23:11]

lgtm