WebSocket Pepper API: fix data corruption at ReceiveMessage in NaCl

WebSocket Pepper API: fix data corruption at ReceiveMessage in NaCl

PpbWebSocketRpcServer::PPB_WebSocket_ReceiveMessage sometime gets unexpected
synchronous PP_OK result on PPBWebSocketInterface calling. Receiving data is
passed only in completion callbacks. Then, it causes data corruption.

This CL fix this issue and in order to reproduce this synchronous PP_OK situation,
add one stress test, StressedSendReceive.

This CL also fix CcInterface test flakiness on Mac.

BUG=111636
TEST=browser_tests --test_filter='PPAPI*Test.WebSocket_CcInterfaces'; browser_tests --test_filter='PPAPI*Test.WebSocket_StressedSendReceive'

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

[Takashi Toyoshima, 2012-03-20 23:27:58]

Hi David, could you review this CL?

At first, I think I'll use PP_RunCompletionCallback in PpbWebSocketRpcServer.
But, I'm not sure calling it asynchronously from here. Patch Set 2 is the CL by
this approach and looks working fine locally.

Next, I try to create non-optional callback in MakeRemoteCompletionCallback for
callers requiring buffer copy, or PP_Var serialization. These kinds of callbacks
must not be optional, and its task must complete asynchronously in successful
cases. So I think this is better and safe approach.

[dmichael (off chromium), 2012-03-21 19:16:46]

http://codereview.chromium.org/9724007/diff/8001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc (right):

http://codereview.chromium.org/9724007/diff/8001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc:143:
RunRemoteCallback, remote_callback.release());
This is going to affect URLLoader::ReadResponseBody, which is designed to
correctly handle an optional callback.

I think it would be better to either:
(a) Make your code also handle the optional case (this would be the best
performing approach, though possibly complicated. You can see the way URLLoader
does it for an example)
(b) Clear the "optional" flag after you get the result from
MakeRemoteCompletionCallback (with appropriate documentation there). This is
probably the simplest way to go, but you won't get as good of performance.

[dmichael (off chromium), 2012-03-21 19:16:47]

http://codereview.chromium.org/9724007/diff/8001/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc (right):

http://codereview.chromium.org/9724007/diff/8001/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc:143:
RunRemoteCallback, remote_callback.release());
This is going to affect URLLoader::ReadResponseBody, which is designed to
correctly handle an optional callback.

I think it would be better to either:
(a) Make your code also handle the optional case (this would be the best
performing approach, though possibly complicated. You can see the way URLLoader
does it for an example)
(b) Clear the "optional" flag after you get the result from
MakeRemoteCompletionCallback (with appropriate documentation there). This is
probably the simplest way to go, but you won't get as good of performance.

[Takashi Toyoshima, 2012-03-21 19:57:33]

Thanks.
Sorry, I didn't notice URLLoader case.
That's a nice example I must follow.

So first, I'd land a CL of plan (b), because this fix must be merged to branches
as soon as possible. Then, I'll work on (a) as soon as possible.

[Takashi Toyoshima, 2012-03-21 21:05:07]

I uploaded CL of plan (b) as Patch Set 5.

[dmichael (off chromium), 2012-03-23 17:38:14]

lgtm

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

http://codereview.chromium.org/9724007/diff/16001/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:656: // Send amounts of messages.
Did you mean something like: "Send many messages." ?

http://codereview.chromium.org/9724007/diff/16001/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:666: // Receive echoed binary.
binary -> data?

[Takashi Toyoshima, 2012-03-26 08:41:51]

Thanks!

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

http://codereview.chromium.org/9724007/diff/16001/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:656: // Send amounts of messages.
On 2012/03/23 17:38:14, dmichael wrote:
> Did you mean something like: "Send many messages." ?

Done.

http://codereview.chromium.org/9724007/diff/16001/ppapi/tests/test_websocket....
ppapi/tests/test_websocket.cc:666: // Receive echoed binary.
On 2012/03/23 17:38:14, dmichael wrote:
> binary -> data?

Done.

[commit-bot: I haz the power, 2012-03-26 08:42:22]

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

[commit-bot: I haz the power, 2012-03-26 11:13:31]

Change committed as 128890