PPB_TCPSocket: add support for TCP server socket operations.

PPB_TCPSocket: add support for TCP server socket operations.

BUG=262601
TEST=new tests in test_tcp_socket.

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

[yzshen1, 2013-09-17 18:14:09]

Hi,

Would you please take a look? Thanks!

Bill: main reviewer.

Brett: OWNER review for content_browser.gypi

Justin: security review for ppapi_messages.h. I added messages for
bind/listen/accept. "tcp-listen" apps permission is required to allow listening.
This is enforced by the browser process.

Noel and David: FYI, but welcome to review.

[yzshen1, 2013-09-17 18:30:58]

There are two changes to the IDL after our last discussion. Please take a look.

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:90: [in] PP_NetAddress_Family family);
This is needed to create the underlying OS socket.
We cannot deduce it from the address passed into Bind() or Connect(), because
PP_TCPSOCKET_OPTION_ADDRESS_REUSE needs to be set before Bind().

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:145: * the socket to be closed.
According to POSIX:
"""If connect() fails, the state of the socket is unspecified. Conforming
applications should close the file descriptor and create a new socket before
attempting to reconnect."""

In v1.0, we always create a new OS socket when Connect() is called. So it is
okay to allow multiple connection attempts.

In v1.1, however, we cannot easily create a new OS socket for the second
connection attempt. Because Bind() and SetOption() may have been called,
creating a new OS socket will lose the bound address or some options that have
been set.

Therefore, I changed the behavior of v1.1 to close the socket if a connection
attempt failed. I also made sure that the behavior of v1.0 doesn't change after
this CL.

What do you think?

[noelallen1, 2013-09-17 18:59:43]

I'm not sure people expect the socket to get destroyed with the connection
fails.  While it's only legal to connect once, I believe in Linux you can get
away with reusing the socket to try and connect somewhere else so long as it
doesn't connect.

That being said, I'm not sure how much effort we should put in to supporting
what's arguably a broken use case.  If need be we could add the code to reset
socket options inside of NaCl IO.

LGTM

[bbudge, 2013-09-17 19:42:17]

Some comments on IDL before full review.

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:197:
host_->GetPpapiHost(), instance, params.pp_resource(), tcp_socket));
There's a lot of code in common here and above. Could you create a utility
function that takes the instance, version and family?

You could call that directly here. Above you would unpack the message and call
it.

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:234:
new PepperTCPSocketMessageFilter(host_, instance, version,
Why don't you call PepperTCPSocketMessageFilter::Create here?

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:11:
#include "net/socket/tcp_socket.h"
Could you forward declare net::TCPSocket?

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:84: * @param[in] faimly The address family type for
the socket.
s/faimly/family

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:90: [in] PP_NetAddress_Family family);
On 2013/09/17 18:30:58, yzshen1 wrote:
> This is needed to create the underlying OS socket.
> We cannot deduce it from the address passed into Bind() or Connect(), because
> PP_TCPSOCKET_OPTION_ADDRESS_REUSE needs to be set before Bind().

Is there any way that address family could be specified by SetOption?

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:145: * the socket to be closed.
On 2013/09/17 18:30:58, yzshen1 wrote:
> According to POSIX:
> """If connect() fails, the state of the socket is unspecified. Conforming
> applications should close the file descriptor and create a new socket before
> attempting to reconnect."""
> 
> In v1.0, we always create a new OS socket when Connect() is called. So it is
> okay to allow multiple connection attempts.
> 
> In v1.1, however, we cannot easily create a new OS socket for the second
> connection attempt. Because Bind() and SetOption() may have been called,
> creating a new OS socket will lose the bound address or some options that have
> been set.
> 
> Therefore, I changed the behavior of v1.1 to close the socket if a connection
> attempt failed. I also made sure that the behavior of v1.0 doesn't change
after
> this CL.
> 
> What do you think?

This seems OK to me. We can add this capability later on if we want (by saving
the address and options that have been set).

[noelallen1, 2013-09-18 04:08:50]

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:90: [in] PP_NetAddress_Family family);
On 2013/09/17 19:42:18, bbudge1 wrote:
> On 2013/09/17 18:30:58, yzshen1 wrote:
> > This is needed to create the underlying OS socket.
> > We cannot deduce it from the address passed into Bind() or Connect(),
because
> > PP_TCPSOCKET_OPTION_ADDRESS_REUSE needs to be set before Bind().
> 
> Is there any way that address family could be specified by SetOption?

