Don't delay creation of system URLRequestContext until first use

components/proxy_config/pref_proxy_config_tracker_impl.cc

 // Copyright (c) 2011 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 "components/proxy_config/pref_proxy_config_tracker_impl.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/values.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/proxy_config/proxy_config_dictionary.h"
 #include "components/proxy_config/proxy_config_pref_names.h"
 #include "net/proxy/proxy_server.h"
 #include "url/gurl.h"
 
 //============================= ProxyConfigServiceImpl =======================
 
 ProxyConfigServiceImpl::ProxyConfigServiceImpl(
-    net::ProxyConfigService* base_service)
+    net::ProxyConfigService* base_service,
+    ProxyPrefs::ConfigState initial_config_state,
+    const net::ProxyConfig& initial_config)
     : base_service_(base_service),
-      pref_config_state_(ProxyPrefs::CONFIG_UNSET),
-      pref_config_read_pending_(true),
+      pref_config_state_(initial_config_state),
+      pref_config_(initial_config),
       registered_observer_(false) {
   // ProxyConfigServiceImpl is created on the UI thread, but used on the network
   // thread.
   thread_checker_.DetachFromThread();
 }
 
 ProxyConfigServiceImpl::~ProxyConfigServiceImpl() {
   if (registered_observer_ && base_service_.get())
     base_service_->RemoveObserver(this);
 }
 
 void ProxyConfigServiceImpl::AddObserver(
     net::ProxyConfigService::Observer* observer) {
   RegisterObserver();
   observers_.AddObserver(observer);
 }
 
 void ProxyConfigServiceImpl::RemoveObserver(
     net::ProxyConfigService::Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 net::ProxyConfigService::ConfigAvailability
 ProxyConfigServiceImpl::GetLatestProxyConfig(net::ProxyConfig* config) {
   RegisterObserver();
 
-  if (pref_config_read_pending_)
-    return net::ProxyConfigService::CONFIG_PENDING;
-
   // Ask the base service if available.
   net::ProxyConfig system_config;
   ConfigAvailability system_availability =
       net::ProxyConfigService::CONFIG_UNSET;
   if (base_service_.get())
     system_availability = base_service_->GetLatestProxyConfig(&system_config);
 
   ProxyPrefs::ConfigState config_state;
   return PrefProxyConfigTrackerImpl::GetEffectiveProxyConfig(
       pref_config_state_, pref_config_, system_availability, system_config,
       false, &config_state, config);
 }
 
 void ProxyConfigServiceImpl::OnLazyPoll() {
   if (base_service_.get())
     base_service_->OnLazyPoll();
 }
 
 void ProxyConfigServiceImpl::UpdateProxyConfig(
     ProxyPrefs::ConfigState config_state,
     const net::ProxyConfig& config) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  pref_config_read_pending_ = false;
   pref_config_state_ = config_state;
   pref_config_ = config;
 
   if (!observers_.might_have_observers())
     return;
 
   // Evaluate the proxy configuration. If GetLatestProxyConfig returns
   // CONFIG_PENDING, we are using the system proxy service, but it doesn't have
   // a valid configuration yet. Once it is ready, OnProxyConfigChanged() will be
   // called and broadcast the proxy configuration.
   // Note: If a switch between a preference proxy configuration and the system
   // proxy configuration occurs an unnecessary notification might get send if
   // the two configurations agree. This case should be rare however, so we don't
   // handle that case specially.
   net::ProxyConfig new_config;
   ConfigAvailability availability = GetLatestProxyConfig(&new_config);
   if (availability != CONFIG_PENDING) {
     for (net::ProxyConfigService::Observer& observer : observers_)
       observer.OnProxyConfigChanged(new_config, availability);
   }
 }
 
 void ProxyConfigServiceImpl::OnProxyConfigChanged(
     const net::ProxyConfig& config,
     ConfigAvailability availability) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Check whether there is a proxy configuration defined by preferences. In
   // this case that proxy configuration takes precedence and the change event
