Allow ProxyService to share URLRequestContext with everything else.

net/proxy/proxy_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/proxy/proxy_service.h"
 
 #include <algorithm>
 #include <cmath>
 #include <utility>
 
@@ skipping 1026 matching lines @@
                                      const CompletionCallback& callback,
                                      PacRequest** pac_request,
                                      ProxyDelegate* proxy_delegate,
                                      const NetLogWithSource& net_log) {
   DCHECK(CalledOnValidThread());
 
   net_log.BeginEvent(NetLogEventType::PROXY_SERVICE);
 
   // 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();
 
   // Sanitize the URL before passing it on to the proxy resolver (i.e. PAC
   // script). The goal is to remove sensitive data (like embedded user names
   // and password), and local data (i.e. reference fragment) which does not need
   // to be disclosed to the resolver.
   GURL url = SanitizeUrl(raw_url, sanitize_url_policy_);
 
@@ skipping 68 matching lines @@
   config_.proxy_rules().Apply(url, result);
   result->config_source_ = config_.source();
   result->config_id_ = config_.id();
 
   return OK;
 }
 
 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();
        it != pending_requests_.end();
        ++it) {
     (*it)->Cancel();
   }
 }
 
 void ProxyService::SuspendAllPendingRequests() {
@@ skipping 293 matching lines @@
         ApplyProxyConfigIfAvailable();
     }
   }
 
   net_log.EndEvent(NetLogEventType::PROXY_SERVICE);
   return result_code;
 }
 
 void ProxyService::SetProxyScriptFetchers(
     ProxyScriptFetcher* proxy_script_fetcher,
     std::unique_ptr<DhcpProxyScriptFetcher> dhcp_proxy_script_fetcher) {

[mmenke, 2017/04/27 18:37:31]

Another option would be to just call this method with null fetchers, and make it
safe to do that while it's working (move old fetchers to temporary locations
before calling ApplyProxyConfigIfAvailable), but I'm worried that would add more
weird behavior prior to shutdown.

   DCHECK(CalledOnValidThread());
   State previous_state = ResetProxyConfig(false);
   proxy_script_fetcher_.reset(proxy_script_fetcher);
   dhcp_proxy_script_fetcher_ = std::move(dhcp_proxy_script_fetcher);
   if (previous_state != STATE_NONE)
     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) {
   DCHECK(CalledOnValidThread());
   State previous_state = current_state_;
 
   permanent_error_ = OK;
   proxy_retry_info_.clear();
   script_poller_.reset();
   init_proxy_resolver_.reset();
@@ skipping 19 matching lines @@
   // Set the new configuration service.
   config_service_ = std::move(new_proxy_config_service);
   config_service_->AddObserver(this);
 
   if (previous_state != STATE_NONE)
     ApplyProxyConfigIfAvailable();
 }
 
 void ProxyService::ForceReloadProxyConfig() {
   DCHECK(CalledOnValidThread());
+
+  // Refuse to reload proxy config if already shut down.
+  if (current_state_ == STATE_SHUTDOWN)
+    return;
+
   ResetProxyConfig(false);
   ApplyProxyConfigIfAvailable();
 }
 
 // static
 std::unique_ptr<ProxyConfigService>
 ProxyService::CreateSystemProxyConfigService(
     const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner,
     const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner) {
 #if defined(OS_WIN)
@@ skipping 143 matching lines @@
   State previous_state = ResetProxyConfig(false);
   if (previous_state != STATE_NONE)
     ApplyProxyConfigIfAvailable();
 }
 
 void ProxyService::OnDNSChanged() {
   OnIPAddressChanged();
 }
 
 }  // namespace net