WebSocket Pepper API: SRPC proxy implementation

WebSocket Pepper API: SRPC proxy implementation

Implement PPB_WebSocket _Dev SRPC proxy
- support PP_Var write back in plugin/browser callback bridge
- resource leak fix in browser_callback
 
Enable all WebSocket related ppapi tests in NaCl
- support TEST_PPAPI_NACL_VIA_HTTP_WITH_WS macro in ppapi_uitest
- fix a test which expects synchronous completion

BUG=87310
TEST=ui_tests --gtest_filter='PPAPINaClTest.WebSocket_*'

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

[Takashi Toyoshima, 2012-01-16 16:59:22]

Hi,

I finally finish SRPC proxy implementation.
Could you review this CL?
I'm sorry but it's a little large.
> 25 files changed, 1743 insertions(+), 22 deletions(-)

Thanks

[dmichael (off chromium), 2012-01-17 23:32:45]

http://codereview.chromium.org/9227008/diff/4008/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9227008/diff/4008/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:139: RunTestViaHTTP(test_case);
I'm fine with this way, but couldn't you also just always run it via HTTP?

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

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc:21: using
pp::Var;
Do you need this? I don't see where you're using it. If you can avoid using the
C++ wrappers, I'd prefer that. (Admittedly we already use them in the NaCl proxy
even though we shouldn't).

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

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:27:
// inputsless
inputsless -> inputs?

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:35:
DebugPrintf("PPB_WebSocket::Create: resource=%"NACL_PRIu32"\n", *resource);
Should that be NACL_PRId32?

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:51:
DebugPrintf("PPB_WebSocket::IsWebSocket: pp_success=%d\n", pp_success);
nit: It's probably safest to do static_cast<int>(pp_success)

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:53:
*success = (pp_success == PP_TRUE);
We usually use PP_ToBool() and PP_FromBool for these conversions.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_callback.cc (right):

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_callback.cc:111: if (user_var
!= NULL && read_buffer_size > 0)
nit: else if, just for clarity

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc (right):

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:60:
nacl_abi_size_t url_size = kMaxReturnVarSize;
nit: kMaxReturnVarSize is meant for out-params; url_size is ignored when passed
to Serialize. I'd rather you initialize it to 0 for clarity. (same elsewhere)

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:134:
&pp_error);
Should you have a faster path, as PPB_URLLoader does, for when there's data
immediately available? There's already overhead with the way the NaCl proxy
implements URLLoader :-(.

But written this way, it seems you're *always* sending an callback separately,
which will mean:
1) an extra RPC call, and
2) Re-entry in to the plugin code. That might work fine, but it's likely to
cause some hard-to-debug problems.

I'd rather you did it like PPB_URLLoader and pass back the var immediately as an
SRPC out-parameter if the browser returns it immediately. The downside is that
it will limit your frame size to just less than 64k. Is that okay?


[Not for this CL:  Also note that when you do the Chrome IPC proxy, it will be
significantly different. You'll want to model after the URLLoader proxy there,
and I think that one pushes data from the Browser to the Plugin.]

[Takashi Toyoshima, 2012-01-18 11:07:16]

http://codereview.chromium.org/9227008/diff/4008/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9227008/diff/4008/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:139: RunTestViaHTTP(test_case);
Yes, I can. Is it make things simple?
Done.

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

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_callback.cc:21: using
pp::Var;
Sorry. It was needed, but is not used now.
I delete this line.

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

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:27:
// inputsless
Done.
PPB_URLLoader_Create looks having the same nits.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:35:
DebugPrintf("PPB_WebSocket::Create: resource=%"NACL_PRIu32"\n", *resource);
I see. PP_Resource is defined as typedef int32_t PP_Resource;.
As you said, I must use d32 instead of u32.
I fixed this line, but many implements in ppapi_proxy use the same idiom.
I'll prepare another CL to fix nits in them.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:51:
DebugPrintf("PPB_WebSocket::IsWebSocket: pp_success=%d\n", pp_success);
I found a good sample which use *success after PP_ToBool().
I'll follow this style.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:53:
*success = (pp_success == PP_TRUE);
Oh, sorry.
I remember you taught me this before.
Done.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_callback.cc (right):

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_callback.cc:111: if (user_var
!= NULL && read_buffer_size > 0)
On 2012/01/17 23:32:45, dmichael wrote:
> nit: else if, just for clarity

Done.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc (right):

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:60:
nacl_abi_size_t url_size = kMaxReturnVarSize;
On 2012/01/17 23:32:45, dmichael wrote:
> nit: kMaxReturnVarSize is meant for out-params; url_size is ignored when
passed
> to Serialize. I'd rather you initialize it to 0 for clarity. (same elsewhere)

Done.

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:134:
&pp_error);
This first implementation doesn't care about performance optimization, then
apply straightforward way.
How about making another CL to improve ReceiveMessage performance?

Actually, WebSocket frame size could be int64 max in the spec, so 64k is not
enough to handle it.
Simple hack is to adapt optimization for <64k frames as you mentioned.
And the best way is adding another path in order to handle receiving frames as
soon as possible.

