Revert of Require FilePathWatcher destructor to be called in sequence with Watch().

base/files/file_path_watcher_unittest.cc

Index: base/files/file_path_watcher_unittest.cc
diff --git a/base/files/file_path_watcher_unittest.cc b/base/files/file_path_watcher_unittest.cc
index d2ec37bbec1ba32acfb31407a8141ea1838ae36d..fc6f5a85fc919d4a080feac6d1b05283fdf92471 100644
--- a/base/files/file_path_watcher_unittest.cc
+++ b/base/files/file_path_watcher_unittest.cc
@@ -28,6 +28,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/test/test_file_util.h"
 #include "base/test/test_timeouts.h"
+#include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -35,10 +36,6 @@
 #if defined(OS_ANDROID)
 #include "base/android/path_utils.h"
 #endif  // defined(OS_ANDROID)
-
-#if defined(OS_POSIX)
-#include "base/files/file_descriptor_watcher_posix.h"
-#endif  // defined(OS_POSIX)
 
 namespace base {
 
@@ -134,19 +131,30 @@
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
+void SetupWatchCallback(const FilePath& target,
+                        FilePathWatcher* watcher,
+                        TestDelegateBase* delegate,
+                        bool recursive_watch,
+                        bool* result,
+                        base::WaitableEvent* completion) {
+  *result = watcher->Watch(target, recursive_watch,
+                           base::Bind(&TestDelegateBase::OnFileChanged,
+                                      delegate->AsWeakPtr()));
+  completion->Signal();
+}
+
 class FilePathWatcherTest : public testing::Test {
  public:
   FilePathWatcherTest()
-#if defined(OS_POSIX)
-      : file_descriptor_watcher_(&loop_)
-#endif
-  {
-  }
+      : file_thread_("FilePathWatcherTest") {}
 
   ~FilePathWatcherTest() override {}
 
  protected:
   void SetUp() override {
+    // Create a separate file thread in order to test proper thread usage.
+    base::Thread::Options options(MessageLoop::TYPE_IO, 0);
+    ASSERT_TRUE(file_thread_.StartWithOptions(options));
 #if defined(OS_ANDROID)
     // Watching files is only permitted when all parent directories are
     // accessible, which is not the case for the default temp directory
@@ -163,6 +171,10 @@
 
   void TearDown() override { RunLoop().RunUntilIdle(); }
 
+  void DeleteDelegateOnFileThread(TestDelegate* delegate) {
+    file_thread_.task_runner()->DeleteSoon(FROM_HERE, delegate);
+  }
+
   FilePath test_file() {
     return temp_dir_.GetPath().AppendASCII("FilePathWatcherTest");
   }
@@ -184,23 +196,18 @@
 
   bool WaitForEvents() WARN_UNUSED_RESULT {
     collector_->Reset();
-
-    RunLoop run_loop;
     // Make sure we timeout if we don't get notified.
-    ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, run_loop.QuitWhenIdleClosure(),
-        TestTimeouts::action_timeout());
-    run_loop.Run();
+    loop_.task_runner()->PostDelayedTask(FROM_HERE,
+                                         MessageLoop::QuitWhenIdleClosure(),
+                                         TestTimeouts::action_timeout());
+    RunLoop().Run();
     return collector_->Success();
   }
 
   NotificationCollector* collector() { return collector_.get(); }
 
-  MessageLoopForIO loop_;
-#if defined(OS_POSIX)
-  FileDescriptorWatcher file_descriptor_watcher_;
-#endif
-
+  MessageLoop loop_;
+  base::Thread file_thread_;
   ScopedTempDir temp_dir_;
   scoped_refptr<NotificationCollector> collector_;
 
@@ -212,9 +219,14 @@
                                      FilePathWatcher* watcher,
                                      TestDelegateBase* delegate,
                                      bool recursive_watch) {
-  return watcher->Watch(
-      target, recursive_watch,
-      base::Bind(&TestDelegateBase::OnFileChanged, delegate->AsWeakPtr()));
+  base::WaitableEvent completion(WaitableEvent::ResetPolicy::AUTOMATIC,
+                                 WaitableEvent::InitialState::NOT_SIGNALED);
+  bool result;
+  file_thread_.task_runner()->PostTask(
+      FROM_HERE, base::Bind(SetupWatchCallback, target, watcher, delegate,
+                            recursive_watch, &result, &completion));
+  completion.Wait();
+  return result;
 }
 
 // Basic test: Create the file and verify that we notice.
@@ -225,6 +237,7 @@
 
   ASSERT_TRUE(WriteFile(test_file(), "content"));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that modifying the file is caught.
@@ -238,6 +251,7 @@
   // Now make sure we get notified if the file is modified.
   ASSERT_TRUE(WriteFile(test_file(), "new content"));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that moving the file into place is caught.
@@ -252,6 +266,7 @@
   // Now make sure we get notified if the file is modified.
   ASSERT_TRUE(base::Move(source_file, test_file()));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, DeletedFile) {
@@ -264,6 +279,7 @@
   // Now make sure we get notified if the file is deleted.
   base::DeleteFile(test_file(), false);
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Used by the DeleteDuringNotify test below.
@@ -311,9 +327,11 @@
 // Flaky on MacOS (and ARM linux): http://crbug.com/85930
 TEST_F(FilePathWatcherTest, DISABLED_DestroyWithPendingNotification) {
   std::unique_ptr<TestDelegate> delegate(new TestDelegate(collector()));
-  FilePathWatcher watcher;
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get(), false));
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  FilePathWatcher* watcher = new FilePathWatcher;
+  ASSERT_TRUE(SetupWatch(test_file(), watcher, delegate.get(), false));
+  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  file_thread_.task_runner()->DeleteSoon(FROM_HERE, watcher);
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, MultipleWatchersSingleFile) {
@@ -325,6 +343,8 @@
 
   ASSERT_TRUE(WriteFile(test_file(), "content"));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate1.release());
