Start using the proxy resolve IPC for plugins....

Move proxy resolve requests out of plugin/renderer process, and into the browser.


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

[eroman, 2008-12-16 01:02:16]

I am sending this change out primarily to get design feedback.
Written as-is it is incomplete.

The problem that is arising, is the need to have 4 separate implementations:

1. chrome.exe:
  - IPC from plugin to browser 

2. chrome.exe (-in-process-plugins)
  - IPC from renderer to browser

3. chrome.exe (--single-process)
  - need to run
Profile::GetDefaultRequestContext()->proxy_service()->ResovleProxy() run the
Chrome_IOThread, and provide a sync wrapper.

4. test_shell.exe
  - need to run request_context->proxy_service()->ResolveProxy() on io_thread,
and provide a sync wrapper. [both these globals live in
simple_resource_loader_bridge.cc]


I provided (1) and (4) in this changelist, but don't see this scaling nicely to
encompass (2) and (3).

Thanks!

[darin (slow to review), 2008-12-16 03:13:44]

http://codereview.chromium.org/14142/diff/6/207
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/14142/diff/6/207#newcode480
Line 480: class SyncProxyResolver : public base::RefCounted<SyncProxyResolver> {
i think you want to use threadsafe addref/release here.

http://codereview.chromium.org/14142/diff/6/207#newcode495
Line 495: event_.Reset();
this call to .Reset() seems unnecessary.

[jam, 2008-12-16 20:32:04]

the design looks good to me, 2/3 can just be implemented by 4 I think.

http://codereview.chromium.org/14142/diff/6/207
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/14142/diff/6/207#newcode585
Line 585: return rv == net::OK;
why not do the same thing in plugin_thread.cc when running in the
browser/renderer process?

[eroman, 2008-12-16 20:49:50]

http://codereview.chromium.org/14142/diff/6/207
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/14142/diff/6/207#newcode585
Line 585: return rv == net::OK;
On 2008/12/16 20:32:04, John Abd-El-Malek wrote:
> why not do the same thing in plugin_thread.cc when running in the
> browser/renderer process?

In the case of the renderer, we can't do this since the ProxyService object
lives in the browser (part of the profile's URLRequestContext). So would need to
send an IPC to the browser. Not sure if I can directly call
PluginProcessHostMsg_ResolveProxy, or need to define a new IPC that does the
same thing.

[jam, 2008-12-16 21:26:20]

http://codereview.chromium.org/14142/diff/6/207
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/14142/diff/6/207#newcode585
Line 585: return rv == net::OK;
On 2008/12/16 20:49:50, eroman wrote:
> On 2008/12/16 20:32:04, John Abd-El-Malek wrote:
> > why not do the same thing in plugin_thread.cc when running in the
> > browser/renderer process?
> 
> In the case of the renderer, we can't do this since the ProxyService object
> lives in the browser (part of the profile's URLRequestContext). So would need
to
> send an IPC to the browser. Not sure if I can directly call
> PluginProcessHostMsg_ResolveProxy, or need to define a new IPC that does the
> same thing.

that's true, I don't think it's that important to do 2/3 since this is only used
by the java plugin.

do we even really need to do this for test_shell? (i.e. all the code in this
file)

[eroman, 2008-12-16 23:31:52]

Thanks for the feedback.

Updated the patchset: moved the helper out of simple_resource_loader_bridge.cc,
so there there is fewer test_shell-specific code.

The code for(3) --single-process should be straightforward, something like:

// chrome.exe --single-process implementation:
bool PluginFindProxyForUrl(const GURL& url, std::string* proxy_list) {
  MessageLoop* io_thread_message_loop =
      ChromeThread::GetMessageLoop(ChromeThread::IO);
  ProxyService* proxy_service =
      Profile::GetDefaultRequestContext()->proxy_service();

  scoped_refptr<SyncProxyResolverHelper> sync_resolver(
      new SyncProxyResolver(io_thread_message_loop, proxy_service));

  net::ProxyInfo proxy_info;
  int rv = sync_resolver->ResolveProxy(url, &proxy_info);
  if (rv == net::OK) {
    *proxy_list = proxy_info.GetAnnotatedProxyList();
  }

  return rv == net::OK;
}

The issue is where to put such code, since it references chrome symbols. (pretty
sure plugin_thread.cc would be the wrong place).

[eroman, 2009-01-29 02:24:42]

ping.

[eroman, 2009-01-29 02:25:56]

I have updated the patchset so it now encompasses all of the cases (single
process mode, in-renderer mode, and oop-plugin mode).

if this approach works for you, i will update with unit-tests.

[darin (slow to review), 2009-01-29 05:32:33]

design looks good to me.

http://codereview.chromium.org/14142/diff/401/604
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/14142/diff/401/604#newcode198
Line 198: return ipc_ok ? net_error : -1;
maybe instead of -1, use ERR_FAILED or ERR_UNEXPECTED?

http://codereview.chromium.org/14142/diff/401/604#newcode214
Line 214: extern int ResolveProxyFromRenderThread(const GURL&, std::string*);
hrm... it is a little unusual to see a function declaration within a block of
code like this.  i know it is valid, but seems like the normal thing would be to
put it at file scope.

http://codereview.chromium.org/14142/diff/401/613
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/14142/diff/401/613#newcode286
Line 286: return ipc_ok ? net_error : -1;
ditto re: -1

http://codereview.chromium.org/14142/diff/401/605
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/14142/diff/401/605#newcode576
Line 576: event_.Reset();
maybe just create an auto-reset event?

http://codereview.chromium.org/14142/diff/401/603
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/14142/diff/401/603#newcode546
Line 546: return true;
extraneous return statement?

[eroman, 2009-01-29 09:44:06]

Sorry, I was missing two newly added files in the previous upload.

Please also review:
  chrome/browser/resolve_proxy_msg_helper.h
  chrome/browser/resolve_proxy_msg_helper.cc

thx!

http://codereview.chromium.org/14142/diff/401/604
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/14142/diff/401/604#newcode198
Line 198: return ipc_ok ? net_error : -1;
On 2009/01/29 05:32:33, darin wrote:
> maybe instead of -1, use ERR_FAILED or ERR_UNEXPECTED?

Done (ERR_UNEXPECTED).

http://codereview.chromium.org/14142/diff/401/604#newcode214
Line 214: extern int ResolveProxyFromRenderThread(const GURL&, std::string*);
On 2009/01/29 05:32:33, darin wrote:
> hrm... it is a little unusual to see a function declaration within a block of
> code like this.  i know it is valid, but seems like the normal thing would be
to
> put it at file scope.

Moved it to file scope.

I am not really happy with this extern to start with, but it seemed less evil
then trying to include a header for chrome/renderer/<X>

Another solution I was considering is to have a static field on PluginHost, that
gets filled with a function pointer to the correct implementation during
initialization (so now the renderer/plugin symbols remain isolated in their
respective files).

http://codereview.chromium.org/14142/diff/401/613
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/14142/diff/401/613#newcode286
Line 286: return ipc_ok ? net_error : -1;
On 2009/01/29 05:32:33, darin wrote:
> ditto re: -1

Done.

http://codereview.chromium.org/14142/diff/401/605
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/14142/diff/401/605#newcode576
Line 576: event_.Reset();
On 2009/01/29 05:32:33, darin wrote:
> maybe just create an auto-reset event?

Done.

Just to confirm since I am not too clear on what auto-reset events mean:

if event_.Signal() is reached before event_.Wait(), the event_.Wait() call is
not going to block right?

http://codereview.chromium.org/14142/diff/401/603
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/14142/diff/401/603#newcode546
Line 546: return true;
On 2009/01/29 05:32:33, darin wrote:
> extraneous return statement?

eek. sorry!

[darin (slow to review), 2009-01-29 21:20:12]

LGTM

http://codereview.chromium.org/14142/diff/401/604
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/14142/diff/401/604#newcode214
Line 214: extern int ResolveProxyFromRenderThread(const GURL&, std::string*);
Oh, I see...

Your idea of setting a static function pointer is better.

http://codereview.chromium.org/14142/diff/401/605
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/14142/diff/401/605#newcode576
Line 576: event_.Reset();
Right.  Auto-reset means that only one call to Wait will not block.  Other
callers of Wait will have to wait for Signal to be called again.

[darin (slow to review), 2009-01-29 21:22:53]

http://codereview.chromium.org/14142/diff/421/638
File chrome/browser/resolve_proxy_msg_helper.cc (right):

http://codereview.chromium.org/14142/diff/421/638#newcode12
Line 12: // TODO(eroman): Add unit tests
nit: usually best to do so upfront.  otherwise, it is easy to forget :)

http://codereview.chromium.org/14142/diff/421/637
File chrome/browser/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/14142/diff/421/637#newcode6
Line 6: #define CHROME_BROWSER_RESOLVE_PROXY_MSG_HELPER_
actually, can you put this inside chrome/browser/net/ ?

http://codereview.chromium.org/14142/diff/421/637#newcode42
Line 42: virtual ~Delegate();
nit: i think it is better to provide the implementation of ~Delegate inline:

virtual ~Delegate() {}

http://codereview.chromium.org/14142/diff/421/637#newcode91
Line 91: std::deque<PendingRequest> pending_requests_;
nit: make a typedef for this std::deque<T> usage?  maybe PendingRequestList?

[jam, 2009-01-29 21:26:53]

lgtm

it's amazing that so much work had to be done just for the java plugin's proxy
resolve code..

[eroman, 2009-01-29 22:22:01]

> it's amazing that so much work had to be done just for the java plugin's
> proxy resolve code..

Yeah, sorry for the extra complexity.

For what it's worth, it does solve a real bug (java plugin not respecting same
proxy settings as browser). But more importantly it gives us the freedom to
change ProxyService as needed for V8-PAC implementation.

I will address the other comments shortly.

[eroman, 2009-01-30 10:37:02]

http://codereview.chromium.org/14142/diff/421/638
File chrome/browser/resolve_proxy_msg_helper.cc (right):

http://codereview.chromium.org/14142/diff/421/638#newcode12
Line 12: // TODO(eroman): Add unit tests
On 2009/01/29 21:22:53, darin wrote:
> nit: usually best to do so upfront.  otherwise, it is easy to forget :)

