Allow ProxyService to share URLRequestContext with everything else.

net/proxy/proxy_service.cc

Index: net/proxy/proxy_service.cc
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index a81d3f66bc7547e22e813ff62ec94e9c0dd72e7d..41fe8bbac1cf7e8bea2851d15ea6b8925be43642 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -1044,9 +1044,10 @@ int ProxyService::ResolveProxyHelper(const GURL& raw_url,
 
   // Notify our polling-based dependencies that a resolve is taking place.
   // This way they can schedule their polls in response to network activity.
-  config_service_->OnLazyPoll();
+  if (config_service_)
+    config_service_->OnLazyPoll();
   if (script_poller_.get())
-     script_poller_->OnLazyPoll();
+    script_poller_->OnLazyPoll();
 
   if (current_state_ == STATE_NONE)
     ApplyProxyConfigIfAvailable();
@@ -1135,7 +1136,8 @@ int ProxyService::TryToCompleteSynchronously(const GURL& url,
 ProxyService::~ProxyService() {
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
   NetworkChangeNotifier::RemoveDNSObserver(this);
-  config_service_->RemoveObserver(this);
+  if (config_service_)
+    config_service_->RemoveObserver(this);
 
   // Cancel any inprogress requests.
   for (PendingRequests::iterator it = pending_requests_.begin();
@@ -1458,9 +1460,24 @@ void ProxyService::SetProxyScriptFetchers(
     ApplyProxyConfigIfAvailable();
 }
 
-ProxyScriptFetcher* ProxyService::GetProxyScriptFetcher() const {
+void ProxyService::ShutDown() {
   DCHECK(CalledOnValidThread());
-  return proxy_script_fetcher_.get();
+
+  // Cancel up in progress work, and destroy all objects that depend on the two

[eroman, 2017/04/27 19:35:41]

ProxyService is already a huge mess. I would rather not add extra stuff into it,
as it is already too complicated.

How about moving the notion of shutdown into to the leaf dependency
(ProxyScriptFetcher) ?

In particular, ProxyScriptFetcherImpl is the class which holds a pointer to the
URLRequestContext [1].

So we could solve this more directly by adding a
ProxyScriptFetcherImpl::Shutdown() method that:

  * Aborts any outstanding URLRequest
  * Nulls its URLRequestContext pointer
  * Marks itself as shutdown (such that any new requests will fail immediately
with ERR_ABORTED).

This means that ProxyService continues working as before, only its
ProxyScriptFetcher behaves as if it has no network connectivity.

As far as how to plumb that through I leave it up to you. As a strawman proposal
could do:

  ProxyService::DetachURLRequestContext(URLRequest* context)
      calls ProxyScriptFetcher::DetachURLRequestContext(context)

ProxyScriptFetcherImpl::DetachURLRequestContext(context) is the only
implementation that needs to do anything - it would enter shutdown mode per
previous description.

[1] OK so so I failed to describe ProxyScriptDecider, which unfortunately grew a
dependency on the URLRequestContext for the quick detect stuff. So the final
solution won't be as clean as I made it out to be, but should be able to adapt
it.

WDYT?

[mmenke, 2017/04/27 20:02:13]

I agree that this class is a complete mess.  Unwrangling the ProxyService,
ProxyConfigService, ProxyResolver, ProxyResolverFactory, ProxyDecider,
DhcpThingy, ProxyScriptFetcher, and InitializerClassThingy wasn't a particularly
fun experience, and I still only have vague notions of how they all plug in to
each other.

Don't forget we'd also need to hook it up to the
Dhcp-Proxy-Script-Fetcher-Resolver-Service-Host-Decider thingy, too.

Why would we take the URLRequestContext as an argument?  Are you thinking we'd
hook it up to URLRequestContext's destructor, and then if it's not the one being
used by the URLRequest, it just does nothing?  I have been thinking we'd tie
teardown to the URLRequestContextStorage, not the URLRequestContext, because of
all the hacky component storage, but putting stuff in the URLRequestContext
destructor may be fine.

+  // fetchers.
+  ResetProxyConfig(true);
+
+  // Prevent proxy service changes from waking up the ProxyService.

[eroman, 2017/04/27 19:35:42]

config service

+  if (config_service_) {
+    config_service_->RemoveObserver(this);
+    config_service_.reset();
+  }
+
+  // Destroy fetchers, which also cancels in-progress network requests.
+  proxy_script_fetcher_.reset();
+  dhcp_proxy_script_fetcher_.reset();
+
+  current_state_ = STATE_SHUTDOWN;
 }
 
 ProxyService::State ProxyService::ResetProxyConfig(bool reset_fetched_config) {
@@ -1500,6 +1517,11 @@ void ProxyService::ResetConfigService(
 
 void ProxyService::ForceReloadProxyConfig() {
   DCHECK(CalledOnValidThread());
+
+  // Refuse to reload proxy config if already shut down.
+  if (current_state_ == STATE_SHUTDOWN)
+    return;
+
   ResetProxyConfig(false);
   ApplyProxyConfigIfAvailable();
 }