Fixed destruction race bug in FakeNetworkChangeNotifierThread.

Added unit tests for the above bug.

BUG=none
TEST=unit tests

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

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

3 files changed, 60 insertions, 10 deletions

diff --git a/chrome/common/net/fake_network_change_notifier_thread.cc b/chrome/common/net/fake_network_change_notifier_thread.cc
index 63930ac..6c22d48 100644
--- a/chrome/common/net/fake_network_change_notifier_thread.cc
+++ b/chrome/common/net/fake_network_change_notifier_thread.cc
@@ -30,9 +30,9 @@ FakeNetworkChangeNotifierThread::~FakeNetworkChangeNotifierThread() {
 }
 
 void FakeNetworkChangeNotifierThread::Start() {
+  network_change_notifier_.reset(new net::MockNetworkChangeNotifier());
   CHECK(thread_.Start());
   thread_blocker_.reset(new ThreadBlocker(&thread_));
-  network_change_notifier_.reset(new net::MockNetworkChangeNotifier());
   thread_blocker_->Block();
 }
 
@@ -43,9 +43,9 @@ void FakeNetworkChangeNotifierThread::Pump() {
 
 void FakeNetworkChangeNotifierThread::Stop() {
   thread_blocker_->Unblock();
-  network_change_notifier_.reset();
   thread_blocker_.reset();
   thread_.Stop();
+  network_change_notifier_.reset();
 }
 
 void FakeNetworkChangeNotifierThread::NotifyIPAddressChange() {
diff --git a/chrome/common/net/fake_network_change_notifier_thread.h b/chrome/common/net/fake_network_change_notifier_thread.h
index 5a594f1..907a5b1 100644
--- a/chrome/common/net/fake_network_change_notifier_thread.h
+++ b/chrome/common/net/fake_network_change_notifier_thread.h
@@ -50,11 +50,14 @@ class FakeNetworkChangeNotifierThread : public NetworkChangeNotifierThread {
   virtual net::NetworkChangeNotifier* GetNetworkChangeNotifier() const;
 
  private:
+  // Used in unit tests.
+  friend class FakeNetworkChangeNotifierThreadDestructionObserver;
+
   void NotifyIPAddressChangeOnSourceThread();
 
+  scoped_ptr<net::MockNetworkChangeNotifier> network_change_notifier_;
   base::Thread thread_;
   scoped_ptr<ThreadBlocker> thread_blocker_;
-  scoped_ptr<net::MockNetworkChangeNotifier> network_change_notifier_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeNetworkChangeNotifierThread);
 };
diff --git a/chrome/common/net/fake_network_change_notifier_thread_unittest.cc b/chrome/common/net/fake_network_change_notifier_thread_unittest.cc
index 0cfb741..3902888 100644
--- a/chrome/common/net/fake_network_change_notifier_thread_unittest.cc
+++ b/chrome/common/net/fake_network_change_notifier_thread_unittest.cc
@@ -57,13 +57,52 @@ class FlagToggler : public net::NetworkChangeNotifier::Observer {
   DISALLOW_COPY_AND_ASSIGN(FlagToggler);
 };
 
+// Utility class that sanity-checks a
+// FakeNetworkChangeNotifierThread's member variables right before its
+// message loop gets destroyed (used in DestructionRace test).
+class FakeNetworkChangeNotifierThreadDestructionObserver
+    : public MessageLoop::DestructionObserver {
+ public:
+  explicit FakeNetworkChangeNotifierThreadDestructionObserver(
+      const FakeNetworkChangeNotifierThread& thread)
+      : thread_(thread) {}
+
+  virtual ~FakeNetworkChangeNotifierThreadDestructionObserver() {}
+
+  virtual void WillDestroyCurrentMessageLoop() {
+    EXPECT_FALSE(thread_.thread_blocker_.get());
+    // We can't use
+    // FakeNetworkChangeNotifierThread::GetNetworkChangeNotifier() as
+    // it would CHECK-fail on the current thread's message loop being
+    // NULL.
+    EXPECT_TRUE(thread_.network_change_notifier_.get());
+    delete this;
+  }
+
+ private:
+  const FakeNetworkChangeNotifierThread& thread_;
+
+  DISALLOW_COPY_AND_ASSIGN(
+      FakeNetworkChangeNotifierThreadDestructionObserver);
+};
+
+// Utility function to add the
+// FakeNetworkChangeNotifierThreadDestructionObserver from the
+// FakeNetworkChangeNotifierThread's thread.
+void AddFakeNetworkChangeNotifierThreadDestructionObserver(
+    const FakeNetworkChangeNotifierThread* thread) {
+  CHECK_EQ(MessageLoop::current(), thread->GetMessageLoop());
+  thread->GetMessageLoop()->AddDestructionObserver(
+      new FakeNetworkChangeNotifierThreadDestructionObserver(*thread));
+}
+
 namespace {
 
-class FakeNetworkChangeNotifierTest : public testing::Test {
+class FakeNetworkChangeNotifierThreadTest : public testing::Test {
  protected:
-  FakeNetworkChangeNotifierTest() {}
+  FakeNetworkChangeNotifierThreadTest() {}
 
-  virtual ~FakeNetworkChangeNotifierTest() {}
+  virtual ~FakeNetworkChangeNotifierThreadTest() {}
 
   virtual void SetUp() {
     thread_.Start();
@@ -77,10 +116,10 @@ class FakeNetworkChangeNotifierTest : public testing::Test {
   FlagToggler flag_toggler_;
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(FakeNetworkChangeNotifierTest);
+  DISALLOW_COPY_AND_ASSIGN(FakeNetworkChangeNotifierThreadTest);
 };
 
-TEST_F(FakeNetworkChangeNotifierTest, Pump) {
+TEST_F(FakeNetworkChangeNotifierThreadTest, Pump) {
   thread_.GetMessageLoop()->PostTask(
       FROM_HERE, NewRunnableMethod(&flag_toggler_, &FlagToggler::ToggleFlag));
   EXPECT_FALSE(flag_toggler_.flag());
@@ -88,7 +127,7 @@ TEST_F(FakeNetworkChangeNotifierTest, Pump) {
   EXPECT_TRUE(flag_toggler_.flag());
 }
 
-TEST_F(FakeNetworkChangeNotifierTest, Basic) {
+TEST_F(FakeNetworkChangeNotifierThreadTest, Basic) {
   thread_.GetMessageLoop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(&flag_toggler_, &FlagToggler::Observe, &thread_));
@@ -101,7 +140,7 @@ TEST_F(FakeNetworkChangeNotifierTest, Basic) {
   EXPECT_TRUE(flag_toggler_.flag());
 }
 
-TEST_F(FakeNetworkChangeNotifierTest, Multiple) {
+TEST_F(FakeNetworkChangeNotifierThreadTest, Multiple) {
   FlagToggler observer;
   thread_.GetMessageLoop()->PostTask(
       FROM_HERE,
@@ -116,6 +155,14 @@ TEST_F(FakeNetworkChangeNotifierTest, Multiple) {
   EXPECT_FALSE(flag_toggler_.flag());
 }
 
+TEST_F(FakeNetworkChangeNotifierThreadTest, DestructionRace) {
+  thread_.GetMessageLoop()->PostTask(
+      FROM_HERE,
+      NewRunnableFunction(
+          &AddFakeNetworkChangeNotifierThreadDestructionObserver,
+          &thread_));
+}
+
 }  // namespace
 
 }  // namespace chrome_common_net