Stop NetworkChangeNotifierMac from calling a virtual function before the constructor finishes.

NetworkChangeNotifierMac was posting a task to a separate thread in its constructor.  It's possible (although highly unlikely, but happens on buildbots) for the separate thread to start up and run the task (which invokes a virtual function) before the constructor completes (and, more importantly, the subtype's constructor initializes its vtable entries which are NULL / pure virtual in the base class).
The solution is to simply use a Delegate instead.  The Delegate should have been fully constructed (and thus, will not have the vtable initialization race) before being passed into NetworkChangeWatcherMac's constructor.  This also solves the stylistic issue of avoiding multiple inheritance, since NetworkConfigWatcherMac was not strictly an interface.

BUG=53138

Review URL: http://codereview.chromium.org/3344002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@58230 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 106 insertions, 47 deletions

diff --git a/net/base/network_change_notifier_mac.cc b/net/base/network_change_notifier_mac.cc
index 4c46a99..d07a15c 100644
--- a/net/base/network_change_notifier_mac.cc
+++ b/net/base/network_change_notifier_mac.cc
@@ -9,10 +9,14 @@
 
 namespace net {
 
-NetworkChangeNotifierMac::NetworkChangeNotifierMac() {}
+NetworkChangeNotifierMac::NetworkChangeNotifierMac()
+    : forwarder_(this),
+      config_watcher_(&forwarder_) {}
+NetworkChangeNotifierMac::~NetworkChangeNotifierMac() {}
 
 void NetworkChangeNotifierMac::SetDynamicStoreNotificationKeys(
     SCDynamicStoreRef store) {
+  // Called on notifier thread.
   scoped_cftyperef<CFMutableArrayRef> notification_keys(
       CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
   scoped_cftyperef<CFStringRef> key(SCDynamicStoreKeyCreateNetworkGlobalEntity(
diff --git a/net/base/network_change_notifier_mac.h b/net/base/network_change_notifier_mac.h
index 1d23524..33cf99c 100644
--- a/net/base/network_change_notifier_mac.h
+++ b/net/base/network_change_notifier_mac.h
@@ -14,17 +14,39 @@
 
 namespace net {
 
-class NetworkChangeNotifierMac : public NetworkConfigWatcherMac,
-                                 public NetworkChangeNotifier {
+class NetworkChangeNotifierMac: public NetworkChangeNotifier {
  public:
   NetworkChangeNotifierMac();
-
- protected:
-  // NetworkConfigWatcherMac implementation:
-  virtual void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store);
-  virtual void OnNetworkConfigChange(CFArrayRef changed_keys);
+  virtual ~NetworkChangeNotifierMac();
 
  private:
+  // Forwarder just exists to keep the NetworkConfigWatcherMac API out of
+  // NetworkChangeNotifierMac's public API.
+  class Forwarder : public NetworkConfigWatcherMac::Delegate {
+   public:
+    explicit Forwarder(NetworkChangeNotifierMac* net_config_watcher)
+        : net_config_watcher_(net_config_watcher_) {}
+
+    // NetworkConfigWatcherMac::Delegate implementation:
+    virtual void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store) {
+      net_config_watcher_->SetDynamicStoreNotificationKeys(store);
+    }
+    virtual void OnNetworkConfigChange(CFArrayRef changed_keys) {
+      net_config_watcher_->OnNetworkConfigChange(changed_keys);
+    }
+
+   private:
+    NetworkChangeNotifierMac* const net_config_watcher_;
+    DISALLOW_COPY_AND_ASSIGN(Forwarder);
+  };
+
+  // NetworkConfigWatcherMac::Delegate implementation:
+  void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store);
+  void OnNetworkConfigChange(CFArrayRef changed_keys);
+
+  Forwarder forwarder_;
+  const NetworkConfigWatcherMac config_watcher_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierMac);
 };
 
diff --git a/net/base/network_config_watcher_mac.cc b/net/base/network_config_watcher_mac.cc
index 8767dff..7a77b35 100644
--- a/net/base/network_config_watcher_mac.cc
+++ b/net/base/network_config_watcher_mac.cc
@@ -15,8 +15,23 @@ DISABLE_RUNNABLE_METHOD_REFCOUNT(net::NetworkConfigWatcherMac);
 
 namespace net {
 
-NetworkConfigWatcherMac::NetworkConfigWatcherMac()
-    : notifier_thread_(new base::Thread("NetworkConfigWatcher")) {
+namespace {
+
+// Called back by OS.  Calls OnNetworkConfigChange().
+void DynamicStoreCallback(SCDynamicStoreRef /* store */,
+                          CFArrayRef changed_keys,
+                          void* config_delegate) {
+  NetworkConfigWatcherMac::Delegate* net_config_delegate =
+      static_cast<NetworkConfigWatcherMac::Delegate*>(config_delegate);
+  net_config_delegate->OnNetworkConfigChange(changed_keys);
+}
+
+}  // namespace
+
+NetworkConfigWatcherMac::NetworkConfigWatcherMac(
+    Delegate* delegate)
+    : notifier_thread_(new base::Thread("NetworkConfigWatcher")),
+      delegate_(delegate) {
   // We create this notifier thread because the notification implementation
   // needs a thread with a CFRunLoop, and there's no guarantee that
   // MessageLoop::current() meets that criterion.
@@ -25,7 +40,8 @@ NetworkConfigWatcherMac::NetworkConfigWatcherMac()
   // TODO(willchan): Look to see if there's a better signal for when it's ok to
   // initialize this, rather than just delaying it by a fixed time.
   const int kNotifierThreadInitializationDelayMS = 1000;
-  notifier_thread_->message_loop()->PostDelayedTask(FROM_HERE,
+  notifier_thread_->message_loop()->PostDelayedTask(
+      FROM_HERE,
       NewRunnableMethod(this, &NetworkConfigWatcherMac::Init),
       kNotifierThreadInitializationDelayMS);
 }
@@ -37,16 +53,6 @@ NetworkConfigWatcherMac::~NetworkConfigWatcherMac() {
   DCHECK(run_loop_source_ == NULL);
 }
 
-// static
-void NetworkConfigWatcherMac::DynamicStoreCallback(
-    SCDynamicStoreRef /* store */,
-    CFArrayRef changed_keys,
-    void* config) {
-  NetworkConfigWatcherMac* net_config =
-      static_cast<NetworkConfigWatcherMac*>(config);
-  net_config->OnNetworkConfigChange(changed_keys);
-}
-
 void NetworkConfigWatcherMac::WillDestroyCurrentMessageLoop() {
   DCHECK(notifier_thread_ != NULL);
   // We can't check the notifier_thread_'s message_loop(), as it's now 0.
@@ -78,7 +84,7 @@ void NetworkConfigWatcherMac::Init() {
                      kCFRunLoopCommonModes);
 
   // Set up notifications for interface and IP address changes.
-  SetDynamicStoreNotificationKeys(store.get());
+  delegate_->SetDynamicStoreNotificationKeys(store.get());
 
   MessageLoop::current()->AddDestructionObserver(this);
 }
diff --git a/net/base/network_config_watcher_mac.h b/net/base/network_config_watcher_mac.h
index 71fb825..78f06a5 100644
--- a/net/base/network_config_watcher_mac.h
+++ b/net/base/network_config_watcher_mac.h
@@ -21,26 +21,26 @@ namespace net {
 // Base class for watching the Mac OS system network settings.
 class NetworkConfigWatcherMac : public MessageLoop::DestructionObserver {
  public:
-  NetworkConfigWatcherMac();
-
- protected:
+  // NOTE: The lifetime of Delegate is expected to exceed the lifetime of
+  // NetworkConfigWatcherMac.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+
+    // Called to register the notification keys on |store|.
+    // Implementors are expected to call SCDynamicStoreSetNotificationKeys().
+    // Will be called on the notifier thread.
+    virtual void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store) = 0;
+
+    // Called when one of the notification keys has changed.
+    // Will be called on the notifier thread.
+    virtual void OnNetworkConfigChange(CFArrayRef changed_keys) = 0;
+  };
+
+  explicit NetworkConfigWatcherMac(Delegate* delegate);
   virtual ~NetworkConfigWatcherMac();
 
-  // Called to register the notification keys on |store|.
-  // Implementors are expected to call SCDynamicStoreSetNotificationKeys().
-  // Will be called on the notifier thread.
-  virtual void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store) = 0;
-
-  // Called when one of the notification keys has changed.
-  // Will be called on the notifier thread.
-  virtual void OnNetworkConfigChange(CFArrayRef changed_keys) = 0;
-
  private:
-  // Called back by OS.  Calls OnNetworkConfigChange().
-  static void DynamicStoreCallback(SCDynamicStoreRef /* store */,
-                                   CFArrayRef changed_keys,
-                                   void* config);
-
   // MessageLoop::DestructionObserver:
   virtual void WillDestroyCurrentMessageLoop();
 
@@ -57,6 +57,8 @@ class NetworkConfigWatcherMac : public MessageLoop::DestructionObserver {
 
   scoped_cftyperef<CFRunLoopSourceRef> run_loop_source_;
 
+  Delegate* const delegate_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkConfigWatcherMac);
 };
 
diff --git a/net/proxy/proxy_config_service_mac.cc b/net/proxy/proxy_config_service_mac.cc
index 84a5da3..a197313 100644
--- a/net/proxy/proxy_config_service_mac.cc
+++ b/net/proxy/proxy_config_service_mac.cc
@@ -186,7 +186,9 @@ class ProxyConfigServiceMac::Helper
 };
 
 ProxyConfigServiceMac::ProxyConfigServiceMac(MessageLoop* io_loop)
-    : has_fetched_config_(false),
+    : forwarder_(this),
+      config_watcher_(&forwarder_),
+      has_fetched_config_(false),
       helper_(new Helper(this)),
       io_loop_(io_loop) {
   DCHECK(io_loop);
@@ -223,6 +225,8 @@ bool ProxyConfigServiceMac::GetLatestProxyConfig(ProxyConfig* config) {
 
 void ProxyConfigServiceMac::SetDynamicStoreNotificationKeys(
     SCDynamicStoreRef store) {
+  // Called on notifier thread.
+
   CFStringRef proxies_key = SCDynamicStoreKeyCreateProxies(NULL);
   CFArrayRef key_array = CFArrayCreate(
       NULL, (const void **)(&proxies_key), 1, &kCFTypeArrayCallBacks);
diff --git a/net/proxy/proxy_config_service_mac.h b/net/proxy/proxy_config_service_mac.h
index fd2d524..ede8c5e 100644
--- a/net/proxy/proxy_config_service_mac.h
+++ b/net/proxy/proxy_config_service_mac.h
@@ -15,8 +15,7 @@
 
 namespace net {
 
-class ProxyConfigServiceMac : public ProxyConfigService,
-                              public NetworkConfigWatcherMac {
+class ProxyConfigServiceMac : public ProxyConfigService {
  public:
   // Constructs a ProxyConfigService that watches the Mac OS system settings.
   // This instance is expected to be operated and deleted on |io_loop|
@@ -30,17 +29,39 @@ class ProxyConfigServiceMac : public ProxyConfigService,
   virtual void RemoveObserver(Observer* observer);
   virtual bool GetLatestProxyConfig(ProxyConfig* config);
 
- protected:
-  // NetworkConfigWatcherMac implementation:
-  virtual void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store);
-  virtual void OnNetworkConfigChange(CFArrayRef changed_keys);
-
  private:
   class Helper;
 
+  // Forwarder just exists to keep the NetworkConfigWatcherMac API out of
+  // ProxyConfigServiceMac's public API.
+  class Forwarder : public NetworkConfigWatcherMac::Delegate {
+   public:
+    explicit Forwarder(ProxyConfigServiceMac* net_config_watcher)
+        : net_config_watcher_(net_config_watcher_) {}
+
+    // NetworkConfigWatcherMac::Delegate implementation:
+    virtual void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store) {
+      net_config_watcher_->SetDynamicStoreNotificationKeys(store);
+    }
+    virtual void OnNetworkConfigChange(CFArrayRef changed_keys) {
+      net_config_watcher_->OnNetworkConfigChange(changed_keys);
+    }
+
+   private:
+    ProxyConfigServiceMac* const net_config_watcher_;
+    DISALLOW_COPY_AND_ASSIGN(Forwarder);
+  };
+
+  // NetworkConfigWatcherMac::Delegate implementation:
+  void SetDynamicStoreNotificationKeys(SCDynamicStoreRef store);
+  void OnNetworkConfigChange(CFArrayRef changed_keys);
+
   // Called when the proxy configuration has changed, to notify the observers.
   void OnProxyConfigChanged(const ProxyConfig& new_config);
 
+  Forwarder forwarder_;
+  const NetworkConfigWatcherMac config_watcher_;
+
   ObserverList<Observer> observers_;
 
   // Holds the last system proxy settings that we fetched.