WebSocket Pepper API: synchronous completion support

WebSocket Pepper API: synchronous completion support.

Allow optional remote callback, then perform synchronous completion
if receiving data is enough small for SRPC buffer. Otherwise invoke
another callback for asynchronous completion from main thread.

BUG=87310
TEST=browser_tests --gtest_filter=PPAPINaclTest.WebSocket_StressedSendReceive

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

[Takashi Toyoshima, 2012-03-28 12:05:47]

Hi David,

I'm trying to support synchronous completion in ReceiveMessage.
If received data is less than 64k in serialized format, it looks easy like this
implementation. But, I could not find out how can I handle larger data than 64k.
To my understanding, only way to pass over 64k data from browser to plugin is to
use CompletionCallback like asynchronous completion, right? But, I could not
find out the way to schedule callback from here. If I use
CompletionCallbackRpcClient::RunCompletionCallback directly, the user callback
looks also invoked before API call returns PP_OK_COMPLETIONPENDING.

Can you give me advice on this?

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:168:
// TODO(toyoshim): Schedule to invoke callback_id here.
I could not find out how can I schedule remote_callback to be invoked later.

[dmichael (off chromium), 2012-04-02 03:28:01]

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:152:
// about re-entrancy.
This comment's not valid anymore, right?

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:168:
// TODO(toyoshim): Schedule to invoke callback_id here.
On 2012/03/28 12:05:47, toyoshim wrote:
> I could not find out how can I schedule remote_callback to be invoked later.
Could you maybe use PPB_Core.CallOnMainThread? It might be tricky to make sure
it won't misbehave if the WebSocket gets deleted before that call. You can
actually use CompletionCallbackFactory here, though, so maybe that would work.

Alternatively...  I think URLLoader has the same 64k limitation, right? Is it
perhaps acceptable to just make that limitation for now? The  Chrome IPC proxy
won't have that limitation, so it would be temporary. Just a thought.

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc (right):

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:159:
DebugPrintf("PPB_WebSocket::ReceiveMessage: PP_Var doesn't match\n");
These two checks are just temporary while you debug, right? Could they be
DCHECKs once you have it working as you intend?

[Takashi Toyoshima, 2012-04-05 06:44:00]

Thank you for great advice.
Now, it works!!

Could you review this for landing again?

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:152:
// about re-entrancy.
Right. I remove this comment.

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:168:
// TODO(toyoshim): Schedule to invoke callback_id here.
Thanks!
If I can use CallIOnMainThread here, it looks not so difficult.
I believe I can reuse remote_callback for CallOnMainThread... and looks working
fine to me:)

For test purpose, I removed SerializeTo() and called CallOnMainThread(0,
remote_callback, PP_OK) every time. It works fine. So I'd like to use it in case
of SerializeTo() failures.

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc (right):

http://codereview.chromium.org/9802027/diff/1/ppapi/native_client/src/shared/...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:159:
DebugPrintf("PPB_WebSocket::ReceiveMessage: PP_Var doesn't match\n");
Oops... yes that's right.
I should use DCHECK here.

[dmichael (off chromium), 2012-04-06 17:12:12]

Looks good overall, thanks! If possible, it would be great to add a test where
the resource gets released while the read call is in-flight to make sure the
behavior is right (for buffers <64k and >64k).

http://codereview.chromium.org/9802027/diff/8008/ppapi/native_client/src/shar...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9802027/diff/8008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:167:
// Buffer for synchronous completion is not enough. Uses asynchronous
nit: "not enough" -> "not big enough"

http://codereview.chromium.org/9802027/diff/8008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:171:
PPBCoreInterface()->CallOnMainThread(0, remote_callback, PP_OK);
Have you made sure this will work okay if the WebSocket resource is destroyed
while this callback is in flight? That's the only part that worries me. We don't
want a crash (especially not in the renderer); it should fail gracefully.

I commented in the test; it would be good to check for this. I think it will
work fine, but a test would help verify it.

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

http://codereview.chromium.org/9802027/diff/8008/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:660: std::vector<uint8_t> large_binary(128 *
1024);
Win XP has been giving me fits when testing large amounts of data. You might
want to scale back to ~65k; just enough to make sure you get the code path you
want. Otherwise it's more likely to be flaky on the Win XP bots :-(

http://codereview.chromium.org/9802027/diff/8008/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:673: result =
websocket_interface_->SendMessage(ws, large_binary_var);
Would it be possible to also add a test where you send a message and then
release the resource? You should get a PP_ERROR_ABORTED sent to your callback.

[Takashi Toyoshima, 2012-04-11 11:28:45]

Hi David.
Sorry for late update.

I added a test which contains abort tests for SendMessage and other APIs which
have callback.

BTW, I use std::auto_ptr in this test because I cannot use
base/memory/scoped_ptr in NaCl. Is it OK?

http://codereview.chromium.org/9802027/diff/8008/ppapi/native_client/src/shar...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9802027/diff/8008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:167:
// Buffer for synchronous completion is not enough. Uses asynchronous
On 2012/04/06 17:12:12, dmichael wrote:
> nit: "not enough" -> "not big enough"

Done.

http://codereview.chromium.org/9802027/diff/8008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:171:
PPBCoreInterface()->CallOnMainThread(0, remote_callback, PP_OK);
Agreed.
I added new test for this.

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

http://codereview.chromium.org/9802027/diff/8008/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:660: std::vector<uint8_t> large_binary(128 *
1024);
Oh, I see.
65k looks big enough. I change this size from 128k to 65k.

http://codereview.chromium.org/9802027/diff/8008/ppapi/tests/test_websocket.c...
ppapi/tests/test_websocket.cc:673: result =
websocket_interface_->SendMessage(ws, large_binary_var);
On 2012/04/06 17:12:12, dmichael wrote:
> Would it be possible to also add a test where you send a message and then
> release the resource? You should get a PP_ERROR_ABORTED sent to your callback.

Done.

[dmichael (off chromium), 2012-04-13 18:48:21]

lgtm

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

http://codereview.chromium.org/9802027/diff/17005/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:773: // Firstly, make sure the behavior for
SendMessage().
nit: make sure->test?

http://codereview.chromium.org/9802027/diff/17005/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:787: // Make sure the behavior for Connect().
Make sure -> Test?
(globally)

[Takashi Toyoshima, 2012-04-16 14:09:16]

thanks!

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

http://codereview.chromium.org/9802027/diff/17005/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:773: // Firstly, make sure the behavior for
SendMessage().
On 2012/04/13 18:48:21, dmichael wrote:
> nit: make sure->test?

Done.

http://codereview.chromium.org/9802027/diff/17005/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:787: // Make sure the behavior for Connect().
On 2012/04/13 18:48:21, dmichael wrote:
> Make sure -> Test?
> (globally)

Done.

[commit-bot: I haz the power, 2012-04-16 14:13:58]

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

[commit-bot: I haz the power, 2012-04-16 14:14:05]

Presubmit check for 9802027-29001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 3301, 83
chars

Presubmit checks took 1.7s to calculate.

[Takashi Toyoshima, 2012-04-16 14:15:21]

oops.
auto-generated code breaks presubmit check.
I'll use dcommit.

[commit-bot: I haz the power, 2012-04-16 16:43:02]

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

[commit-bot: I haz the power, 2012-04-16 16:43:08]

Presubmit check for 9802027-27013 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 3301, 83
chars