To tell the truth, I couldn't perfectly understand the details of out of process
proxy mechanism yet.
I feel it's more difficult than SRPC and will require some structural change on
thunk API to support it.
That's why I apply the simplest way to SRPC for now.
I'd like to apply optimization after out of process proxy implementation because
it require perfect understanding over all proxy mechanisms.

[dmichael (off chromium), 2012-01-18 17:04:40]

lgtm

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

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:35:
DebugPrintf("PPB_WebSocket::Create: resource=%"NACL_PRIu32"\n", *resource);
On 2012/01/18 11:07:16, toyoshim wrote:
> I see. PP_Resource is defined as typedef int32_t PP_Resource;.
> As you said, I must use d32 instead of u32.
> I fixed this line, but many implements in ppapi_proxy use the same idiom.
> I'll prepare another CL to fix nits in them.
Thanks!

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc (right):

http://codereview.chromium.org/9227008/diff/4008/ppapi/native_client/src/shar...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_websocket.cc:134:
&pp_error);
On 2012/01/18 11:07:16, toyoshim wrote:
> This first implementation doesn't care about performance optimization, then
> apply straightforward way.
> How about making another CL to improve ReceiveMessage performance?
> 
> Actually, WebSocket frame size could be int64 max in the spec, so 64k is not
> enough to handle it.
> Simple hack is to adapt optimization for <64k frames as you mentioned.
> And the best way is adding another path in order to handle receiving frames as
> soon as possible.
> 
> To tell the truth, I couldn't perfectly understand the details of out of
process
> proxy mechanism yet.
> I feel it's more difficult than SRPC and will require some structural change
on
> thunk API to support it.
> That's why I apply the simplest way to SRPC for now.
> I'd like to apply optimization after out of process proxy implementation
because
> it require perfect understanding over all proxy mechanisms.
For what it's worth, I'm not *only* concerned with performance here. I was
afraid you were doing a re-entrant SRPC call back from the browser to the
plugin, but I guess not. Since you are not making the callback "optional", I
think your response will fire asynchronously, so there's not a problem of
re-entrancy (though the overhead is fairly big; the response gets posted as a
task to run later, which then does the extra SRPC call back to the plugin).


As for the out-of-process proxy... it is a bit more complicated. But it's easier
to make that proxy more efficient, too. In any case, the SRPC proxy is
definitely the priority, since that's what will allow NaCl apps to use your
stuff.

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:52:
DebugPrintf("PPB_WebSocket::IsWebSocket: success=%d\n", *success);
Is it safe to use %d for bool? Generally, sizeof(int) == 4, while sizeof(bool)
== 1.

how about using:
"...success=%s\n", *success ? "true" : "false");

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:144:
MakeRemoteCompletionCallback(rpc->channel, callback_id, &callback_var);
Maybe add a comment somewhere nearby to note that the callback will invoke
asynchronously?
(It actually looks like the URLLoader proxy does this too, so its "fast" path is
probably never executed :-/ )

[dmichael (off chromium), 2012-01-18 17:07:47]

lgtm

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:52:
DebugPrintf("PPB_WebSocket::IsWebSocket: success=%d\n", *success);
On 2012/01/18 17:04:40, dmichael wrote:
> Is it safe to use %d for bool? Generally, sizeof(int) == 4, while sizeof(bool)
> == 1.
> 
> how about using:
> "...success=%s\n", *success ? "true" : "false");
Aaah, nevermind, it's an int32_t, not a bool. You should use NACL_PRId32 for
that.

[Takashi Toyoshima, 2012-01-19 04:18:57]

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
File
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc
(right):

http://codereview.chromium.org/9227008/diff/13001/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_websocket_rpc_server.cc:144:
MakeRemoteCompletionCallback(rpc->channel, callback_id, &callback_var);
On 2012/01/18 17:04:40, dmichael wrote:
> Maybe add a comment somewhere nearby to note that the callback will invoke
> asynchronously?
> (It actually looks like the URLLoader proxy does this too, so its "fast" path
is
> probably never executed :-/ )

Done.

[commit-bot: I haz the power, 2012-01-19 04:19:27]

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

[commit-bot: I haz the power, 2012-01-19 04:19:37]

Presubmit check for 9227008-13002 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 3071, 82
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 3073, 87
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 3075, 81
chars \
  ppapi/native_client/src/shared/ppapi_proxy/ppb_rpc_server.cc, line 3076, 85
chars

Presubmit checks took 2.3s to calculate.

[Takashi Toyoshima, 2012-01-19 04:21:46]

Could not use CQ for style violation by auto generated file.
I use try -b linux_clang,mac_rel,win_rel,linux_rel, then use dcommit to land.

[Takashi Toyoshima, 2012-01-19 06:05:14]

FileIO_Open, FileIO_TouchQuery, and FileRef constantly fail.
I'll check my callback related change.

[Takashi Toyoshima, 2012-01-19 07:07:27]

Oh, sorry.
Miss to pass check_result_func and get_size_read_func through in browser
callback.
I'll try -b linux_clang,mac_rel,win_rel,linux_rel again, then use dcommit to
land.

[Takashi Toyoshima, 2012-01-19 08:37:17]

win_rel's failure is unrelated flakiness.
And latest rebase looks no problem.
I land it now.