Fixes up the race condition that exists with deleting a file vs watching it.

The tests created a file and attempted to hook a watcher on it on a separate thread
but it was deleting it before the thread had a chance to watch it.

BUG=77217, 77064, 69641, 76987
TEST=BUILD

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79307

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

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

4 files changed, 49 insertions, 41 deletions

diff --git a/chrome/common/service_process_util_posix.cc b/chrome/common/service_process_util_posix.cc
index 5592edf..83a1edd 100644
--- a/chrome/common/service_process_util_posix.cc
+++ b/chrome/common/service_process_util_posix.cc
@@ -7,6 +7,7 @@
 #include "base/basictypes.h"
 #include "base/eintr_wrapper.h"
 #include "base/message_loop_proxy.h"
+#include "base/synchronization/waitable_event.h"
 
 namespace {
 int g_signal_socket = -1;
@@ -58,11 +59,18 @@ ServiceProcessState::StateData::StateData() : set_action_(false) {
   memset(&old_action_, 0, sizeof(old_action_));
 }
 
-void ServiceProcessState::StateData::SignalReady() {
+void ServiceProcessState::StateData::SignalReady(base::WaitableEvent* signal,
+                                                 bool* success) {
   CHECK_EQ(g_signal_socket, -1);
-  CHECK(MessageLoopForIO::current()->WatchFileDescriptor(
+  CHECK(!signal->IsSignaled());
+   *success = MessageLoopForIO::current()->WatchFileDescriptor(
       sockets_[0], true, MessageLoopForIO::WATCH_READ,
-      &watcher_, shut_down_monitor_.get()));
+      &watcher_, shut_down_monitor_.get());
+  if (!*success) {
+    LOG(ERROR) << "WatchFileDescriptor";
+    signal->Signal();
+    return;
+  }
   g_signal_socket = sockets_[1];
 
   // Set up signal handler for SIGTERM.
@@ -70,23 +78,31 @@ void ServiceProcessState::StateData::SignalReady() {
   action.sa_sigaction = SigTermHandler;
   sigemptyset(&action.sa_mask);
   action.sa_flags = SA_SIGINFO;
-  if (sigaction(SIGTERM, &action, &old_action_) == 0) {
-    // If the old_action is not default, somebody else has installed a
-    // a competing handler. Our handler is going to override it so it
-    // won't be called. If this occurs it needs to be fixed.
-    DCHECK_EQ(old_action_.sa_handler, SIG_DFL);
-    set_action_ = true;
+  *success = sigaction(SIGTERM, &action, &old_action_) == 0;
+  if (!*success) {
+    PLOG(ERROR) << "sigaction";
+    signal->Signal();
+    return;
+  }
+
+  // If the old_action is not default, somebody else has installed a
+  // a competing handler. Our handler is going to override it so it
+  // won't be called. If this occurs it needs to be fixed.
+  DCHECK_EQ(old_action_.sa_handler, SIG_DFL);
+  set_action_ = true;
+
 #if defined(OS_LINUX)
-    initializing_lock_.reset();
+  initializing_lock_.reset();
 #endif  // OS_LINUX
 #if defined(OS_MACOSX)
-    if (!WatchExecutable()) {
-      LOG(ERROR) << "WatchExecutable";
-    }
-#endif  // OS_MACOSX
-  } else {
-    PLOG(ERROR) << "sigaction";
+  *success = WatchExecutable();
+  if (!*success) {
+    LOG(ERROR) << "WatchExecutable";
+    signal->Signal();
+    return;
   }
+#endif  // OS_MACOSX
+  signal->Signal();
 }
 
 ServiceProcessState::StateData::~StateData() {
@@ -136,9 +152,15 @@ bool ServiceProcessState::SignalReady(
     PLOG(ERROR) << "pipe";
     return false;
   }
+  base::WaitableEvent signal_ready(true, false);
+  bool success = false;
+
   message_loop_proxy->PostTask(FROM_HERE,
-      NewRunnableMethod(state_, &ServiceProcessState::StateData::SignalReady));
-  return true;
+      NewRunnableMethod(state_, &ServiceProcessState::StateData::SignalReady,
+                        &signal_ready,
+                        &success));
+  signal_ready.Wait();
+  return success;
 }
 
 void ServiceProcessState::TearDownState() {
diff --git a/chrome/common/service_process_util_posix.h b/chrome/common/service_process_util_posix.h
index b0a908b..08cca27 100644
--- a/chrome/common/service_process_util_posix.h
+++ b/chrome/common/service_process_util_posix.h
@@ -28,6 +28,10 @@ CFDictionaryRef CreateServiceProcessLaunchdPlist(CommandLine* cmd_line,
                                                  bool for_auto_launch);
 #endif  // OS_MACOSX
 
+namespace base {
+class WaitableEvent;
+}
+
 // Watches for |kShutDownMessage| to be written to the file descriptor it is
 // watching. When it reads |kShutDownMessage|, it performs |shutdown_task_|.
 // Used here to monitor the socket listening to g_signal_socket.
@@ -56,7 +60,7 @@ struct ServiceProcessState::StateData
 
   // WatchFileDescriptor needs to be set up by the thread that is going
   // to be monitoring it.
-  void SignalReady();
+  void SignalReady(base::WaitableEvent* signal, bool* success);
 
 
   // TODO(jhawkins): Either make this a class or rename these public member
diff --git a/chrome/common/service_process_util_unittest.cc b/chrome/common/service_process_util_unittest.cc
index 950953d..ab55bb5 100644
--- a/chrome/common/service_process_util_unittest.cc
+++ b/chrome/common/service_process_util_unittest.cc
@@ -368,7 +368,7 @@ class ServiceProcessStateFileManipulationTest : public ::testing::Test {
         NULL));
     loop_.PostDelayedTask(FROM_HERE,
                           new MessageLoop::QuitTask,
-                          TestTimeouts::large_test_timeout_ms());
+                          TestTimeouts::action_max_timeout_ms());
   }
 
   bool MakeABundle(const FilePath& dst,
@@ -484,8 +484,7 @@ void TrashFunc(const FilePath& src) {
   EXPECT_EQ(status, noErr)  << "FSMoveObjectToTrashSync " << status;
 }
 
-// Disabled because of race in FilePathWatcher.  http://crbug.com/77326
-TEST_F(ServiceProcessStateFileManipulationTest, DISABLED_DeleteFile) {
+TEST_F(ServiceProcessStateFileManipulationTest, DeleteFile) {
   GetIOMessageLoopProxy()->PostTask(
       FROM_HERE,
       NewRunnableFunction(&DeleteFunc, executable_path()));
@@ -494,8 +493,7 @@ TEST_F(ServiceProcessStateFileManipulationTest, DISABLED_DeleteFile) {
   ASSERT_TRUE(mock_launchd()->delete_called());
 }
 
-// Disabled because of race in FilePathWatcher.  http://crbug.com/77326
-TEST_F(ServiceProcessStateFileManipulationTest, DISABLED_DeleteBundle) {
+TEST_F(ServiceProcessStateFileManipulationTest, DeleteBundle) {
   GetIOMessageLoopProxy()->PostTask(
       FROM_HERE,
       NewRunnableFunction(&DeleteFunc, bundle_path()));
@@ -538,8 +536,7 @@ TEST_F(ServiceProcessStateFileManipulationTest, TrashBundle) {
   ASSERT_TRUE(file_util::Delete(file_path, true));
 }
 
-// http://crbug.com/77391
-TEST_F(ServiceProcessStateFileManipulationTest, FLAKY_ChangeAttr) {
+TEST_F(ServiceProcessStateFileManipulationTest, ChangeAttr) {
   ScopedAttributesRestorer restorer(bundle_path(), 0777);
   GetIOMessageLoopProxy()->PostTask(
       FROM_HERE,
diff --git a/tools/valgrind/gtest_exclude/unit_tests.gtest_mac.txt b/tools/valgrind/gtest_exclude/unit_tests.gtest_mac.txt
index 4440afb..59976ca 100644
--- a/tools/valgrind/gtest_exclude/unit_tests.gtest_mac.txt
+++ b/tools/valgrind/gtest_exclude/unit_tests.gtest_mac.txt
@@ -19,22 +19,7 @@ PrefsControllerTest.GetToolbarItemForPage
 PrefsControllerTest.ShowAndClose
 PrefsControllerTest.SwitchToPage
 
-# MultiProcessNotificationTests crash on the valgrind bot.
-# See http://crbug.com/69641.
-MultiProcessNotificationTest.BasicCreationTest
-MultiProcessNotificationTest.PostInProcessNotification
-MultiProcessNotificationTest.MultiListener
-MultiProcessNotificationTest.PostProfileNotification
-MultiProcessNotificationTest.PostUserNotification
-MultiProcessNotificationTest.PostSystemNotification
-MultiProcessNotificationTest.ProfileCrossDomainPosting
-MultiProcessNotificationTest.UserCrossDomainPosting
-MultiProcessNotificationTest.SystemCrossDomainPosting
-
 # Hangs
 # See http://crbug.com/75733
 BookmarkBarControllerTest.DeleteFromOffTheSideWhileItIsOpen
 
-# Fails under Valgrind.
-# See http://crbug.com/77064
-ServiceProcessStateFileManipulationTest.DeleteBundle