Done.

Please see "resolve_proxy_msg_helper_unittest.cc"

http://codereview.chromium.org/14142/diff/421/637
File chrome/browser/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/14142/diff/421/637#newcode6
Line 6: #define CHROME_BROWSER_RESOLVE_PROXY_MSG_HELPER_
On 2009/01/29 21:22:53, darin wrote:
> actually, can you put this inside chrome/browser/net/ ?

Done.

http://codereview.chromium.org/14142/diff/421/637#newcode42
Line 42: virtual ~Delegate();
On 2009/01/29 21:22:53, darin wrote:
> nit: i think it is better to provide the implementation of ~Delegate inline:
> 
> virtual ~Delegate() {}

Done.

http://codereview.chromium.org/14142/diff/421/637#newcode91
Line 91: std::deque<PendingRequest> pending_requests_;
On 2009/01/29 21:22:53, darin wrote:
> nit: make a typedef for this std::deque<T> usage?  maybe PendingRequestList?

Done.

[darin (slow to review), 2009-01-30 16:47:19]

LGTM

http://codereview.chromium.org/14142/diff/456/684
File chrome/browser/net/resolve_proxy_msg_helper_unittest.cc (right):

http://codereview.chromium.org/14142/diff/456/684#newcode205
Line 205: // Avoid the need to have an AddRef / ReleaseRef.
nit: ReleaseRef -> Release