Added out-of-process support for server sockets.

Added out-of-process support for server sockets.

BUG=108277
TEST=OutOfProcessPPAPITest.TCPServerSocketPrivate


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

[brettw, 2012-03-12 06:27:21]

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:28: IDToServerSocketMap*
g_id_to_uninitialized_server_socket = NULL;
You shouldn't need this uninitialized socket ID map. I think all we need to do
is send the HostResource (just call host_resource() on your object) to the
browser and have it echo that back instead of this ID. Then when you want to get
back to the object, just do
EnterProxyFromHostResource<PPB_TCPServerSocket_API> enter;
if (enter.succeeded())
  static_cast<TCPServerSocket*>(enter.object())->OnAcceptCompleted();
when you get the message back from the browser (the static cast is OK here since
you're the one implementing this API so you know if the enter succeeded for that
API, the object is yours).

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:91:
::ppapi::TrackedCallback::ClearAndRun(&accept_callback_,
You shouldn't need the ::ppapi:: anywhere in this file.

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:177: int32_t status) {
I think you only need g_id_to_initialized_server_socket map between here and
OnMsgAcceptACK, so I think you can make it a member of the proxy object instead
of a global.

The only tricky bit is to ensure that the object is removed from the map when
it's destroyed. I think you can do 
static_cast<PPB_TCPServerSocket_Private_Proxy*>(GetInterfaceProxy(API_ID_PPB_TCPSOCKET_PRIVATE))->ObjectDestroyed(id);

Although the static_cast is a bit annoying, I think this is cleaner than having
the global.

[ygorshenin1, 2012-03-12 12:59:37]

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:28: IDToServerSocketMap*
g_id_to_uninitialized_server_socket = NULL;
In the PROXY case, host_resource().host_resource() is always zero (because
HostResource is created by HostResource::MakeInstanceOnly, so the resource part
of HostResource instance is zero). Therefore, enter will always fail. Maybe
instead of passing host_resource() to browser (and receiving it back as echo) we
should pass pp_resource()? And then use thunk::EnterResource?

On 2012/03/12 06:27:21, brettw wrote:
> You shouldn't need this uninitialized socket ID map. I think all we need to do
> is send the HostResource (just call host_resource() on your object) to the
> browser and have it echo that back instead of this ID. Then when you want to
get
> back to the object, just do
> EnterProxyFromHostResource<PPB_TCPServerSocket_API> enter;
> if (enter.succeeded())
>   static_cast<TCPServerSocket*>(enter.object())->OnAcceptCompleted();
> when you get the message back from the browser (the static cast is OK here
since
> you're the one implementing this API so you know if the enter succeeded for
that
> API, the object is yours).

[brettw, 2012-03-12 15:22:49]

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:28: IDToServerSocketMap*
g_id_to_uninitialized_server_socket = NULL;
Oh, sorry. You should just do plain EnterResource<> wirth the resource ID from
that host resource.

[ygorshenin1, 2012-03-13 11:33:17]

Thanks, PTAL.

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:28: IDToServerSocketMap*
g_id_to_uninitialized_server_socket = NULL;
On 2012/03/12 15:22:49, brettw wrote:
> Oh, sorry. You should just do plain EnterResource<> wirth the resource ID from
> that host resource.

Done.

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:91:
::ppapi::TrackedCallback::ClearAndRun(&accept_callback_,
On 2012/03/12 06:27:21, brettw wrote:
> You shouldn't need the ::ppapi:: anywhere in this file.

Done.

http://codereview.chromium.org/9669038/diff/20/ppapi/proxy/ppb_tcp_server_soc...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:177: int32_t status) {
On 2012/03/12 06:27:21, brettw wrote:
> I think you only need g_id_to_initialized_server_socket map between here and
> OnMsgAcceptACK, so I think you can make it a member of the proxy object
instead
> of a global.
> 
> The only tricky bit is to ensure that the object is removed from the map when
> it's destroyed. I think you can do 
>
static_cast<PPB_TCPServerSocket_Private_Proxy*>(GetInterfaceProxy(API_ID_PPB_TCPSOCKET_PRIVATE))->ObjectDestroyed(id);
> 
> Although the static_cast is a bit annoying, I think this is cleaner than
having
> the global.

Done.

[brettw, 2012-03-13 16:27:39]

lgtm

[yzshen1, 2012-03-13 17:46:32]

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:637: this, routing_id,
plugin_dispatcher_id, socket_resource, real_socket_id);
Please change the name of real_socket_id. There isn't 'temp' ID anymore so
'real' is not necessary.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.h:155: void
OnTCPServerAccept(int32 tcp_client_sockets_routing_id,
nit: sockets->socket

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.h:156: uint32
real_socket_id);
I'd change this to, say, server_socket_id to be more clear. Moreover, there
isn't temp ID anymore, so 'real' is not necessary.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_tcp_server_socket.h (right):

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_tcp_server_socket.h:54: PP_Resource
socket_resource_;
Please consider commenting that this is used as an opaque ID at the browser
side. We don't hold a ref to the resource and it may become invalid at the
renderer/plugin side.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_tcp_server_socket.h:55: uint32
real_socket_id_;
Please change it to socket_id.

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1470:
uninitialized_tcp_server_sockets_.Lookup(socket_resource);
Can we also use EnterResource and remove uninitialized_tcp_server_sockets_?

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1498: }
Please consider sending back a message to destroy the accepted tcp socket (if
created) when the server socket cannot be found.

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.h:300: virtual void
TCPServerSocketAccept(uint32 real_socket_id) OVERRIDE;
Please remove real. (There are several places in this file.)

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:351: uint32 /* real_socket_id */,
Please remove 'real' (several places in this file.)

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:137: PluginDispatcher*
dispatcher = PluginDispatcher::GetForInstance(instance);
Wrong indent.

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:176: new
PpapiHostMsg_PPBTCPServerSocket_Destroy(real_socket_id);
Only send this message when real_socket_id is valid && status == PP_OK.

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:194: }
Please consider sending back a message to destroy the accepted tcp socket (if
created) when the server socket cannot be found.

