Experimental workaround for CHECK in NetworkChangeNotifierWin.

Also adds a related UMA histogram.

BUG=69198
TEST=NetworkChangeNotifierWinTest.*, UMA metrics

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

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

6 files changed, 429 insertions, 17 deletions

diff --git a/net/base/network_change_notifier.cc b/net/base/network_change_notifier.cc
index 29b983e..00e6f0d 100644
--- a/net/base/network_change_notifier.cc
+++ b/net/base/network_change_notifier.cc
@@ -51,7 +51,10 @@ NetworkChangeNotifier* NetworkChangeNotifier::Create() {
     return g_network_change_notifier_factory->CreateInstance();
 
 #if defined(OS_WIN)
-  return new NetworkChangeNotifierWin();
+  NetworkChangeNotifierWin* network_change_notifier =
+      new NetworkChangeNotifierWin();
+  network_change_notifier->WatchForAddressChange();
+  return network_change_notifier;
 #elif defined(OS_CHROMEOS)
   // ChromeOS builds MUST use its own class factory.
   CHECK(false);
@@ -155,4 +158,15 @@ void NetworkChangeNotifier::NotifyObserversOfOnlineStateChange() {
   }
 }
 
+NetworkChangeNotifier::DisableForTest::DisableForTest()
+    : network_change_notifier_(g_network_change_notifier) {
+  DCHECK(g_network_change_notifier);
+  g_network_change_notifier = NULL;
+}
+
+NetworkChangeNotifier::DisableForTest::~DisableForTest() {
+  DCHECK(!g_network_change_notifier);
+  g_network_change_notifier = network_change_notifier_;
+}
+
 }  // namespace net
diff --git a/net/base/network_change_notifier.h b/net/base/network_change_notifier.h
index 39b9e05..85fff2c9 100644
--- a/net/base/network_change_notifier.h
+++ b/net/base/network_change_notifier.h
@@ -17,6 +17,8 @@ class NetworkChangeNotifierFactory;
 // NetworkChangeNotifier monitors the system for network changes, and notifies
 // registered observers of those events.  Observers may register on any thread,
 // and will be called back on the thread from which they registered.
