Add new text for indicating we are resolving hosts during proxy resolution.

Add new text for indicating we are resolving hosts during proxy resolution.

Required adding a new GetLoadStateThreadSafe() virtual method to the ProxyResolver interface. ProxyResolverV8 uses this.

BUG=59308
TEST=none


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

[willchan no longer on Chromium, 2011-10-24 19:28:48]

Eric, please advise on a better way to improve this so I can detect the actual
DNS lookup. I'm using a very hacky heuristic right now.

[eroman, 2011-10-25 00:26:37]

There are a couple different ways to get this information accurately. Probably
the easiest (since you have to whack through fewer layers) is to implement a
threadsafe function for the synchronous ProxyResolver implementations.

The way that would look, is first off in MultiThreadedProxyResolver you would
dispatch to the specific  proxy resolver for that executor (each thread has its
own instance of ProxyResolver).

LoadState MultiThreadedProxyResolver::GetLoadState(RequestHandle req) const {
   DCHECK(CalledOnValidThread());
   DCHECK(req);
 
   Job* job = reinterpret_cast<Job*>(req);

   if (!job->executor()) {
     // The request is queued
     return ....;
   }

   // NOTE: resolver() is not thread safe, and should only be operated from the
   // executor's thread... However GetLoadState is special and is allowed to be
   // called from a different thread.
   return job->executor()->resolver()->GetLoadState();
}


Now the remaining work is to make the synchronous implementations of
ProxyResolver have a useful GetLoadState(). The only synchronous implementation
that you will need to update is ProxyResolverV8, since that is the main one we
are using in Chrome.

Basically if you search for v8::Unlocker in the file proxy_resolver_v8.cc, you
will find all the locations where we call out to slow C++ bindings (i.e. DNS
resolves). Just set a boolean around each of these callsites which can be read
from another thread. For instance:

{
  v8::Unlocker unlocker;

  context->is_running_js_bindings_ = true;  // <<< ADDED
  success = context->js_bindings_->MyIpAddressEx(&ip_address_list);     
  context->is_running_js_bindings_ = false;   // <<< ADDED
}

Now add an implementation of GetLoadState() that does something like:

ProxyResolverV8::GetLoadState() {
  // NOTE: This could get run from other threads so needs to be thread safe.
  // Really is_running_js_bindings_ should be protected by a lock, but we
  // don't care too much about missing updates so lockless reads should be
  // fine...

  if (context_->is_running_js_bindings) {
    // The bindings basically just do DNS, so this means we are
    // host resolving. Or alternately could recursively ask the bindings
    // class what it is doing.
    return ...;
  }
}

I left the locking kinda hand-wavy in this example (I feel we could get away
without using a lock without danger here, but would be easy to wrap with a lock
too)

http://codereview.chromium.org/8373014/diff/4003/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8373014/diff/4003/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6375: Resolving host for proxy script...
nit: in my mind "resolving host for proxy script" means resolving the host of
the PAC URL itself. I would rather "resolving host by proxy script" or
"resolving host in proxy script"

[willchan no longer on Chromium, 2011-10-26 17:39:48]

OK Eric, I think I've implemented stuff as we discussed. Lemme know what you
think.

[willchan no longer on Chromium, 2011-10-31 16:51:30]

On 2011/10/26 17:39:48, willchan wrote:
> OK Eric, I think I've implemented stuff as we discussed. Lemme know what you
> think.

ping!

[willchan no longer on Chromium, 2011-11-01 18:58:34]

On 2011/10/31 16:51:30, willchan wrote:
> On 2011/10/26 17:39:48, willchan wrote:
> > OK Eric, I think I've implemented stuff as we discussed. Lemme know what you
> > think.
> 
> ping!

ping!

[eroman, 2011-11-07 21:41:46]

yikes apologies for the lag, went radio-silent last week.

Looking now!

[eroman, 2011-11-07 21:52:00]

LGTM!

http://codereview.chromium.org/8373014/diff/10001/net/proxy/multi_threaded_pr...
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/multi_threaded_pr...
net/proxy/multi_threaded_proxy_resolver.cc:466: return
job->executor()->resolver()->GetLoadStateThreadSafe(req);
passing in |req| doesn't make sense here, please pass NULL instead.

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_ma...
File net/proxy/proxy_resolver_mac.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_ma...
net/proxy/proxy_resolver_mac.cc:197: NOTREACHED();
This could cause DCHECKs in debug builds, of things like testshell, or when
running with flags like --single-process.

I suggest removing the NOTREACHED() here.

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_v8.cc
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_v8...
net/proxy/proxy_resolver_v8.cc:338: class ScopedHostResolve {
nit: you can move this to the private section. doesn't really matter since this
whole class is file-private though, so whatever.

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_wi...
File net/proxy/proxy_resolver_winhttp.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_wi...
net/proxy/proxy_resolver_winhttp.cc:134: NOTREACHED();
Please avoid using NOTREACHED(), for the same reasons I described in the Mac one
-- this code can actually be reached in certain configurations.

[commit-bot: I haz the power, 2011-11-07 23:22:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/8373014/16004

[willchan no longer on Chromium, 2011-11-07 23:23:39]

http://codereview.chromium.org/8373014/diff/10001/net/proxy/multi_threaded_pr...
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/multi_threaded_pr...
net/proxy/multi_threaded_proxy_resolver.cc:466: return
job->executor()->resolver()->GetLoadStateThreadSafe(req);
On 2011/11/07 21:52:01, eroman wrote:
> passing in |req| doesn't make sense here, please pass NULL instead.

Done.

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_ma...
File net/proxy/proxy_resolver_mac.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_ma...
net/proxy/proxy_resolver_mac.cc:197: NOTREACHED();
On 2011/11/07 21:52:01, eroman wrote:
> This could cause DCHECKs in debug builds, of things like testshell, or when
> running with flags like --single-process.
> 
> I suggest removing the NOTREACHED() here.

Done.

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_v8.cc
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_v8...
net/proxy/proxy_resolver_v8.cc:338: class ScopedHostResolve {
On 2011/11/07 21:52:01, eroman wrote:
> nit: you can move this to the private section. doesn't really matter since
this
> whole class is file-private though, so whatever.

Done.

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_wi...
File net/proxy/proxy_resolver_winhttp.cc (right):

http://codereview.chromium.org/8373014/diff/10001/net/proxy/proxy_resolver_wi...
net/proxy/proxy_resolver_winhttp.cc:134: NOTREACHED();
On 2011/11/07 21:52:01, eroman wrote:
> Please avoid using NOTREACHED(), for the same reasons I described in the Mac
one
> -- this code can actually be reached in certain configurations.

Done.

[commit-bot: I haz the power, 2011-11-08 00:33:19]

Change committed as 108950