Adding the ability to alter Chrome's proxy settings via the automation interf...

Adding the ability to alter Chrome's proxy settings via the automation interface for automated testing.


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

[amit, 2009-05-18 21:51:38]

[+ wtc - review proxy stuff]
[+ ian - security implications of parsing proxy information encoded as JSON]

The code looks good overall. Passing proxy information as JSON is what I would
prefer too but lets see if this has any security implications.

http://codereview.chromium.org/113482/diff/1/6
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/113482/diff/1/6#newcode2335
Line 2335: SetProxyConfigTask set_proxy_config_task(new_proxy_config);
I am not clear about the need to create a Task subclass for this, since we are
not running it out of context.

[eroman, 2009-05-19 00:51:10]

http://codereview.chromium.org/113482/diff/1/6
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/113482/diff/1/6#newcode2255
Line 2255: virtual void Run() {
What thread is this being run on?

For the call to proxy_service->ResetConfigService() to be valid, this must be
called from the IO thread.

However, for the calls to g_browser_process below to be valid, must be called
from UI thread.

So something is off here...

http://codereview.chromium.org/113482/diff/1/6#newcode2259
Line 2259: // TODO(robertshield): Investigate when the first URLRequestContext
is
Whenever I debug this, I find that it gets lazily created when creating the
first render view. Definitely not the first network request.

 	chrome.dll!ChromeURLRequestContext::CreateOriginal()  Line 64	C++
 	chrome.dll!ProfileImpl::GetRequestContext()  Line 619	C++
>	chrome.dll!ProfileImpl::InitializeSpellChecker()  Line 842	C++
 	chrome.dll!ProfileImpl::GetSpellChecker()  Line 873	C++
 	chrome.dll!BrowserRenderProcessHost::Init()  Line 204	C++
 	chrome.dll!RenderViewHost::CreateRenderView()  Line 135	C++
 	chrome.dll!TabContents::CreateRenderViewForRenderManager()  Line 2385	C++
 	chrome.dll!RenderViewHostManager::Navigate()  Line 84	C++
 	chrome.dll!TabContents::NavigateToPendingEntry()  Line 665	C++
 	chrome.dll!NavigationController::NavigateToPendingEntry()  Line 862	C++
 	chrome.dll!NavigationController::LoadEntry()  Line 204	C++
 	chrome.dll!NavigationController::LoadURL()  Line 382	C++
 	chrome.dll!Browser::CreateTabContentsForURL()  Line 1416	C++
 	chrome.dll!Browser::AddTabWithURL()  Line 521	C++

http://codereview.chromium.org/113482/diff/1/6#newcode2267
Line 2267: Profile* profile = profile_manager->GetDefaultProfile(user_data_dir);
See comment above; BrowserProcessImpl is not thread safe.

http://codereview.chromium.org/113482/diff/1/6#newcode2289
Line 2289: << error_message.c_str();
The ".c_str()" seems unnecessary.

http://codereview.chromium.org/113482/diff/1/6#newcode2301
Line 2301: proxy_service->ResetConfigService(proxy_config_service.release());
I wander about the case where the ProxyService has a NULL value for
ProxyResolver, and yet |pc.MayRequirePACResolver()|

(Basically the same scenario we discussed in
http://codereview.chromium.org/115029).

Maybe the best thing would be to never let ProxyResolver be NULL, so we don't
need to always worry about if it might be NULL. Instead of NULL we could pass in
a dummy like:

class ProxyResolverNull : public ProxyResolver {
  public:
    virtual int GetProxyForURL(const GURL&, cont GURL&, ProxyInfo*) {
       return ERR_NOT_IMPLEMENTED;
    }
};

If you are cool with that let me know and I can make the change to
proxy_service.cc.

http://codereview.chromium.org/113482/diff/1/13
File chrome/chrome.gyp (right):

http://codereview.chromium.org/113482/diff/1/13#newcode354
Line 354: 'common/automation_constants.cc',
flip the order so .cc appears before .h

http://codereview.chromium.org/113482/diff/1/7
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/113482/diff/1/7#newcode541
Line 541: void ProxyService::ResetConfigService(
I realize this is a small function, but can you please add a unit to
proxy_service.cc ?

Will be real simple, just to verify that the next resolve uses the new config.

[robertshield, 2009-05-19 18:05:34]

http://codereview.chromium.org/113482/diff/1/6
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/113482/diff/1/6#newcode2255
Line 2255: virtual void Run() {
On 2009/05/19 00:51:10, eroman wrote:
> What thread is this being run on?
> 
> For the call to proxy_service->ResetConfigService() to be valid, this must be
> called from the IO thread.
> 
> However, for the calls to g_browser_process below to be valid, must be called
> from UI thread.
> 
> So something is off here...

Yes, I wasn't particularly sure that this was correct :-) This runs on the main
thread (aka the UI thread?), allowing access to g_browser_process and keeping
URLRequestContext creation (and so by extension ProxyService creation) happening
on the main thread too. Since the ProxyService can be created here, I update the
settings here. Where I fall down is if the ProxyService is already created.

I think that what I want to do is:

* If I need to create the default request context leading to the creation of the
proxy service, then I want to call proxy_service->ResetConfigService()
immediately on the main thread as is done now.

* If a request context already exists (and thus the proxy service has already
been created) then I want to call proxy_service->ResetConfigService() on the IO
thread.

Does this make sense?

http://codereview.chromium.org/113482/diff/1/6#newcode2259
Line 2259: // TODO(robertshield): Investigate when the first URLRequestContext
is
On 2009/05/19 00:51:10, eroman wrote:
> Whenever I debug this, I find that it gets lazily created when creating the
> first render view. Definitely not the first network request.
> 
>  	chrome.dll!ChromeURLRequestContext::CreateOriginal()  Line 64	C++
>  	chrome.dll!ProfileImpl::GetRequestContext()  Line 619	C++
> >	chrome.dll!ProfileImpl::InitializeSpellChecker()  Line 842	C++
>  	chrome.dll!ProfileImpl::GetSpellChecker()  Line 873	C++
>  	chrome.dll!BrowserRenderProcessHost::Init()  Line 204	C++
>  	chrome.dll!RenderViewHost::CreateRenderView()  Line 135	C++
>  	chrome.dll!TabContents::CreateRenderViewForRenderManager()  Line 2385	C++
>  	chrome.dll!RenderViewHostManager::Navigate()  Line 84	C++
>  	chrome.dll!TabContents::NavigateToPendingEntry()  Line 665	C++
>  	chrome.dll!NavigationController::NavigateToPendingEntry()  Line 862	C++
>  	chrome.dll!NavigationController::LoadEntry()  Line 204	C++
>  	chrome.dll!NavigationController::LoadURL()  Line 382	C++
>  	chrome.dll!Browser::CreateTabContentsForURL()  Line 1416	C++
>  	chrome.dll!Browser::AddTabWithURL()  Line 521	C++
> 
> 

Ah, I should have removed the TODO, I had found the same thing. Comment removed.

http://codereview.chromium.org/113482/diff/1/6#newcode2267
Line 2267: Profile* profile = profile_manager->GetDefaultProfile(user_data_dir);
On 2009/05/19 00:51:10, eroman wrote:
> See comment above; BrowserProcessImpl is not thread safe.

See threading comment above. (This is being run on the main thread).

http://codereview.chromium.org/113482/diff/1/6#newcode2289
Line 2289: << error_message.c_str();
On 2009/05/19 00:51:10, eroman wrote:
> The ".c_str()" seems unnecessary.

Old habits.. removed.

http://codereview.chromium.org/113482/diff/1/6#newcode2301
Line 2301: proxy_service->ResetConfigService(proxy_config_service.release());
On 2009/05/19 00:51:10, eroman wrote:
> I wander about the case where the ProxyService has a NULL value for
> ProxyResolver, and yet |pc.MayRequirePACResolver()|
> 
> (Basically the same scenario we discussed in
> http://codereview.chromium.org/115029).
> 
> Maybe the best thing would be to never let ProxyResolver be NULL, so we don't
> need to always worry about if it might be NULL. Instead of NULL we could pass
in
> a dummy like:
> 
> class ProxyResolverNull : public ProxyResolver {
>   public:
>     virtual int GetProxyForURL(const GURL&, cont GURL&, ProxyInfo*) {
>        return ERR_NOT_IMPLEMENTED;
>     }
> };
> 
> If you are cool with that let me know and I can make the change to
> proxy_service.cc.

Good point. What you suggest makes sense. Longer term, it might be nice to have
a way to reset the resolver being used  as well, but the ProxyResolverNull class
is a good idea in any case.

http://codereview.chromium.org/113482/diff/1/6#newcode2335
Line 2335: SetProxyConfigTask set_proxy_config_task(new_proxy_config);
On 2009/05/18 21:51:38, amit wrote:
> I am not clear about the need to create a Task subclass for this, since we are
> not running it out of context.

See above comments for threading related matters. In summary, we can't always
run this on the main thread, so I'm keeping the Task around.

http://codereview.chromium.org/113482/diff/1/13
File chrome/chrome.gyp (right):

http://codereview.chromium.org/113482/diff/1/13#newcode354
Line 354: 'common/automation_constants.cc',
On 2009/05/19 00:51:10, eroman wrote:
> flip the order so .cc appears before .h

Done.

http://codereview.chromium.org/113482/diff/1/7
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/113482/diff/1/7#newcode541
Line 541: void ProxyService::ResetConfigService(
On 2009/05/19 00:51:10, eroman wrote:
> I realize this is a small function, but can you please add a unit to
> proxy_service.cc ?
> 
> Will be real simple, just to verify that the next resolve uses the new config.

Done.

[eroman, 2009-05-19 20:31:05]

lgtm on the proxy service side.

http://codereview.chromium.org/113482/diff/1/6
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/113482/diff/1/6#newcode2255
Line 2255: virtual void Run() {
On 2009/05/19 18:05:34, robertshield wrote:
> On 2009/05/19 00:51:10, eroman wrote:
> > What thread is this being run on?
> > 
> > For the call to proxy_service->ResetConfigService() to be valid, this must
be
> > called from the IO thread.
> > 
> > However, for the calls to g_browser_process below to be valid, must be
called
> > from UI thread.
> > 
> > So something is off here...
> 
> Yes, I wasn't particularly sure that this was correct :-) This runs on the
main
> thread (aka the UI thread?), allowing access to g_browser_process and keeping
> URLRequestContext creation (and so by extension ProxyService creation)
happening
> on the main thread too. Since the ProxyService can be created here, I update
the
> settings here. Where I fall down is if the ProxyService is already created.
> 
> I think that what I want to do is:
> 
> * If I need to create the default request context leading to the creation of
the
> proxy service, then I want to call proxy_service->ResetConfigService()
> immediately on the main thread as is done now.
> 
> * If a request context already exists (and thus the proxy service has already
> been created) then I want to call proxy_service->ResetConfigService() on the
IO
> thread.
> 
> Does this make sense?
> 
> 

That sounds good to me.
I reviewed the latest version and it looks good.
(Although I am not very familiar with the "if (!context)" block, so I will leave
that for another reviewer).

[robertshield, 2009-05-20 18:07:46]

Hi Brett, 

  I'm working on a patch here (http://codereview.chromium.org/113482) to set the
proxy settings in Chrome via the automation interface. Amit suggested you could
double check the URLRequestContext-related code in
AutomationProvider::SetProxyConfig in automation_provider.cc. Would you mind
taking a look?

Thanks,
Robert

[brettw, 2009-05-21 21:09:37]

Wan-teh: I don't have much experience with the URLRequestContext and proxy
stuff, can you take a look?

[brettw, 2009-05-21 21:19:38]

Wan-teh: looks like this is not necessary after all, thanks anyway.