Re: Add new text for indicating we are resolving hosts during proxy resolution. (issue 8373014)

[ero...@chromium.org]

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_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8373014/diff/4003/chrome/app/generated_resources.grd#newcode6375
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"

http://codereview.chromium.org/8373014/

[will...@chromium.org]

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

http://codereview.chromium.org/8373014/

[will...@chromium.org]

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!

http://codereview.chromium.org/8373014/

[will...@chromium.org]

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!

http://codereview.chromium.org/8373014/