+// NetworkChangeNotifiers are threadsafe, though they must be created and
+// destroyed on the same thread.
 class NET_EXPORT NetworkChangeNotifier {
  public:
   class NET_EXPORT IPAddressObserver {
@@ -134,6 +136,25 @@ class NET_EXPORT NetworkChangeNotifier {
   static void NotifyObserversOfDNSChange();
 
  private:
+  friend class NetworkChangeNotifierWinTest;
+
+  // Allows a second NetworkChangeNotifier to be created for unit testing, so
+  // the test suite can create a MockNetworkChangeNotifier, but platform
+  // specific NetworkChangeNotifiers can also be created for testing.  To use,
+  // create an DisableForTest object, and then create the new
+  // NetworkChangeNotifier object.  The NetworkChangeNotifier must be
+  // destroyed before the DisableForTest object, as its destruction will restore
+  // the original NetworkChangeNotifier.
+  class NET_EXPORT_PRIVATE DisableForTest {
+   public:
+    DisableForTest();
+    ~DisableForTest();
+
+   private:
+    // The original NetworkChangeNotifier to be restored on destruction.
+    NetworkChangeNotifier* network_change_notifier_;
+  };
+
   const scoped_refptr<ObserverListThreadSafe<IPAddressObserver> >
       ip_address_observer_list_;
   const scoped_refptr<ObserverListThreadSafe<OnlineStateObserver> >
diff --git a/net/base/network_change_notifier_win.cc b/net/base/network_change_notifier_win.cc
index 91d6ffb..590bfca 100644
--- a/net/base/network_change_notifier_win.cc
+++ b/net/base/network_change_notifier_win.cc
@@ -7,23 +7,36 @@
 #include <iphlpapi.h>
 #include <winsock2.h>
 
+#include "base/bind.h"
 #include "base/logging.h"
+#include "base/metrics/histogram.h"
 #include "base/time.h"
 #include "net/base/winsock_init.h"
 
 #pragma comment(lib, "iphlpapi.lib")
 
+namespace {
+
+// Time between NotifyAddrChange retries, on failure.
+const int kWatchForAddressChangeRetryIntervalMs = 500;
+
+}  // namespace
+
 namespace net {
 
-NetworkChangeNotifierWin::NetworkChangeNotifierWin() {
+NetworkChangeNotifierWin::NetworkChangeNotifierWin()
+    : is_watching_(false),
+      sequential_failures_(0),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
   memset(&addr_overlapped_, 0, sizeof addr_overlapped_);
   addr_overlapped_.hEvent = WSACreateEvent();
-  WatchForAddressChange();
 }
 
 NetworkChangeNotifierWin::~NetworkChangeNotifierWin() {
-  CancelIPChangeNotify(&addr_overlapped_);
-  addr_watcher_.StopWatching();
+  if (is_watching_) {
+    CancelIPChangeNotify(&addr_overlapped_);
+    addr_watcher_.StopWatching();
+  }
   WSACloseEvent(addr_overlapped_.hEvent);
 }
 
@@ -145,6 +158,18 @@ bool NetworkChangeNotifierWin::IsCurrentlyOffline() const {
 }
 
 void NetworkChangeNotifierWin::OnObjectSignaled(HANDLE object) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(is_watching_);
+  is_watching_ = false;
+
+  // Start watching for the next address change.
+  WatchForAddressChange();
+
+  NotifyObservers();
+}
+
+void NetworkChangeNotifierWin::NotifyObservers() {
+  DCHECK(CalledOnValidThread());
   NotifyObserversOfIPAddressChange();
 
   // Calling IsOffline() at this very moment is likely to give
@@ -155,16 +180,58 @@ void NetworkChangeNotifierWin::OnObjectSignaled(HANDLE object) {
   timer_.Stop();  // cancel any already waiting notification
   timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(1), this,
                &NetworkChangeNotifierWin::NotifyParentOfOnlineStateChange);
-
-  // Start watching for the next address change.
-  WatchForAddressChange();
 }
 
 void NetworkChangeNotifierWin::WatchForAddressChange() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!is_watching_);
+
+  // NotifyAddrChange occasionally fails with ERROR_OPEN_FAILED for unknown
+  // reasons.  More rarely, it's also been observed failing with
+  // ERROR_NO_SYSTEM_RESOURCES.  When either of these happens, we retry later.
+  if (!WatchForAddressChangeInternal()) {
+    ++sequential_failures_;
+
+    // TODO(mmenke):  If the UMA histograms indicate that this fixes
+    // http://crbug.com/69198, remove this histogram and consider reducing the
+    // retry interval.
+    if (sequential_failures_ == 100) {
+      UMA_HISTOGRAM_COUNTS_100("Net.NotifyAddrChangeFailures",
+                               sequential_failures_);
+    }
+
+    MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&NetworkChangeNotifierWin::WatchForAddressChange,
+                   weak_factory_.GetWeakPtr()),
+        kWatchForAddressChangeRetryIntervalMs);
+    return;
+  }
+
+  // Treat the transition from NotifyAddrChange failing to succeeding as a
+  // network change event, since network changes were not being observed in
+  // that interval.
+  if (sequential_failures_ > 0)
+    NotifyObservers();
+
+  if (sequential_failures_ < 100) {
+    UMA_HISTOGRAM_COUNTS_100("Net.NotifyAddrChangeFailures",
+                             sequential_failures_);
+  }
+
+  is_watching_ = true;
+  sequential_failures_ = 0;
+}
+
+bool NetworkChangeNotifierWin::WatchForAddressChangeInternal() {
+  DCHECK(CalledOnValidThread());
   HANDLE handle = NULL;
   DWORD ret = NotifyAddrChange(&handle, &addr_overlapped_);
-  CHECK_EQ(static_cast<DWORD>(ERROR_IO_PENDING), ret);
+  if (ret != ERROR_IO_PENDING)
+    return false;
+
   addr_watcher_.StartWatching(addr_overlapped_.hEvent, this);
+  return true;
 }
 
 void NetworkChangeNotifierWin::NotifyParentOfOnlineStateChange() {
diff --git a/net/base/network_change_notifier_win.h b/net/base/network_change_notifier_win.h
index 9c2457e..aee5304 100644
--- a/net/base/network_change_notifier_win.h
+++ b/net/base/network_change_notifier_win.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -9,37 +9,86 @@
 #include <windows.h>
 
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/non_thread_safe.h"
 #include "base/timer.h"
 #include "base/win/object_watcher.h"
+#include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 
 namespace net {
 
-class NetworkChangeNotifierWin : public NetworkChangeNotifier,
-                                 public base::win::ObjectWatcher::Delegate {
+// NetworkChangeNotifierWin inherits from NonThreadSafe, as all its internal
+// notification code must be called on the thread it is created and destroyed
+// on.  All the NetworkChangeNotifier methods it implements are threadsafe.
+class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
+    : public NetworkChangeNotifier,
+      public base::win::ObjectWatcher::Delegate,
+      NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   NetworkChangeNotifierWin();
 
- private:
+  // Begins listening for a single subsequent address change.  If it fails to
+  // start watching, it retries on a timer.  Must be called only once, on the
+  // thread |this| was created on.  This cannot be called in the constructor, as
+  // WatchForAddressChangeInternal is mocked out in unit tests.
+  // TODO(mmenke): Consider making this function a part of the
+  //               NetworkChangeNotifier interface, so other subclasses can be
+  //               unit tested in similar fashion, as needed.
+  void WatchForAddressChange();
+
+ protected:
   virtual ~NetworkChangeNotifierWin();
 
+  // For unit tests only.
+  bool is_watching() { return is_watching_; }
+  void set_is_watching(bool is_watching) { is_watching_ = is_watching; }
+  int sequential_failures() { return sequential_failures_; }
+
+ private:
+  friend class NetworkChangeNotifierWinTest;
+
   // NetworkChangeNotifier methods:
-  virtual bool IsCurrentlyOffline() const;
+  virtual bool IsCurrentlyOffline() const OVERRIDE;
 
   // ObjectWatcher::Delegate methods:
-  virtual void OnObjectSignaled(HANDLE object);
+  // Must only be called on the thread |this| was created on.
+  virtual void OnObjectSignaled(HANDLE object) OVERRIDE;
 
-  // Begins listening for a single subsequent address change.
-  void WatchForAddressChange();
+  // Notifies IP address change observers of a change immediately, and notifies
+  // network state change observers on a delay.  Must only be called on the
+  // thread |this| was created on.
+  void NotifyObservers();
 
   // Forwards online state notifications to parent class.
   void NotifyParentOfOnlineStateChange();
 
+  // Tries to start listening for a single subsequent address change.  Returns
+  // false on failure.  The caller is responsible for updating |is_watching_|.
+  // Virtual for unit tests.  Must only be called on the thread |this| was
+  // created on.
+  virtual bool WatchForAddressChangeInternal();
+
+  // All member variables may only be accessed on the thread |this| was created
+  // on.
+
+  // False when not currently watching for network change events.  This only
+  // happens on initialization and when WatchForAddressChangeInternal fails and
+  // there is a pending task to try again.  Needed for safe cleanup.
+  bool is_watching_;
+
   base::win::ObjectWatcher addr_watcher_;
   OVERLAPPED addr_overlapped_;
 
   base::OneShotTimer<NetworkChangeNotifierWin> timer_;
 
+  // Number of times WatchForAddressChange has failed in a row.
+  int sequential_failures_;
+
+  // Used for calling WatchForAddressChange again on failure.
+  base::WeakPtrFactory<NetworkChangeNotifierWin> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierWin);
 };
 
diff --git a/net/base/network_change_notifier_win_unittest.cc b/net/base/network_change_notifier_win_unittest.cc
new file mode 100644
index 0000000..170755a
--- /dev/null
+++ b/net/base/network_change_notifier_win_unittest.cc
@@ -0,0 +1,260 @@
+// 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 "base/message_loop.h"
+#include "net/base/network_change_notifier.h"
+#include "net/base/network_change_notifier_factory.h"
+#include "net/base/network_change_notifier_win.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::AtLeast;
+using ::testing::Invoke;
+using ::testing::Return;
+using ::testing::StrictMock;
+
+namespace net {
+
+namespace {
+
+// Subclass of NetworkChangeNotifierWin that overrides functions so that no
+// Windows API networking functions are ever called.
+class TestNetworkChangeNotifierWin : public NetworkChangeNotifierWin {
+ public:
+  TestNetworkChangeNotifierWin() {}
+
+  virtual ~TestNetworkChangeNotifierWin() {
+    // This is needed so we don't try to stop watching for IP address changes,
+    // as we never actually started.
+    set_is_watching(false);
+  }
+
+  // From NetworkChangeNotifier.
+  virtual bool IsCurrentlyOffline() const OVERRIDE {
+    return false;
+  }
+
+  // From NetworkChangeNotifierWin.
+  MOCK_METHOD0(WatchForAddressChangeInternal, bool());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestNetworkChangeNotifierWin);
+};
+
+class TestIPAddressObserver
+    : public net::NetworkChangeNotifier::IPAddressObserver {
+ public:
+  TestIPAddressObserver() {
+    NetworkChangeNotifier::AddIPAddressObserver(this);
+  }
+
+  ~TestIPAddressObserver() {
+    NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  }
+
+  MOCK_METHOD0(OnIPAddressChanged, void());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestIPAddressObserver);
+};
+
+bool ExitMessageLoopAndReturnFalse() {
+  MessageLoop::current()->Quit();
+  return false;
+}
+
+}  // namespace
+
+class NetworkChangeNotifierWinTest : public testing::Test {
+ public:
+  // Calls WatchForAddressChange, and simulates a WatchForAddressChangeInternal
+  // success.  Expects that |network_change_notifier_| has just been created, so
+  // it's not watching anything yet, and there have been no previous
+  // WatchForAddressChangeInternal failures.
+  void StartWatchingAndSucceed() {
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+
+    EXPECT_CALL(test_ip_address_observer_, OnIPAddressChanged()).Times(0);
+    EXPECT_CALL(network_change_notifier_, WatchForAddressChangeInternal())
+        .Times(1)
+        .WillOnce(Return(true));
+
+    network_change_notifier_.WatchForAddressChange();
+
+    EXPECT_TRUE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+
+    // If a task to notify observers of the IP address change event was
+    // incorrectly posted, make sure it gets run to trigger a failure.
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Calls WatchForAddressChange, and simulates a WatchForAddressChangeInternal
+  // failure.
+  void StartWatchingAndFail() {
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+
+    EXPECT_CALL(test_ip_address_observer_, OnIPAddressChanged()).Times(0);
+    EXPECT_CALL(network_change_notifier_, WatchForAddressChangeInternal())
+        // Due to an expected race, it's theoretically possible for more than
+        // one call to occur, though unlikely.
+        .Times(AtLeast(1))
+        .WillRepeatedly(Return(false));
+
+    network_change_notifier_.WatchForAddressChange();
+
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_LT(0, network_change_notifier_.sequential_failures());
+
+    // If a task to notify observers of the IP address change event was
+    // incorrectly posted, make sure it gets run.
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Simulates a network change event, resulting in a call to OnObjectSignaled.
+  // The resulting call to WatchForAddressChangeInternal then succeeds.
+  void SignalAndSucceed() {
+    EXPECT_TRUE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+
+    EXPECT_CALL(test_ip_address_observer_, OnIPAddressChanged()).Times(1);
+    EXPECT_CALL(network_change_notifier_, WatchForAddressChangeInternal())
+        .Times(1)
+        .WillOnce(Return(true));
+
+    network_change_notifier_.OnObjectSignaled(INVALID_HANDLE_VALUE);
+
+    EXPECT_TRUE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+
+    // Run the task to notify observers of the IP address change event.
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Simulates a network change event, resulting in a call to OnObjectSignaled.
+  // The resulting call to WatchForAddressChangeInternal then fails.
+  void SignalAndFail() {
+    EXPECT_TRUE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+
+    EXPECT_CALL(test_ip_address_observer_, OnIPAddressChanged()).Times(1);
+    EXPECT_CALL(network_change_notifier_, WatchForAddressChangeInternal())
+        // Due to an expected race, it's theoretically possible for more than
+        // one call to occur, though unlikely.
+        .Times(AtLeast(1))
+        .WillRepeatedly(Return(false));
+
+    network_change_notifier_.OnObjectSignaled(INVALID_HANDLE_VALUE);
+
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_LT(0, network_change_notifier_.sequential_failures());
+
+    // Run the task to notify observers of the IP address change event.
+    MessageLoop::current()->RunAllPending();
+  }
+
+  // Runs the message loop until WatchForAddressChange is called again, as a
+  // result of the already posted task after a WatchForAddressChangeInternal
+  // failure.  Simulates a success on the resulting call to
+  // WatchForAddressChangeInternal.
+  void RetryAndSucceed() {
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_LT(0, network_change_notifier_.sequential_failures());
+
+    EXPECT_CALL(test_ip_address_observer_, OnIPAddressChanged())
+        .Times(1)
+        .WillOnce(Invoke(MessageLoop::current(), &MessageLoop::Quit));
+    EXPECT_CALL(network_change_notifier_, WatchForAddressChangeInternal())
+        .Times(1)
+        .WillOnce(Return(true));
+
+    MessageLoop::current()->Run();
+
+    EXPECT_TRUE(network_change_notifier_.is_watching());
+    EXPECT_EQ(0, network_change_notifier_.sequential_failures());
+  }
+
+  // Runs the message loop until WatchForAddressChange is called again, as a
+  // result of the already posted task after a WatchForAddressChangeInternal
+  // failure.  Simulates a failure on the resulting call to
+  // WatchForAddressChangeInternal.
+  void RetryAndFail() {
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_LT(0, network_change_notifier_.sequential_failures());
+
+    int initial_sequential_failures =
+        network_change_notifier_.sequential_failures();
+
+    EXPECT_CALL(test_ip_address_observer_, OnIPAddressChanged()).Times(0);
+    EXPECT_CALL(network_change_notifier_, WatchForAddressChangeInternal())
+        // Due to an expected race, it's theoretically possible for more than
+        // one call to occur, though unlikely.
+        .Times(AtLeast(1))
+        .WillRepeatedly(Invoke(ExitMessageLoopAndReturnFalse));
+
+    MessageLoop::current()->Run();
+
+    EXPECT_FALSE(network_change_notifier_.is_watching());
+    EXPECT_LT(initial_sequential_failures,
+              network_change_notifier_.sequential_failures());
+
+    // If a task to notify observers of the IP address change event was
+    // incorrectly posted, make sure it gets run.
+    MessageLoop::current()->RunAllPending();
+  }
+
+ private:
+  // Note that the order of declaration here is important.
+
+  // Allows creating a new NetworkChangeNotifier.  Must be created before
+  // |network_change_notifier_| and destroyed after it to avoid DCHECK failures.
+  NetworkChangeNotifier::DisableForTest disable_for_test_;
+
+  StrictMock<TestNetworkChangeNotifierWin> network_change_notifier_;
+
+  // Must be created after |network_change_notifier_|, so it can add itself as
+  // an IPAddressObserver.
+  StrictMock<TestIPAddressObserver> test_ip_address_observer_;
+};
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinBasic) {
+  StartWatchingAndSucceed();
+}
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinFailStart) {
+  StartWatchingAndFail();
+}
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinFailStartOnce) {
+  StartWatchingAndFail();
+  RetryAndSucceed();
+}
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinFailStartTwice) {
+  StartWatchingAndFail();
+  RetryAndFail();
+  RetryAndSucceed();
+}
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinSignal) {
+  StartWatchingAndSucceed();
+  SignalAndSucceed();
+}
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinFailSignalOnce) {
+  StartWatchingAndSucceed();
+  SignalAndFail();
+  RetryAndSucceed();
+}
+
+TEST_F(NetworkChangeNotifierWinTest, NetChangeWinFailSignalTwice) {
+  StartWatchingAndSucceed();
+  SignalAndFail();
+  RetryAndFail();
+  RetryAndSucceed();
+}
+
+}  // namespace net
diff --git a/net/net.gyp b/net/net.gyp
index 0bc2862..fdde802 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -949,6 +949,7 @@
         'base/net_log_unittest.cc',
         'base/net_log_unittest.h',
         'base/net_util_unittest.cc',
+        'base/network_change_notifier_win_unittest.cc',
         'base/origin_bound_cert_service_unittest.cc',
         'base/pem_tokenizer_unittest.cc',
         'base/registry_controlled_domain_unittest.cc',