Fix shutdown race in IPCSyncChannelTest and get rid of "suppressions"/annotations.

Shut down explicitly, rather than doing it in a virtual destructor, which leads
to a vtable race.

(Possibly also related is http://crbug.com/70075, but I haven't been able to
reproduce, so I'll try to re-enable that separately.)

BUG=25841
TEST=ipc_tests + TSAN still happy


Review URL: https://chromiumcodereview.appspot.com/11761038

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

2 files changed, 30 insertions, 26 deletions

diff --git a/ipc/ipc.gyp b/ipc/ipc.gyp
index 6e9bd9e..678d166 100644
--- a/ipc/ipc.gyp
+++ b/ipc/ipc.gyp
@@ -18,6 +18,7 @@
       },
       'dependencies': [
         '../base/base.gyp:base',
+        # TODO(vtl): Needed for base/lazy_instance.h, which is suspect.
         '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
       ],
       # TODO(gregoryd): direct_dependent_settings should be shared with the
@@ -37,7 +38,6 @@
         '../base/base.gyp:base',
         '../base/base.gyp:base_i18n',
         '../base/base.gyp:test_support_base',
-        '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
         '../testing/gtest.gyp:gtest',
       ],
       'include_dirs': [
@@ -106,6 +106,7 @@
           },
           'dependencies': [
             '../base/base.gyp:base_nacl_win64',
+            # TODO(vtl): Needed for base/lazy_instance.h, which is suspect.
             '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations_win64',
           ],
           # TODO(gregoryd): direct_dependent_settings should be shared with the
diff --git a/ipc/ipc_sync_channel_unittest.cc b/ipc/ipc_sync_channel_unittest.cc
index c31565a..a4e9bd8 100644
--- a/ipc/ipc_sync_channel_unittest.cc
+++ b/ipc/ipc_sync_channel_unittest.cc
@@ -15,9 +15,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/process_util.h"
-#include "base/stl_util.h"
 #include "base/string_util.h"
-#include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
 #include "base/synchronization/waitable_event.h"
@@ -45,12 +43,8 @@ class Worker : public Listener, public Sender {
         ipc_thread_((thread_name + "_ipc").c_str()),
         listener_thread_((thread_name + "_listener").c_str()),
         overrided_thread_(NULL),
-        shutdown_event_(true, false) {
-    // The data race on vfptr is real but is very hard
-    // to suppress using standard Valgrind mechanism (suppressions).
-    // We have to use ANNOTATE_BENIGN_RACE to hide the reports and
-    // make ThreadSanitizer bots green.
-    ANNOTATE_BENIGN_RACE(this, "Race on vfptr, http://crbug.com/25841");
+        shutdown_event_(true, false),
+        is_shutdown_(false) {
   }
 
   // Will create a named channel and use this name for the threads' name.
@@ -62,25 +56,13 @@ class Worker : public Listener, public Sender {
         ipc_thread_((channel_name + "_ipc").c_str()),
         listener_thread_((channel_name + "_listener").c_str()),
         overrided_thread_(NULL),
-        shutdown_event_(true, false) {
-    // The data race on vfptr is real but is very hard
-    // to suppress using standard Valgrind mechanism (suppressions).
-    // We have to use ANNOTATE_BENIGN_RACE to hide the reports and
-    // make ThreadSanitizer bots green.
-    ANNOTATE_BENIGN_RACE(this, "Race on vfptr, http://crbug.com/25841");
+        shutdown_event_(true, false),
+        is_shutdown_(false) {
   }
 
-  // The IPC thread needs to outlive SyncChannel, so force the correct order of
-  // destruction.
   virtual ~Worker() {
-    WaitableEvent listener_done(false, false), ipc_done(false, false);
-    ListenerThread()->message_loop()->PostTask(
-        FROM_HERE, base::Bind(&Worker::OnListenerThreadShutdown1, this,
-                              &listener_done, &ipc_done));
-    listener_done.Wait();
-    ipc_done.Wait();
-    ipc_thread_.Stop();
-    listener_thread_.Stop();
+    // Shutdown() must be called before destruction.
+    CHECK(is_shutdown_);
   }
   void AddRef() { }
   void Release() { }
@@ -98,6 +80,20 @@ class Worker : public Listener, public Sender {
     ListenerThread()->message_loop()->PostTask(
         FROM_HERE, base::Bind(&Worker::OnStart, this));
   }
+  void Shutdown() {
+    // The IPC thread needs to outlive SyncChannel. We can't do this in
+    // ~Worker(), since that'll reset the vtable pointer (to Worker's), which
+    // may result in a race conditions. See http://crbug.com/25841.
+    WaitableEvent listener_done(false, false), ipc_done(false, false);
+    ListenerThread()->message_loop()->PostTask(
+        FROM_HERE, base::Bind(&Worker::OnListenerThreadShutdown1, this,
+                              &listener_done, &ipc_done));
+    listener_done.Wait();
+    ipc_done.Wait();
+    ipc_thread_.Stop();
+    listener_thread_.Stop();
+    is_shutdown_ = true;
+  }
   void OverrideThread(base::Thread* overrided_thread) {
     DCHECK(overrided_thread_ == NULL);
     overrided_thread_ = overrided_thread;
@@ -236,6 +232,8 @@ class Worker : public Listener, public Sender {
 
   base::WaitableEvent shutdown_event_;
 
+  bool is_shutdown_;
+
   DISALLOW_COPY_AND_ASSIGN(Worker);
 };
 
@@ -262,7 +260,10 @@ void RunTest(std::vector<Worker*> workers) {
   for (size_t i = 0; i < workers.size(); ++i)
     workers[i]->done_event()->Wait();
 
-  STLDeleteContainerPointers(workers.begin(), workers.end());
+  for (size_t i = 0; i < workers.size(); ++i) {
+    workers[i]->Shutdown();
+    delete workers[i];
+  }
 }
 
 }  // namespace
@@ -1194,6 +1195,8 @@ TEST_F(IPCSyncChannelTest, SendAfterClose) {
   server.done_event()->Wait();
 
   EXPECT_FALSE(server.send_result());
+
+  server.Shutdown();
 }
 
 //-----------------------------------------------------------------------------