http://codereview.chromium.org/9669038/diff/7032/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h (right):

http://codereview.chromium.org/9669038/diff/7032/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:67: uint32
real_socket_id_;
Please remove 'real'.

[ygorshenin1, 2012-03-14 11:36:18]

PTAL

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:637: this, routing_id,
plugin_dispatcher_id, socket_resource, real_socket_id);
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please change the name of real_socket_id. There isn't 'temp' ID anymore so
> 'real' is not necessary.

Done.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.h:155: void
OnTCPServerAccept(int32 tcp_client_sockets_routing_id,
On 2012/03/13 17:46:32, yzshen1 wrote:
> nit: sockets->socket

Done.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.h:156: uint32
real_socket_id);
On 2012/03/13 17:46:32, yzshen1 wrote:
> I'd change this to, say, server_socket_id to be more clear. Moreover, there
> isn't temp ID anymore, so 'real' is not necessary.

Done.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_tcp_server_socket.h (right):

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_tcp_server_socket.h:54: PP_Resource
socket_resource_;
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please consider commenting that this is used as an opaque ID at the browser
> side. We don't hold a ref to the resource and it may become invalid at the
> renderer/plugin side.

Done.

http://codereview.chromium.org/9669038/diff/7032/content/browser/renderer_hos...
content/browser/renderer_host/pepper_tcp_server_socket.h:55: uint32
real_socket_id_;
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please change it to socket_id.

Done.

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1470:
uninitialized_tcp_server_sockets_.Lookup(socket_resource);
Sure, thanks!

On 2012/03/13 17:46:32, yzshen1 wrote:
> Can we also use EnterResource and remove uninitialized_tcp_server_sockets_?

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1498: }
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please consider sending back a message to destroy the accepted tcp socket (if
> created) when the server socket cannot be found.

Done.

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
File content/renderer/pepper/pepper_plugin_delegate_impl.h (right):

http://codereview.chromium.org/9669038/diff/7032/content/renderer/pepper/pepp...
content/renderer/pepper/pepper_plugin_delegate_impl.h:300: virtual void
TCPServerSocketAccept(uint32 real_socket_id) OVERRIDE;
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please remove real. (There are several places in this file.)

Done.

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppapi_messages.h...
ppapi/proxy/ppapi_messages.h:351: uint32 /* real_socket_id */,
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please remove 'real' (several places in this file.)

Done.

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:137: PluginDispatcher*
dispatcher = PluginDispatcher::GetForInstance(instance);
On 2012/03/13 17:46:32, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:176: new
PpapiHostMsg_PPBTCPServerSocket_Destroy(real_socket_id);
On 2012/03/13 17:46:32, yzshen1 wrote:
> Only send this message when real_socket_id is valid && status == PP_OK.

Done.

http://codereview.chromium.org/9669038/diff/7032/ppapi/proxy/ppb_tcp_server_s...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:194: }
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please consider sending back a message to destroy the accepted tcp socket (if
> created) when the server socket cannot be found.

Done.

http://codereview.chromium.org/9669038/diff/7032/ppapi/shared_impl/private/pp...
File ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h (right):

http://codereview.chromium.org/9669038/diff/7032/ppapi/shared_impl/private/pp...
ppapi/shared_impl/private/ppb_tcp_server_socket_shared.h:67: uint32
real_socket_id_;
On 2012/03/13 17:46:32, yzshen1 wrote:
> Please remove 'real'.

Done.

[yzshen1, 2012-03-14 17:05:04]

lgtm

http://codereview.chromium.org/9669038/diff/24002/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9669038/diff/24002/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1492: render_view_->Send(
nit: only send Disconnect when accepted_socket_id != 0.

http://codereview.chromium.org/9669038/diff/24002/ppapi/proxy/ppb_tcp_server_...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/24002/ppapi/proxy/ppb_tcp_server_...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:196:
PluginGlobals::Get()->plugin_proxy_delegate()->SendToBrowser(
Send Disconnect only if accepted_socket_id is valid, please.

[ygorshenin1, 2012-03-15 07:08:03]

謝謝！

http://codereview.chromium.org/9669038/diff/24002/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/9669038/diff/24002/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:1492: render_view_->Send(
On 2012/03/14 17:05:04, yzshen1 wrote:
> nit: only send Disconnect when accepted_socket_id != 0.

Done.

http://codereview.chromium.org/9669038/diff/24002/ppapi/proxy/ppb_tcp_server_...
File ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc (right):

http://codereview.chromium.org/9669038/diff/24002/ppapi/proxy/ppb_tcp_server_...
ppapi/proxy/ppb_tcp_server_socket_private_proxy.cc:196:
PluginGlobals::Get()->plugin_proxy_delegate()->SendToBrowser(
On 2012/03/14 17:05:04, yzshen1 wrote:
> Send Disconnect only if accepted_socket_id is valid, please.

Done.

[commit-bot: I haz the power, 2012-03-15 07:09:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/9669038/20038

[commit-bot: I haz the power, 2012-03-15 09:39:59]

Change committed as 126873