Private Pepper extension for Flapper to allow TCP connections to be made

Private Pepper extension for Flapper to allow TCP connections to be made
(from inside the renderer sandbox).

(Only enabled on ChromeOS, since it opens a hole in the renderer. This should be
revisited, with controls tightened, once Flapper runs out of process.)

BUG=none
TEST=none

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

[piman, 2010-11-17 21:41:22]

http://codereview.chromium.org/5098002/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/5098002/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/pepper_message_filter.cc:65:
IPC_MESSAGE_HANDLER(ViewHostMsg_PepperConnectTcp, OnPepperConnectTcp)
OnPepperConnectTcpAddress ?

http://codereview.chromium.org/5098002/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/pepper_message_filter.cc:176: void
OnLookupFinished(int /*result*/) {
Shouldn't we handle / forward failure here ? Or is it that implicitly no
addresses will be found, hence we'll just return an invalid socket below (in
which case it might be worth a comment - we will still be doing a couple of
unnecessary thread hops, but since it's a failure case maybe it's fine).

http://codereview.chromium.org/5098002/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/pepper_message_filter.h (right):

http://codereview.chromium.org/5098002/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/pepper_message_filter.h:43: int request_id,
nit: parameters should align with the above one.

http://codereview.chromium.org/5098002/diff/2001/chrome/renderer/pepper_plugi...
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/5098002/diff/2001/chrome/renderer/pepper_plugi...
chrome/renderer/pepper_plugin_delegate_impl.cc:821: PP_CompletionCallback
callback) {
maybe add a check for a NULL callback, which implies a blocking call (in other
interfaces). We don't have to implement it, but returning failure would be good
I think.
Same below for ConnectTcpAddress.

http://codereview.chromium.org/5098002/diff/2001/webkit/glue/plugins/ppb_priv...
File webkit/glue/plugins/ppb_private2.h (right):

http://codereview.chromium.org/5098002/diff/2001/webkit/glue/plugins/ppb_priv...
webkit/glue/plugins/ppb_private2.h:132: PP_CompletionCallback callback);
Something bugs me about this interface. If it's used using the common callback
factory pattern, then it's unsafe: even if the callback is cancelled, the output
parameters will still be written. So there's no way to safely cancel the request
if you need to delete the object that needs to receive the result.

[viettrungluu, 2011-01-06 17:40:54]

It's alive (again)!

I think I addressed all your comments, though attaching the connector to a
resource changed quite a few things. (The "backend" browser-side stuff remains
mostly the same.)

[viettrungluu, 2011-01-06 19:23:58]

+brettw, since piman is apparently not a morning person.

(For the checkdeps errors, I'm going to move ppb_flash, etc. to more sensible
locations.)

[piman, 2011-01-06 19:31:15]

One small thing, otherwise LGTM

http://codereview.chromium.org/5098002/diff/26001/webkit/plugins/ppapi/plugin...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/5098002/diff/26001/webkit/plugins/ppapi/plugin...
webkit/plugins/ppapi/plugin_module.cc:245: return
PPB_Flash_Impl::GetInterface();
I think it would make sense to only expose this interface if it is supported
(i.e. ENABLE_FLAPPER_HACKS is defined). That way we don't have mysterious
connection failures, but rather an early strict failure in the plugin if we
choose so.

[brettw, 2011-01-06 20:41:24]

If we're going to have a compile define, it seems like the message handler in
the browser that opens the hole should also use the same define. So when we
remove this hack, we won't accidentally leave a hold in the sandbox.

[viettrungluu, 2011-01-06 23:26:48]

On 2011/01/06 20:41:24, brettw wrote:
> If we're going to have a compile define, it seems like the message handler in
> the browser that opens the hole should also use the same define. So when we
> remove this hack, we won't accidentally leave a hold in the sandbox.

Okay, now the messages just aren't handled when ENABLE_FLAPPER_HACKS isn't
defined (and this allowed me to remove other stuff as well). (I could just not
have the PepperMessageFilter in that case, but I'm guessing we'll eventually
want it for other Pepper stuff anyway.)

I also did as Antoine suggested and only expose the interface when
ENABLE_FLAPPER_HACKS is defined.

Could you take another look to see if it looks okay? Hopefully it actually
builds....

[piman, 2011-01-06 23:35:16]

LGTM