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

ipc/ipc_sync_channel_unittest.cc

 // Copyright (c) 2012 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.
 //
 // Unit test for SyncChannel.
 
 #include "ipc/ipc_sync_channel.h"
 
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/logging.h"
 #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"
 #include "ipc/ipc_listener.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_sender.h"
 #include "ipc/ipc_sync_message_filter.h"
 #include "ipc/ipc_sync_message_unittest.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using base::WaitableEvent;
 
 namespace IPC {
 
 namespace {
 
 // Base class for a "process" with listener and IPC threads.
 class Worker : public Listener, public Sender {
  public:
   // Will create a channel without a name.
   Worker(Channel::Mode mode, const std::string& thread_name)
       : done_(new WaitableEvent(false, false)),
         channel_created_(new WaitableEvent(false, false)),
         mode_(mode),
         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.
   Worker(const std::string& channel_name, Channel::Mode mode)
       : done_(new WaitableEvent(false, false)),
         channel_created_(new WaitableEvent(false, false)),
         channel_name_(channel_name),
         mode_(mode),
         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() { }
   bool Send(Message* msg) { return channel_->Send(msg); }
   bool SendWithTimeout(Message* msg, int timeout_ms) {
     return channel_->SendWithTimeout(msg, timeout_ms);
   }
   void WaitForChannelCreation() { channel_created_->Wait(); }
   void CloseChannel() {
     DCHECK(MessageLoop::current() == ListenerThread()->message_loop());
     channel_->Close();
   }
   void Start() {
     StartThread(&listener_thread_, MessageLoop::TYPE_DEFAULT);
     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;
   }
   bool SendAnswerToLife(bool pump, int timeout, bool succeed) {
     int answer = 0;
     SyncMessage* msg = new SyncChannelTestMsg_AnswerToLife(&answer);
     if (pump)
       msg->EnableMessagePumping();
     bool result = SendWithTimeout(msg, timeout);
@@ skipping 118 matching lines @@
   scoped_ptr<WaitableEvent> channel_created_;
   std::string channel_name_;
   Channel::Mode mode_;
   scoped_ptr<SyncChannel> channel_;
   base::Thread ipc_thread_;
   base::Thread listener_thread_;
   base::Thread* overrided_thread_;
 
   base::WaitableEvent shutdown_event_;
 
+  bool is_shutdown_;
+
   DISALLOW_COPY_AND_ASSIGN(Worker);
 };
 
 
 // Starts the test with the given workers.  This function deletes the workers
 // when it's done.
 void RunTest(std::vector<Worker*> workers) {
   // First we create the workers that are channel servers, or else the other
   // workers' channel initialization might fail because the pipe isn't created..
   for (size_t i = 0; i < workers.size(); ++i) {
     if (workers[i]->mode() & Channel::MODE_SERVER_FLAG) {
       workers[i]->Start();
       workers[i]->WaitForChannelCreation();
     }
   }
 
   // now create the clients
   for (size_t i = 0; i < workers.size(); ++i) {
     if (workers[i]->mode() & Channel::MODE_CLIENT_FLAG)
       workers[i]->Start();
   }
 
   // wait for all the workers to finish
   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
 
 class IPCSyncChannelTest : public testing::Test {
  private:
   MessageLoop message_loop_;
 };
 
 //-----------------------------------------------------------------------------
@@ skipping 911 matching lines @@
   ServerSendAfterClose server;
   server.Start();
 
   server.done_event()->Wait();
   server.done_event()->Reset();
 
   server.SendDummy();
   server.done_event()->Wait();
 
   EXPECT_FALSE(server.send_result());
+
+  server.Shutdown();
 }
 
 //-----------------------------------------------------------------------------
 
 namespace {
 
 class RestrictedDispatchServer : public Worker {
  public:
   RestrictedDispatchServer(WaitableEvent* sent_ping_event,
                            WaitableEvent* wait_event)
@@ skipping 749 matching lines @@
 
 }  // namespace
 
 // Windows needs to send an out-of-band secret to verify the client end of the
 // channel. Test that we still connect correctly in that case.
 TEST_F(IPCSyncChannelTest, Verified) {
   Verified();
 }
 
 }  // namespace IPC