PPAPI: Introduce PPB_NetworkProxy_Dev

PPAPI: Introduce PPB_NetworkProxy_Dev

This adds support for PPB_NetworkProxy_Dev for out-of-process and NaCl (when using --enable-nacl).

Work still to do in future CLs:
- Add permissions checking
- Translate appropriate error codes from net_errors.h

BUG=247225
R=jam@chromium.org, palmer@chromium.org, yzshen@chromium.org

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

[dmichael (off chromium), 2013-06-15 03:51:26]

Yuzhu: This is almost done. If you get bored over the weekend, feel free to
look. But don't sweat it if not; I want to do a little clean up and republish
next week.

I just thought you might be able to relax a little knowing it's very nearly
done.

[yzshen1, 2013-06-15 15:33:26]

On 2013/06/15 03:51:26, dmichael wrote:
> Yuzhu: This is almost done. If you get bored over the weekend, feel free to
> look. But don't sweat it if not; I want to do a little clean up and republish
> next week.
> 
> I just thought you might be able to relax a little knowing it's very nearly
> done.

This is great! Thanks, David. :)
I will read it before Monday.

Have a nice weekend!

[yzshen1, 2013-06-16 23:49:53]

Thanks David!
It mostly looks good, just a few nits.

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:31:
RenderProcessHost::FromID(render_process_id);
2 more spaces of indent, please.

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.h (right):

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_NETWORK_HOST_HOST_H_
HOST_HOST -> PROXY_HOST, please.

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
File ppapi/cpp/dev/network_proxy_dev.cc (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
ppapi/cpp/dev/network_proxy_dev.cc:35: if (url_var.is_null())
It seems unnecessary to check |url_var| here: the implementation of the C
interface should have already checked the input arguments.

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
File ppapi/cpp/dev/network_proxy_dev.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
ppapi/cpp/dev/network_proxy_dev.h:27: const std::string& url,
- I think once Brett suggested me to have the C++ interface as a "thin" wrapper
around the C interface, not the convert the types too much. Maybe we should use
pp::Var for |url| here? Besides, it is more consistent with the output
parameter. What do you think?
(I think we should also look at how the other C++ interfaces deal with this
problem, and do it in a consistent way.)

- WRT cancelling the callback, I think that is fine, it will get cancelled when
the instance goes away.

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
File ppapi/proxy/network_proxy_resource.cc (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
ppapi/proxy/network_proxy_resource.cc:40: this,
nit, optional: It is safe and more clear to use base::Unretained(this). Because
this callback is stored in the base class of NetworkProxyResource. (Although it
also works to use |this| because we explicitly clear callbacks when all plugin
references go away.)

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
File ppapi/proxy/network_proxy_resource.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
ppapi/proxy/network_proxy_resource.h:25: virtual thunk::PPB_NetworkProxy_API*
AsPPB_NetworkProxy_API() OVERRIDE;
nit: both of these two virtual methods are public in the base classes. It seems
better to still make them public, because when we call them with base class
pointers, them are public anyway.

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

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1434: // HostResolverPrivate
---------------------------------------------------------
nit, optional: Maybe put the style&order fix in a separate CL will be easier to
review for the message file OWNER. And moving messages around may cause
unnecessary conflicts because we are both changing them in a rush.
(But it is not a big issue. I am fine if you would like to make the changes in
this CL.)

https://codereview.chromium.org/16819002/diff/9001/ppapi/tests/test_network_p...
File ppapi/tests/test_network_proxy.cc (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/tests/test_network_p...
ppapi/tests/test_network_proxy.cc:9: #include "ppapi/cpp/module.h"
This include is not needed.

https://codereview.chromium.org/16819002/diff/9001/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_public_dev.h:31: PROXIED_IFACE(PPB_Instance,
PPB_NETWORKPROXY_DEV_INTERFACE_0_1,
Maybe PPB_Instance could be replaced with NoAPIName?

[dmichael (off chromium), 2013-06-17 21:48:27]

Thanks for the review! I addressed your comments in PS#2, merged in PS#3, and
fixed a couple things in PS#4.

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:31:
RenderProcessHost::FromID(render_process_id);
On 2013/06/16 23:49:53, yzshen1 wrote:
> 2 more spaces of indent, please.

Done.

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.h (right):

https://codereview.chromium.org/16819002/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_NETWORK_HOST_HOST_H_
On 2013/06/16 23:49:53, yzshen1 wrote:
> HOST_HOST -> PROXY_HOST, please.
Hah, nice catch, thanks.

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
File ppapi/cpp/dev/network_proxy_dev.cc (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
ppapi/cpp/dev/network_proxy_dev.cc:35: if (url_var.is_null())
On 2013/06/16 23:49:53, yzshen1 wrote:
> It seems unnecessary to check |url_var| here: the implementation of the C
> interface should have already checked the input arguments.
Good point. I took your suggestion of using Var, so it's gone now anyway. :-)

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
File ppapi/cpp/dev/network_proxy_dev.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/cpp/dev/network_prox...
ppapi/cpp/dev/network_proxy_dev.h:27: const std::string& url,
On 2013/06/16 23:49:53, yzshen1 wrote:
> - I think once Brett suggested me to have the C++ interface as a "thin"
wrapper
> around the C interface, not the convert the types too much. Maybe we should
use
> pp::Var for |url| here? Besides, it is more consistent with the output
> parameter. What do you think?
Yes, I wanted them to be consistent. It seems like the simplest for plugins is a
string. But I'm fine with pp::Var...  that's simpler for us, and does keep this
class thinner.

> (I think we should also look at how the other C++ interfaces deal with this
> problem, and do it in a consistent way.)
> 
> - WRT cancelling the callback, I think that is fine, it will get cancelled
when
> the instance goes away.

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
File ppapi/proxy/network_proxy_resource.cc (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
ppapi/proxy/network_proxy_resource.cc:40: this,
On 2013/06/16 23:49:53, yzshen1 wrote:
> nit, optional: It is safe and more clear to use base::Unretained(this).
Because
> this callback is stored in the base class of NetworkProxyResource. (Although
it
> also works to use |this| because we explicitly clear callbacks when all plugin
> references go away.)
Done, thanks for pointing that out.

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
ppapi/proxy/network_proxy_resource.cc:54: return;
So I guess that means I don't need this check. Since PluginResource clears all
its callbacks when the last plugin ref is deleted, we should never get here with
a callback that's not pending. Turned in to a DCHECK.

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
File ppapi/proxy/network_proxy_resource.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
ppapi/proxy/network_proxy_resource.h:25: virtual thunk::PPB_NetworkProxy_API*
AsPPB_NetworkProxy_API() OVERRIDE;
On 2013/06/16 23:49:53, yzshen1 wrote:
> nit: both of these two virtual methods are public in the base classes. It
seems
> better to still make them public, because when we call them with base class
> pointers, them are public anyway.
I subscribe to the philosophy of using the lowest visibility necessary. I like
that you can tell that the method is only ever called via the base class. This
has come up on chromium-dev before with differing opinions. Unfortunately, in
Pepper we're not very consistent right now. But I usually try to encourage least
privelege when I'm reviewing :)
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MkBEHBx3g_4/n-KkC...

We should sync up so we're reviewing consistently. WDYT?

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

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1434: // HostResolverPrivate
---------------------------------------------------------
On 2013/06/16 23:49:53, yzshen1 wrote:
> nit, optional: Maybe put the style&order fix in a separate CL will be easier
to
> review for the message file OWNER. And moving messages around may cause
> unnecessary conflicts because we are both changing them in a rush.
> (But it is not a big issue. I am fine if you would like to make the changes in
> this CL.)
Good point, done.

https://codereview.chromium.org/16819002/diff/9001/ppapi/tests/test_network_p...
File ppapi/tests/test_network_proxy.cc (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/tests/test_network_p...
ppapi/tests/test_network_proxy.cc:9: #include "ppapi/cpp/module.h"
On 2013/06/16 23:49:53, yzshen1 wrote:
> This include is not needed.

Done.

https://codereview.chromium.org/16819002/diff/9001/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_public_dev.h:31: PROXIED_IFACE(PPB_Instance,
PPB_NETWORKPROXY_DEV_INTERFACE_0_1,
On 2013/06/16 23:49:53, yzshen1 wrote:
> Maybe PPB_Instance could be replaced with NoAPIName?

Done.

[yzshen1, 2013-06-18 04:41:26]

LGTM
Thanks!

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
File ppapi/proxy/network_proxy_resource.h (right):

https://codereview.chromium.org/16819002/diff/9001/ppapi/proxy/network_proxy_...
ppapi/proxy/network_proxy_resource.h:25: virtual thunk::PPB_NetworkProxy_API*
AsPPB_NetworkProxy_API() OVERRIDE;
Okay. I am fine if this is well-thought-out. I previously had the same point of
view as Ryan's (in the mail thread you linked to).

On 2013/06/17 21:48:28, dmichael wrote:
> On 2013/06/16 23:49:53, yzshen1 wrote:
> > nit: both of these two virtual methods are public in the base classes. It
> seems
> > better to still make them public, because when we call them with base class
> > pointers, them are public anyway.
> I subscribe to the philosophy of using the lowest visibility necessary. I like
> that you can tell that the method is only ever called via the base class. This
> has come up on chromium-dev before with differing opinions. Unfortunately, in
> Pepper we're not very consistent right now. But I usually try to encourage
least
> privelege when I'm reviewing :)
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/MkBEHBx3g_4/n-KkC...
> 
> We should sync up so we're reviewing consistently. WDYT?

https://codereview.chromium.org/16819002/diff/40001/ppapi/proxy/network_proxy...
File ppapi/proxy/network_proxy_resource.cc (right):

https://codereview.chromium.org/16819002/diff/40001/ppapi/proxy/network_proxy...
ppapi/proxy/network_proxy_resource.cc:52:
DCHECK(TrackedCallback::IsPending(callback));
It seems better to me to still check whether it is pending and return if not 
(the original code).

Although I agree with you that DCHECK should be enough for now, it relies on a
few things implicitly:
- callbacks are cleared when the last plugin ref is gone.
- this is the only class that will ever try to run |callback|. (|callback| is
ref-counted, it is possible that someone else holds a ref to it and ran it
already.)

Besides, it is hard to hit this DCHECK if later we change code and some of those
implicit prerequisites are not true anymore. And we may not realize it and cause
memory corruption. 

What do you think?

[dmichael (off chromium), 2013-06-18 22:22:43]

+jam for content/content_browser.gypi
+jln for ppapi/proxy/ppapi_messages.h:
Note that this does not increase the attack surface. We already have
GetProxyForURL for Flash:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...
This is only exposed as a Dev interface (only trusted plugins get it, and NaCl
when --enable-nacl is used). You can see where the Dev check happens in
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc

https://codereview.chromium.org/16819002/diff/40001/ppapi/proxy/network_proxy...
File ppapi/proxy/network_proxy_resource.cc (right):

https://codereview.chromium.org/16819002/diff/40001/ppapi/proxy/network_proxy...
ppapi/proxy/network_proxy_resource.cc:52:
DCHECK(TrackedCallback::IsPending(callback));
On 2013/06/18 04:41:26, yzshen1 wrote:
> It seems better to me to still check whether it is pending and return if not 
> (the original code).
> 
> Although I agree with you that DCHECK should be enough for now, it relies on a
> few things implicitly:
> - callbacks are cleared when the last plugin ref is gone.
(This is also true of using base::Unretained(this) above instead of letting the
callback refcount it)
> - this is the only class that will ever try to run |callback|. (|callback| is
> ref-counted, it is possible that someone else holds a ref to it and ran it
> already.)
If we do that, it's probably a bug.

> 
> Besides, it is hard to hit this DCHECK if later we change code and some of
those
> implicit prerequisites are not true anymore. And we may not realize it and
cause
> memory corruption. 
> 
> What do you think?
I went for a compromise...  I put the old code back in, so that it will behave
reasonably if the callback has already been run. But I stuck in NOTIMPLEMENTED
this time, since if we hit that, I think it's probably a bug.

[jam, 2013-06-18 23:34:42]

On 2013/06/18 22:22:43, dmichael wrote:
> +jam for content/content_browser.gypi
lgtm

[yzshen1, 2013-06-19 16:50:59]

still LGTM

https://codereview.chromium.org/16819002/diff/40001/ppapi/proxy/network_proxy...
File ppapi/proxy/network_proxy_resource.cc (right):

https://codereview.chromium.org/16819002/diff/40001/ppapi/proxy/network_proxy...
ppapi/proxy/network_proxy_resource.cc:52:
DCHECK(TrackedCallback::IsPending(callback));
> (This is also true of using base::Unretained(this) above instead of letting
the
> callback refcount it)

There is a comment in plugin_resource.h about passing base::Unretained(), so it
is not implicit. :)

> I went for a compromise...  I put the old code back in, so that it will behave
> reasonably if the callback has already been run. But I stuck in NOTIMPLEMENTED
> this time, since if we hit that, I think it's probably a bug.
That sounds good to me.

[dmichael (off chromium), 2013-06-20 19:58:17]

+palmer for ppapi/proxy/ppapi_messages.h
Note that this does not increase the attack surface. We already have
GetProxyForURL for Flash:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppapi_...
This is only exposed as a Dev interface (only trusted plugins get it, and NaCl
when --enable-nacl is used). You can see where the Dev check happens in
content/browser/renderer_host/pepper/content_browser_pepper_host_factory.cc

[palmer, 2013-06-20 20:55:59]

https://codereview.chromium.org/16819002/diff/25004/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/16819002/diff/25004/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:94: const
std::string& url) {
Is this a good time to validate that the |url| is a real GURL? (Or just send
GURLs across the IPC boundary in the first place?)

https://codereview.chromium.org/16819002/diff/25004/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:110: } else if
(!gurl.is_valid()) {
At this point, we've done a fair amount of work. If it's not a valid GURL,
perhaps we could/should have failed faster/sooner?

https://codereview.chromium.org/16819002/diff/25004/ppapi/api/dev/ppb_network...
File ppapi/api/dev/ppb_network_proxy_dev.idl (right):

https://codereview.chromium.org/16819002/diff/25004/ppapi/api/dev/ppb_network...
ppapi/api/dev/ppb_network_proxy_dev.idl:21: *
http://en.wikipedia.org/wiki/Proxy_auto-config
Where does the PAC string get parsed? (How robust is the parser?)

[dmichael (off chromium), 2013-06-20 21:30:07]

palmer: Thanks for the review. Comments addressed, PTAL.

https://codereview.chromium.org/16819002/diff/25004/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/16819002/diff/25004/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:94: const
std::string& url) {
On 2013/06/20 20:56:00, Chromium Palmer wrote:
> Is this a good time to validate that the |url| is a real GURL?
Good suggestion, done.

> (Or just send
> GURLs across the IPC boundary in the first place?)
We can't send GURLs across the IPC boundary, because ppapi/proxy can't depend on
GURL. Well, it could...  but we'd have to build GURL in to the NaCl IRT, and we
want to avoid that. The plugin gives us a plain string anyway, so we just
validate on the host side.

https://codereview.chromium.org/16819002/diff/25004/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:110: } else if
(!gurl.is_valid()) {
On 2013/06/20 20:56:00, Chromium Palmer wrote:
> At this point, we've done a fair amount of work. If it's not a valid GURL,
> perhaps we could/should have failed faster/sooner?
Most of the time, as long as proxy_service_ has already been retrieved [which
happens once at plugin instance startup], we come in to this function
immediately, so it's not too bad. But validating GURL earlier seems even better
(especially when we're still waiting for proxy_service_).

https://codereview.chromium.org/16819002/diff/25004/ppapi/api/dev/ppb_network...
File ppapi/api/dev/ppb_network_proxy_dev.idl (right):

https://codereview.chromium.org/16819002/diff/25004/ppapi/api/dev/ppb_network...
ppapi/api/dev/ppb_network_proxy_dev.idl:21: *
http://en.wikipedia.org/wiki/Proxy_auto-config
On 2013/06/20 20:56:00, Chromium Palmer wrote:
> Where does the PAC string get parsed? (How robust is the parser?)
It's the plugin's responsibility, so we don't know or care whether it's robust.
We just give them the string.

[dmichael (off chromium), 2013-06-21 19:50:03]

palmer (or jln): ping? PTAL when you can, or let me know if I should find
another reviewer. Thanks!

[dmichael (off chromium), 2013-06-24 15:48:50]

Ping for security review? I was hoping to get this in before m29 branch point,
and I still have a slim chance if you can look at it this morning.

[palmer, 2013-06-24 20:48:20]

IPC Security LGTM.

[dmichael (off chromium), 2013-06-24 21:06:09]

Committed patchset #12 manually as r208259 (presubmit successful).