tcp::Create(inst, family) = socket(family, SOCK_STREAM, 0)
So specifying the family at create makes sense from the POSIX side, where as
SetOption for picking family seams strange IMO.

[yzshen1, 2013-09-18 16:56:25]

Thanks, Bill and Noel!

> I'm not sure people expect the socket to get destroyed with the connection
> fails.  While it's only legal to connect once, I believe in Linux you can get
> away with reusing the socket to try and connect somewhere else so long as it
> doesn't connect.
Yeah, I agree that we probably can reconnect on most platforms.
But it seems safer to explicitly forbid it. And we could remove this restriction
later if we are sure it is safe or users really ask for it.

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc
(right):

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:197:
host_->GetPpapiHost(), instance, params.pp_resource(), tcp_socket));
On 2013/09/17 19:42:18, bbudge1 wrote:
> There's a lot of code in common here and above. Could you create a utility
> function that takes the instance, version and family?
> 
> You could call that directly here. Above you would unpack the message and call
> it.

Done.

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc:234:
new PepperTCPSocketMessageFilter(host_, instance, version,
Because this class has a static Create() method for new sockets (which could
fail and return NULL) and a constructor for accepted sockets(which take
different parameters than Create()).

On 2013/09/17 19:42:18, bbudge1 wrote:
> Why don't you call PepperTCPSocketMessageFilter::Create here?

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://codereview.chromium.org/24195004/diff/1/content/browser/renderer_host...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:11:
#include "net/socket/tcp_socket.h"
I tried but it wasn't allowed.
net::TCPSocket is a typedef, I guess that is a problem?

On 2013/09/17 19:42:18, bbudge1 wrote:
> Could you forward declare net::TCPSocket?

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:84: * @param[in] faimly The address family type for
the socket.
On 2013/09/17 19:42:18, bbudge1 wrote:
> s/faimly/family

Done.

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:90: [in] PP_NetAddress_Family family);
On 2013/09/18 04:08:51, noelallen1 wrote:
> On 2013/09/17 19:42:18, bbudge1 wrote:
> > On 2013/09/17 18:30:58, yzshen1 wrote:
> > > This is needed to create the underlying OS socket.
> > > We cannot deduce it from the address passed into Bind() or Connect(),
> because
> > > PP_TCPSOCKET_OPTION_ADDRESS_REUSE needs to be set before Bind().
> > 
> > Is there any way that address family could be specified by SetOption?
> 
> tcp::Create(inst, family) = socket(family, SOCK_STREAM, 0)
> So specifying the family at create makes sense from the POSIX side, where as
> SetOption for picking family seams strange IMO.

I agree with Noel here. It seems natural to have family in Create() because that
is a parameter needed for creating OS socket.
What do you think?

[bbudge, 2013-09-18 17:46:35]

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:90: [in] PP_NetAddress_Family family);
I was looking at the API without considering implementation. I see that Create
needs family and version when creating the platform socket, so I guess we have
to add the new Create function.

Just looking at the API, the family seems like just another option and it would
be cleaner to add options than to add new Create functions. I'm assuming
SetOption must be called after Create but before anything else. Some comments to
this effect are needed, I think.

Would it be difficult to defer creation of the socket until necessary, say in
Bind? The downside of this is that it postpones any error in creating the OS
socket but how likely is that?

[yzshen1, 2013-09-18 18:06:55]

Thanks Bill!

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/1/ppapi/api/ppb_tcp_socket.idl#...
ppapi/api/ppb_tcp_socket.idl:90: [in] PP_NetAddress_Family family);
On 2013/09/18 17:46:35, bbudge1 wrote:
> I was looking at the API without considering implementation. I see that Create
> needs family and version when creating the platform socket, so I guess we have
> to add the new Create function.
> 
> Just looking at the API, the family seems like just another option and it
would
> be cleaner to add options than to add new Create functions.
IMO, it seems unusual to most POSIX develop to consider family as an option.

> I'm assuming
> SetOption must be called after Create but before anything else. Some comments
to
> this effect are needed, I think.

We will need to have an extra step for every socket creation:

...
Create();
SetOption(FAMILY, IPv4);
Bind()/Connect();
...