-  // from the delegate proxy service can be disregarded.
+  // from the delegate proxy config service can be disregarded.
   if (!PrefProxyConfigTrackerImpl::PrefPrecedes(pref_config_state_)) {
     net::ProxyConfig actual_config;
     availability = GetLatestProxyConfig(&actual_config);
     for (net::ProxyConfigService::Observer& observer : observers_)
       observer.OnProxyConfigChanged(actual_config, availability);
   }
 }
 
 void ProxyConfigServiceImpl::RegisterObserver() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!registered_observer_ && base_service_.get()) {
     base_service_->AddObserver(this);
     registered_observer_ = true;
   }
 }
 
 //========================= PrefProxyConfigTrackerImpl =========================
 
 PrefProxyConfigTrackerImpl::PrefProxyConfigTrackerImpl(
     PrefService* pref_service,
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
     : pref_service_(pref_service),
       proxy_config_service_impl_(NULL),
       update_pending_(true),

[eroman, 2017/05/12 21:12:59]

Can you delete this variable?

Seems like the only places that cares about this is
PrefProxyConfigTrackerImpl::OnProxyConfigChanged() and it can check the nullity
of |proxy_config_service_impl_|.

[mmenke, 2017/05/12 21:29:57]

This is used in a subclass, chromeos::ProxyConfigServiceImpl, in a manner there
that implies its configuration updates can also fail.  I'm sufficiently nervous
about its state machinery and use in that class that I think fixing it is beyond
the scope of this CL.

I'll try and remove it in a followup CL.

[eroman, 2017/05/12 21:54:55]

No need to follow-up if it isn't simple.

Just figured if there was an opportunity to add small cleanups while the code
was paged into working memory, would be good.

[mmenke, 2017/05/12 21:57:58]

I think it's probably pretty simple, just didn't want to delve into yet another
weird proxy class.  I'll take a look, and if it seems at all complicated, I'll
skip it.

       io_task_runner_(io_task_runner) {
   config_state_ = ReadPrefConfig(pref_service_, &pref_config_);
   proxy_prefs_.Init(pref_service);
   proxy_prefs_.Add(proxy_config::prefs::kProxy,
                    base::Bind(&PrefProxyConfigTrackerImpl::OnProxyPrefChanged,
                               base::Unretained(this)));
 }
 
 PrefProxyConfigTrackerImpl::~PrefProxyConfigTrackerImpl() {
   DCHECK(pref_service_ == NULL);
 }
 
 std::unique_ptr<net::ProxyConfigService>
 PrefProxyConfigTrackerImpl::CreateTrackingProxyConfigService(

[eroman, 2017/05/12 21:12:59]

The interface for CreateTrackingProxyConfigService() doesn't document whether
this can be called multiple times. Presumably it can't.

Care to add a DCHECK() that proxy_config_service_impl_ is null here, and add a
comment to the interface?

[mmenke, 2017/05/12 21:29:57]

Done (Docs added to PrefProxyConfigTracker)

     std::unique_ptr<net::ProxyConfigService> base_service) {
-  proxy_config_service_impl_ =
-      new ProxyConfigServiceImpl(base_service.release());
+  proxy_config_service_impl_ = new ProxyConfigServiceImpl(
+      base_service.release(), config_state_, pref_config_);
   VLOG(1) << this << ": set chrome proxy config service to "
           << proxy_config_service_impl_;
-  if (proxy_config_service_impl_ && update_pending_)
-    OnProxyConfigChanged(config_state_, pref_config_);
+  update_pending_ = false;
 
   return std::unique_ptr<net::ProxyConfigService>(proxy_config_service_impl_);
 }
 
 void PrefProxyConfigTrackerImpl::DetachFromPrefService() {
   DCHECK(thread_checker_.CalledOnValidThread());
   // Stop notifications.
   proxy_prefs_.RemoveAll();
   pref_service_ = NULL;
   proxy_config_service_impl_ = NULL;
@@ skipping 184 matching lines @@
       (config_state_ != ProxyPrefs::CONFIG_UNSET &&
        !pref_config_.Equals(new_config))) {
     config_state_ = config_state;
     if (config_state_ != ProxyPrefs::CONFIG_UNSET)
       pref_config_ = new_config;
     update_pending_ = true;
   }
   if (update_pending_)
     OnProxyConfigChanged(config_state, new_config);
 }