+  DeleteDelegateOnFileThread(delegate2.release());
 }
 
 // Verify that watching a file whose parent directory doesn't exist yet works if
@@ -350,6 +370,7 @@
   ASSERT_TRUE(base::DeleteFile(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Exercises watch reconfiguration for the case that directories on the path
@@ -382,6 +403,7 @@
   ASSERT_TRUE(WriteFile(file, "content v2"));
   VLOG(1) << "Waiting for file modification";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #if defined(OS_MACOSX)
@@ -399,6 +421,7 @@
 
   ASSERT_TRUE(base::DeleteFile(dir, true));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Tests that a file that is deleted and reappears is tracked correctly.
@@ -415,6 +438,7 @@
   ASSERT_TRUE(WriteFile(test_file(), "content"));
   VLOG(1) << "Waiting for file creation";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, WatchDirectory) {
@@ -447,6 +471,7 @@
   ASSERT_TRUE(WriteFile(file2, "content"));
   VLOG(1) << "Waiting for file2 creation";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, MoveParent) {
@@ -472,6 +497,8 @@
   base::Move(dir, dest);
   VLOG(1) << "Waiting for directory move";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(file_delegate.release());
+  DeleteDelegateOnFileThread(subdir_delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, RecursiveWatch) {
@@ -481,6 +508,7 @@
   bool setup_result = SetupWatch(dir, &watcher, delegate.get(), true);
   if (!FilePathWatcher::RecursiveWatchAvailable()) {
     ASSERT_FALSE(setup_result);
+    DeleteDelegateOnFileThread(delegate.release());
     return;
   }
   ASSERT_TRUE(setup_result);
@@ -536,6 +564,7 @@
   // Delete "$dir/subdir/subdir_child_dir/child_dir_file1".
   ASSERT_TRUE(base::DeleteFile(child_dir_file1, false));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #if defined(OS_POSIX)
@@ -583,6 +612,8 @@
   FilePath target2_file(target2.AppendASCII("file"));
   ASSERT_TRUE(WriteFile(target2_file, "content"));
   ASSERT_TRUE(WaitForEvents());
+
+  DeleteDelegateOnFileThread(delegate.release());
 }
 #endif  // OS_POSIX
 
@@ -609,6 +640,8 @@
   // Move the directory into place, s.t. the watched file appears.
   ASSERT_TRUE(base::Move(source_dir, dest_dir));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(file_delegate.release());
+  DeleteDelegateOnFileThread(subdir_delegate.release());
 }
 
 // Verify that changing attributes on a file is caught
@@ -629,6 +662,7 @@
   // Now make sure we get notified if the file is modified.
   ASSERT_TRUE(base::MakeFileUnreadable(test_file()));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #if defined(OS_LINUX)
@@ -644,6 +678,7 @@
   // Note that test_file() doesn't have to exist.
   ASSERT_TRUE(CreateSymbolicLink(test_file(), test_link()));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that deleting a symlink is caught.
@@ -659,6 +694,7 @@
   // Now make sure we get notified if the link is deleted.
   ASSERT_TRUE(base::DeleteFile(test_link(), false));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that modifying a target file that a link is pointing to
@@ -674,6 +710,7 @@
   // Now make sure we get notified if the file is modified.
   ASSERT_TRUE(WriteFile(test_file(), "new content"));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that creating a target file that a link is pointing to
@@ -688,6 +725,7 @@
   // Now make sure we get notified if the target file is created.
   ASSERT_TRUE(WriteFile(test_file(), "content"));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that deleting a target file that a link is pointing to
@@ -703,6 +741,7 @@
   // Now make sure we get notified if the target file is deleted.
   ASSERT_TRUE(base::DeleteFile(test_file(), false));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that watching a file whose parent directory is a link that
@@ -731,6 +770,7 @@
   ASSERT_TRUE(base::DeleteFile(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that watching a file whose parent directory is a
@@ -760,6 +800,7 @@
   ASSERT_TRUE(base::DeleteFile(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that watching a file with a symlink on the path
@@ -787,6 +828,7 @@
   ASSERT_TRUE(base::DeleteFile(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #endif  // OS_LINUX
@@ -864,6 +906,7 @@
   ASSERT_TRUE(ChangeFilePermissions(test_dir1, Execute, false));
   ASSERT_TRUE(WaitForEvents());
   ASSERT_TRUE(ChangeFilePermissions(test_dir1, Execute, true));
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #endif  // OS_MACOSX