> Would it be difficult to defer creation of the socket until necessary, say in
> Bind? The downside of this is that it postpones any error in creating the OS
> socket but how likely is that?

We will need to create socket at three different places:
- SetOption() for ADDRESS_REUSE (unless we want to cache it).
- Bind().
- Connect() (because Connect() can be called without Bind().

It complicates the logic IMO.

[yzshen1, 2013-09-18 20:45:06]

According to offline discussion, I have removed the 1.1 Create() function.

Please take another look. Thanks!

[bbudge, 2013-09-19 19:41:47]

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:45:
scoped_ptr<ppapi::host::ResourceHost> CreateNewTCPSocket(
Indent is off by 1.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:32:
// TCPServerSocketPrivateResource inherits TCPSocketResourceBase.
nit: s/inherits/inherit

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:201: if
(!pepper_socket_utils::CanUseSocketAPIs(external_plugin_, IsPrivateAPI(),
At this point, IsPrivateAPI() must be true.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:371: if
(!pepper_socket_utils::CanUseSocketAPIs(external_plugin_, IsPrivateAPI(),
IsPrivateAPI must be false.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:413: //
Make sure no further callbacks from |socket_| or |ssl_socket_|.
s/Make sure/Make sure we get

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:736: //
becuase in the private or v1.0 API, connect must be the first operation.
sp because

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:840:
factory_->CreateAcceptedTCPSocket(
Do we need to NULL check factory_?

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:178:
ppapi::host::PpapiHost* ppapi_host_;
These fields could be const. Then any thread could read them. The same
observation applies to PepperTCPSServerSocketMessageFilter.

Brett had a trick where he grouped fields into child structs to make it clearer
which thread could use which fields. See NaClIPCAdapter.

https://codereview.chromium.org/24195004/diff/10001/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/api/ppb_tcp_socket....
ppapi/api/ppb_tcp_socket.idl:109: * Connects the socket to the given address.
The socket must not be listening.
It's not clear to me when or if the socket must be bound.

https://codereview.chromium.org/24195004/diff/10001/ppapi/api/ppb_tcp_socket....
ppapi/api/ppb_tcp_socket.idl:224: * @param[out] accepted_tcp_socket Stores the
accepted TCP socket on success.
Is the returned socket bound and connected?

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1514:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_TCPSocket_CreatePrivate)
Do we still need this message?

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:97: return PP_ERROR_INPROGRESS;  // Can
only have one pending request.
Comment seems superfluous. Here and below.

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:335: if (!state_.IsConnected())
Since you have to do all of this checking again on the host side, why not just
type check the value and convert it and let the host return any other errors?

https://codereview.chromium.org/24195004/diff/10001/ppapi/shared_impl/ppb_tcp...
File ppapi/shared_impl/ppb_tcp_socket_shared.cc (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/shared_impl/ppb_tcp...
ppapi/shared_impl/ppb_tcp_socket_shared.cc:14: DCHECK(initial_state == INITIAL
|| initial_state == CONNECTED);
Maybe better to replace initial_state with state_?

[jschuh, 2013-09-19 21:07:30]

ipc security rubberstamp lgtm.

[yzshen1, 2013-09-19 21:12:15]

Thanks, Bill! :)

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.h:45:
scoped_ptr<ppapi::host::ResourceHost> CreateNewTCPSocket(
On 2013/09/19 19:41:47, bbudge1 wrote:
> Indent is off by 1.

Done.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_server_socket_message_filter.h:32:
// TCPServerSocketPrivateResource inherits TCPSocketResourceBase.
On 2013/09/19 19:41:47, bbudge1 wrote:
> nit: s/inherits/inherit

Done.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:201: if
(!pepper_socket_utils::CanUseSocketAPIs(external_plugin_, IsPrivateAPI(),
On 2013/09/19 19:41:47, bbudge1 wrote:
> At this point, IsPrivateAPI() must be true.

Done.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:371: if
(!pepper_socket_utils::CanUseSocketAPIs(external_plugin_, IsPrivateAPI(),
On 2013/09/19 19:41:47, bbudge1 wrote:
> IsPrivateAPI must be false.

Done.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:413: //
Make sure no further callbacks from |socket_| or |ssl_socket_|.
On 2013/09/19 19:41:47, bbudge1 wrote:
> s/Make sure/Make sure we get

Done.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:736: //
becuase in the private or v1.0 API, connect must be the first operation.
On 2013/09/19 19:41:47, bbudge1 wrote:
> sp because

Done.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:840:
factory_->CreateAcceptedTCPSocket(
It should be fine not to check NULL: only accepted sockets will get a NULL
factory_, but those sockets are already in CONNECTED state. Because only socket
in LISTENING state can get here, we should be safe.

I have added a comment.
On 2013/09/19 19:41:47, bbudge1 wrote:
> Do we need to NULL check factory_?

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:178:
ppapi::host::PpapiHost* ppapi_host_;
On 2013/09/19 19:41:47, bbudge1 wrote:
> These fields could be const. Then any thread could read them. The same
> observation applies to PepperTCPSServerSocketMessageFilter.
Those classes (PpapiHost and ....Factory) are not thread safe, so we should only
use them on the thread they are designed for.

> 
> Brett had a trick where he grouped fields into child structs to make it
clearer
> which thread could use which fields. See NaClIPCAdapter.

One concern is that it will make all the names longer. And it would change most
of the lines .cc file. What do you think?

https://codereview.chromium.org/24195004/diff/10001/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/api/ppb_tcp_socket....
ppapi/api/ppb_tcp_socket.idl:109: * Connects the socket to the given address.
The socket must not be listening.
On 2013/09/19 19:41:47, bbudge1 wrote:
> It's not clear to me when or if the socket must be bound.

Done.

https://codereview.chromium.org/24195004/diff/10001/ppapi/api/ppb_tcp_socket....
ppapi/api/ppb_tcp_socket.idl:224: * @param[out] accepted_tcp_socket Stores the
accepted TCP socket on success.
On 2013/09/19 19:41:47, bbudge1 wrote:
> Is the returned socket bound and connected?
Accepted socket is connected.

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1514:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_TCPSocket_CreatePrivate)
I still keep it because in content_browser_pepper_host_factory we group hosts by
public/private/dev.

If we share the same create message for public and private API, it may be less
consistent with other code.

What do you think?

On 2013/09/19 19:41:47, bbudge1 wrote:
> Do we still need this message?

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:97: return PP_ERROR_INPROGRESS;  // Can
only have one pending request.
On 2013/09/19 19:41:47, bbudge1 wrote:
> Comment seems superfluous. Here and below.

Done.

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:335: if (!state_.IsConnected())
It seems nice to fail early. I think this is consistent with other Pepper
resources?
What do you think?

On 2013/09/19 19:41:47, bbudge1 wrote:
> Since you have to do all of this checking again on the host side, why not just
> type check the value and convert it and let the host return any other errors?

https://codereview.chromium.org/24195004/diff/10001/ppapi/shared_impl/ppb_tcp...
File ppapi/shared_impl/ppb_tcp_socket_shared.cc (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/shared_impl/ppb_tcp...
ppapi/shared_impl/ppb_tcp_socket_shared.cc:14: DCHECK(initial_state == INITIAL
|| initial_state == CONNECTED);
Done. Thanks! I just realized that initial_state is too similar to INITIAL.

On 2013/09/19 19:41:47, bbudge1 wrote:
> Maybe better to replace initial_state with state_?

[bbudge, 2013-09-19 22:30:57]

Found a few more things.

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h
(right):

https://codereview.chromium.org/24195004/diff/10001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h:178:
ppapi::host::PpapiHost* ppapi_host_;
On 2013/09/19 21:12:15, yzshen1 wrote:
> On 2013/09/19 19:41:47, bbudge1 wrote:
> > These fields could be const. Then any thread could read them. The same
> > observation applies to PepperTCPSServerSocketMessageFilter.
> Those classes (PpapiHost and ....Factory) are not thread safe, so we should
only
> use them on the thread they are designed for.
> 
> > 
> > Brett had a trick where he grouped fields into child structs to make it
> clearer
> > which thread could use which fields. See NaClIPCAdapter.
> 
> One concern is that it will make all the names longer. And it would change
most
> of the lines .cc file. What do you think?

Good point. Ignore my comment.

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:1514:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_TCPSocket_CreatePrivate)
On 2013/09/19 21:12:15, yzshen1 wrote:
> I still keep it because in content_browser_pepper_host_factory we group hosts
by
> public/private/dev.
> 
> If we share the same create message for public and private API, it may be less
> consistent with other code.
> 
> What do you think?
> 
> On 2013/09/19 19:41:47, bbudge1 wrote:
> > Do we still need this message?
> 

OK, I understand why you're keeping it.

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
File ppapi/proxy/tcp_socket_resource_base.cc (right):

https://codereview.chromium.org/24195004/diff/10001/ppapi/proxy/tcp_socket_re...
ppapi/proxy/tcp_socket_resource_base.cc:335: if (!state_.IsConnected())
On 2013/09/19 21:12:15, yzshen1 wrote:
> It seems nice to fail early. I think this is consistent with other Pepper
> resources?
> What do you think?
> 
> On 2013/09/19 19:41:47, bbudge1 wrote:
> > Since you have to do all of this checking again on the host side, why not
just
> > type check the value and convert it and let the host return any other
errors?
> 

OK

https://codereview.chromium.org/24195004/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/24195004/diff/21001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:98:
DCHECK(host);
If this is for accept sockets, would a DCHECK for version_ be appropriate here?

https://codereview.chromium.org/24195004/diff/21001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:734: //
In order to maintain backward compatibility, allow to connect the socket
Wording. How about this?
// In order to maintain backward compatibility, allow the socket to be
// connected again.

https://codereview.chromium.org/24195004/diff/21001/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/21001/ppapi/api/ppb_tcp_socket....
ppapi/api/ppb_tcp_socket.idl:55: PP_TCPSOCKET_OPTION_ADDRESS_REUSE = 3
could we use the [version=1.1] annotation here? I see one in ppb_file_io.idl:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file...

https://codereview.chromium.org/24195004/diff/21001/ppapi/proxy/tcp_socket_pr...
File ppapi/proxy/tcp_socket_private_resource.cc (right):

https://codereview.chromium.org/24195004/diff/21001/ppapi/proxy/tcp_socket_pr...
ppapi/proxy/tcp_socket_private_resource.cc:31: CloseImpl();
Why not call this in the base class dtor?
also called in TCPSocketResource.

[yzshen1, 2013-09-19 23:14:41]

https://codereview.chromium.org/24195004/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc
(right):

https://codereview.chromium.org/24195004/diff/21001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:98:
DCHECK(host);
On 2013/09/19 22:30:58, bbudge1 wrote:
> If this is for accept sockets, would a DCHECK for version_ be appropriate
here?

Done.

https://codereview.chromium.org/24195004/diff/21001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc:734: //
In order to maintain backward compatibility, allow to connect the socket
Done. Thanks!
On 2013/09/19 22:30:58, bbudge1 wrote:
> Wording. How about this?
> // In order to maintain backward compatibility, allow the socket to be
> // connected again.

https://codereview.chromium.org/24195004/diff/21001/ppapi/api/ppb_tcp_socket.idl
File ppapi/api/ppb_tcp_socket.idl (right):

https://codereview.chromium.org/24195004/diff/21001/ppapi/api/ppb_tcp_socket....
ppapi/api/ppb_tcp_socket.idl:55: PP_TCPSOCKET_OPTION_ADDRESS_REUSE = 3
I tried but the generation result was wrong.
It will result in PP_TCPSocket_Option_1_0 which is not defined.

PP_FileOpenFlags works is because we never use it as an input parameter name in
the interfaces.

We need to fix the generator before adding this tag.


On 2013/09/19 22:30:58, bbudge1 wrote:
> could we use the [version=1.1] annotation here? I see one in ppb_file_io.idl:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_file...

https://codereview.chromium.org/24195004/diff/21001/ppapi/proxy/tcp_socket_pr...
File ppapi/proxy/tcp_socket_private_resource.cc (right):

https://codereview.chromium.org/24195004/diff/21001/ppapi/proxy/tcp_socket_pr...
ppapi/proxy/tcp_socket_private_resource.cc:31: CloseImpl();
On 2013/09/19 22:30:58, bbudge1 wrote:
> Why not call this in the base class dtor?
> also called in TCPSocketResource.

Done.

[bbudge, 2013-09-19 23:26:56]

LGTM

[brettw, 2013-09-20 04:52:16]

.gypi lgtm

[commit-bot: I haz the power, 2013-09-20 05:01:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/24195004/20036

[commit-bot: I haz the power, 2013-09-20 09:48:40]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-20 14:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/24195004/20036

[commit-bot: I haz the power, 2013-09-20 15:29:24]

Change committed as 224383