Switch DevToolsFileWatcher and DevToolsFileHelper to SequencedTaskRunner

This re-lands https://chromium-review.googlesource.com/535154 while fixing
a silly crash due to binding to a not-yet-initialized member.

Bug: 689520
Change-Id: I42ad68d73b9c3437dcfd97f099527a09ce1a0d09
Reviewed-on: https://chromium-review.googlesource.com/540118
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481079}

chrome/browser/devtools/devtools_file_helper.cc

@@ -15,6 +15,8 @@
 #include "base/md5.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_scheduler/post_task.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/value_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/devtools/devtools_file_watcher.h"
@@ -120,14 +122,14 @@ class SelectFileDialog : public ui::SelectFileDialog::Listener,
 };
 
 void WriteToFile(const base::FilePath& path, const std::string& content) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(!path.empty());
 
   base::WriteFile(path, content.c_str(), content.length());
 }
 
 void AppendToFile(const base::FilePath& path, const std::string& content) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  base::ThreadRestrictions::AssertIOAllowed();
   DCHECK(!path.empty());
 
   base::AppendToFile(path, content.c_str(), content.size());
@@ -210,14 +212,13 @@ DevToolsFileHelper::DevToolsFileHelper(WebContents* web_contents,
     : web_contents_(web_contents),
       profile_(profile),
       delegate_(delegate),
+      file_task_runner_(
+          base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()})),
       weak_factory_(this) {
   pref_change_registrar_.Init(profile_->GetPrefs());
 }
 
-DevToolsFileHelper::~DevToolsFileHelper() {
-  BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE,
-                            file_watcher_.release());
-}
+DevToolsFileHelper::~DevToolsFileHelper() = default;
 
 void DevToolsFileHelper::Save(const std::string& url,
                               const std::string& content,
@@ -275,8 +276,8 @@ void DevToolsFileHelper::Append(const std::string& url,
   if (it == saved_files_.end())
     return;
   callback.Run();
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          BindOnce(&AppendToFile, it->second, content));
+  file_task_runner_->PostTask(FROM_HERE,
+                              BindOnce(&AppendToFile, it->second, content));
 }
 
 void DevToolsFileHelper::SaveAsFileSelected(const std::string& url,
@@ -292,8 +293,7 @@ void DevToolsFileHelper::SaveAsFileSelected(const std::string& url,
   files_map->SetWithoutPathExpansion(base::MD5String(url),
                                      base::CreateFilePathValue(path));
   callback.Run();
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          BindOnce(&WriteToFile, path, content));
+  file_task_runner_->PostTask(FROM_HERE, BindOnce(&WriteToFile, path, content));
 }
 
 void DevToolsFileHelper::AddFileSystem(
@@ -308,23 +308,25 @@ void DevToolsFileHelper::AddFileSystem(
     select_file_dialog->Show(ui::SelectFileDialog::SELECT_FOLDER,
                              base::FilePath());
   } else {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
+    file_task_runner_->PostTask(
+        FROM_HERE,
         BindOnce(&DevToolsFileHelper::CheckProjectFileExistsAndAddFileSystem,
                  weak_factory_.GetWeakPtr(), show_info_bar_callback,
                  base::FilePath::FromUTF8Unsafe(file_system_path)));
   }
 }
 
+// static
 void DevToolsFileHelper::CheckProjectFileExistsAndAddFileSystem(
-    const ShowInfoBarCallback& show_info_bar_callback,
-    const base::FilePath& path) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+    base::WeakPtr<DevToolsFileHelper> self,
+    ShowInfoBarCallback show_info_bar_callback,
+    base::FilePath path) {
+  base::ThreadRestrictions::AssertIOAllowed();
   if (base::PathExists(path.Append(FILE_PATH_LITERAL(".devtools")))) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        BindOnce(&DevToolsFileHelper::InnerAddFileSystem,
-                 weak_factory_.GetWeakPtr(), show_info_bar_callback, path));
+        BindOnce(&DevToolsFileHelper::InnerAddFileSystem, std::move(self),
+                 std::move(show_info_bar_callback), path));
   }
 }
 
@@ -387,8 +389,10 @@ DevToolsFileHelper::GetFileSystems() {
   file_system_paths_ = GetAddedFileSystemPaths(profile_);
   std::vector<FileSystem> file_systems;
   if (!file_watcher_) {
-    file_watcher_.reset(new DevToolsFileWatcher(base::Bind(
-        &DevToolsFileHelper::FilePathsChanged, weak_factory_.GetWeakPtr())));
+    file_watcher_.reset(new DevToolsFileWatcher(
+        base::Bind(&DevToolsFileHelper::FilePathsChanged,
+                   weak_factory_.GetWeakPtr()),
+        base::SequencedTaskRunnerHandle::Get()));
     pref_change_registrar_.Add(
         prefs::kDevToolsFileSystemPaths,
         base::Bind(&DevToolsFileHelper::FileSystemPathsSettingChanged,
@@ -401,10 +405,7 @@ DevToolsFileHelper::GetFileSystems() {
                                                    file_system_id,
                                                    file_system_path);
     file_systems.push_back(filesystem);
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        BindOnce(&DevToolsFileWatcher::AddWatch,
-                 base::Unretained(file_watcher_.get()), path));
+    file_watcher_->AddWatch(std::move(path));
   }
   return file_systems;
 }
@@ -440,10 +441,7 @@ void DevToolsFileHelper::FileSystemPathsSettingChanged() {
                                                      file_system_id,
                                                      file_system_path);
       delegate_->FileSystemAdded(filesystem);
-      BrowserThread::PostTask(
-          BrowserThread::FILE, FROM_HERE,
-          BindOnce(&DevToolsFileWatcher::AddWatch,
-                   base::Unretained(file_watcher_.get()), path));
+      file_watcher_->AddWatch(std::move(path));
     } else {
       remaining.erase(file_system_path);
     }
@@ -453,10 +451,7 @@ void DevToolsFileHelper::FileSystemPathsSettingChanged() {
   for (auto file_system_path : remaining) {
     delegate_->FileSystemRemoved(file_system_path);
     base::FilePath path = base::FilePath::FromUTF8Unsafe(file_system_path);
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        BindOnce(&DevToolsFileWatcher::RemoveWatch,
-                 base::Unretained(file_watcher_.get()), path));
+    file_watcher_->RemoveWatch(std::move(path));
   }
 }
 

chrome/browser/devtools/devtools_file_helper.h

@@ -16,13 +16,14 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
+#include "chrome/browser/devtools/devtools_file_watcher.h"
 #include "components/prefs/pref_change_registrar.h"
 
-class DevToolsFileWatcher;
 class Profile;
 
 namespace base {
 class FilePath;
+class SequencedTaskRunner;
 }
 
 namespace content {
@@ -128,9 +129,6 @@ class DevToolsFileHelper {
   void InnerAddFileSystem(
       const ShowInfoBarCallback& show_info_bar_callback,
       const base::FilePath& path);
-  void CheckProjectFileExistsAndAddFileSystem(
-      const ShowInfoBarCallback& show_info_bar_callback,
-      const base::FilePath& path);
   void AddUserConfirmedFileSystem(
       const base::FilePath& path,
       bool allowed);
@@ -139,6 +137,12 @@ class DevToolsFileHelper {
                         const std::vector<std::string>& added_paths,
                         const std::vector<std::string>& removed_paths);
 
+  // This should only be called on the file sequence.
+  static void CheckProjectFileExistsAndAddFileSystem(
+      base::WeakPtr<DevToolsFileHelper> self,
+      ShowInfoBarCallback show_info_bar_callback,
+      base::FilePath path);
+
   content::WebContents* web_contents_;
   Profile* profile_;
   DevToolsFileHelper::Delegate* delegate_;
@@ -146,7 +150,9 @@ class DevToolsFileHelper {
   PathsMap saved_files_;
   PrefChangeRegistrar pref_change_registrar_;
   std::set<std::string> file_system_paths_;
-  std::unique_ptr<DevToolsFileWatcher> file_watcher_;
+  std::unique_ptr<DevToolsFileWatcher, DevToolsFileWatcher::Deleter>
+      file_watcher_;
+  scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
   base::WeakPtrFactory<DevToolsFileHelper> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(DevToolsFileHelper);
 };

chrome/browser/devtools/devtools_file_watcher.cc

@@ -15,6 +15,9 @@
 #include "base/files/file_path_watcher.h"
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
+#include "base/task_scheduler/lazy_task_runner.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ -51,6 +54,7 @@ class DevToolsFileWatcher::SharedFileWatcher :
   std::set<base::FilePath> pending_paths_;
   base::Time last_event_time_;
   base::TimeDelta last_dispatch_cost_;
+  SEQUENCE_CHECKER(sequence_checker_);
 };
 
 DevToolsFileWatcher::SharedFileWatcher::SharedFileWatcher()
@@ -60,22 +64,26 @@ DevToolsFileWatcher::SharedFileWatcher::SharedFileWatcher()
 }
 
 DevToolsFileWatcher::SharedFileWatcher::~SharedFileWatcher() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DevToolsFileWatcher::s_shared_watcher_ = nullptr;
 }
 
 void DevToolsFileWatcher::SharedFileWatcher::AddListener(
     DevToolsFileWatcher* watcher) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   listeners_.push_back(watcher);
 }
 
 void DevToolsFileWatcher::SharedFileWatcher::RemoveListener(
     DevToolsFileWatcher* watcher) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto it = std::find(listeners_.begin(), listeners_.end(), watcher);
   listeners_.erase(it);
 }
 
 void DevToolsFileWatcher::SharedFileWatcher::AddWatch(
     const base::FilePath& path) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (watchers_.find(path) != watchers_.end())
     return;
   if (!base::FilePathWatcher::RecursiveWatchAvailable())
@@ -104,13 +112,14 @@ void DevToolsFileWatcher::SharedFileWatcher::GetModificationTimes(
 
 void DevToolsFileWatcher::SharedFileWatcher::RemoveWatch(
     const base::FilePath& path) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   watchers_.erase(path);
 }
 
 void DevToolsFileWatcher::SharedFileWatcher::DirectoryChanged(
     const base::FilePath& path,
     bool error) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   pending_paths_.insert(path);
   if (pending_paths_.size() > 1)
     return;  // PostDelayedTask is already pending.
@@ -122,8 +131,8 @@ void DevToolsFileWatcher::SharedFileWatcher::DirectoryChanged(
           base::TimeDelta::FromMilliseconds(kFirstThrottleTimeout) :
           last_dispatch_cost_ * 2;
 
-  BrowserThread::PostDelayedTask(
-      BrowserThread::FILE, FROM_HERE,
+  base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
       base::BindOnce(
           &DevToolsFileWatcher::SharedFileWatcher::DispatchNotifications, this),
       shedule_for);
@@ -131,6 +140,7 @@ void DevToolsFileWatcher::SharedFileWatcher::DirectoryChanged(
 }
 
 void DevToolsFileWatcher::SharedFileWatcher::DispatchNotifications() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!pending_paths_.size())
     return;
   base::Time start = base::Time::Now();
@@ -160,30 +170,49 @@ void DevToolsFileWatcher::SharedFileWatcher::DispatchNotifications() {
   pending_paths_.clear();
 
   for (auto* watcher : listeners_) {
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::BindOnce(watcher->callback_, changed_paths,
-                                           added_paths, removed_paths));
+    watcher->client_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(watcher->callback_, changed_paths,
+                                  added_paths, removed_paths));
   }
   last_dispatch_cost_ = base::Time::Now() - start;
 }
 
+// DevToolsFileWatcher ---------------------------------------------------------
+
+namespace {
+base::SequencedTaskRunner* impl_task_runner() {
+  constexpr base::TaskTraits kImplTaskTraits = {base::MayBlock(),
+                                                base::TaskPriority::BACKGROUND};
+  static base::LazySequencedTaskRunner s_file_task_runner =
+      LAZY_SEQUENCED_TASK_RUNNER_INITIALIZER(kImplTaskTraits);
+
+  return s_file_task_runner.Get().get();
+}
+}  // namespace
+
 // static
 DevToolsFileWatcher::SharedFileWatcher*
 DevToolsFileWatcher::s_shared_watcher_ = nullptr;
 
-// DevToolsFileWatcher ---------------------------------------------------------
+// static
+void DevToolsFileWatcher::Deleter::operator()(const DevToolsFileWatcher* ptr) {
+  impl_task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&DevToolsFileWatcher::Destroy, base::Unretained(ptr)));
+}
 
-DevToolsFileWatcher::DevToolsFileWatcher(const WatchCallback& callback)
-    : callback_(callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::BindOnce(&DevToolsFileWatcher::InitSharedWatcher,
-                     base::Unretained(this)));
+DevToolsFileWatcher::DevToolsFileWatcher(
+    WatchCallback callback,
+    scoped_refptr<base::SequencedTaskRunner> callback_task_runner)
+    : callback_(std::move(callback)),
+      client_task_runner_(std::move(callback_task_runner)) {
+  impl_task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&DevToolsFileWatcher::InitSharedWatcher,
+                                base::Unretained(this)));
 }
 
 DevToolsFileWatcher::~DevToolsFileWatcher() {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  DCHECK(impl_task_runner()->RunsTasksInCurrentSequence());
   shared_watcher_->RemoveListener(this);
 }
 
@@ -194,12 +223,22 @@ void DevToolsFileWatcher::InitSharedWatcher() {
   shared_watcher_->AddListener(this);
 }
 
-void DevToolsFileWatcher::AddWatch(const base::FilePath& path) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  shared_watcher_->AddWatch(path);
+void DevToolsFileWatcher::AddWatch(base::FilePath path) {
+  impl_task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&DevToolsFileWatcher::AddWatchOnImpl,
+                                base::Unretained(this), std::move(path)));
+}
+
+void DevToolsFileWatcher::RemoveWatch(base::FilePath path) {
+  impl_task_runner()->PostTask(
+      FROM_HERE, base::BindOnce(&DevToolsFileWatcher::AddWatchOnImpl,
+                                base::Unretained(this), std::move(path)));
+}
+
+void DevToolsFileWatcher::AddWatchOnImpl(base::FilePath path) {
+  shared_watcher_->AddWatch(std::move(path));
 }
 
-void DevToolsFileWatcher::RemoveWatch(const base::FilePath& path) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  shared_watcher_->RemoveWatch(path);
+void DevToolsFileWatcher::RemoveWatchOnImpl(base::FilePath path) {
+  shared_watcher_->RemoveWatch(std::move(path));
 }

chrome/browser/devtools/devtools_file_watcher.h

@@ -12,28 +12,39 @@
 
 namespace base {
 class FilePath;
+class SequencedTaskRunner;
 }
 
 class DevToolsFileWatcher {
  public:
+  struct Deleter {
+    void operator()(const DevToolsFileWatcher* ptr);
+  };
+
   using WatchCallback = base::Callback<void(const std::vector<std::string>&,
                                             const std::vector<std::string>&,
                                             const std::vector<std::string>&)>;
-  explicit DevToolsFileWatcher(const WatchCallback& callback);
-  ~DevToolsFileWatcher();
+  DevToolsFileWatcher(
+      WatchCallback callback,
+      scoped_refptr<base::SequencedTaskRunner> callback_task_runner);
 
-  void AddWatch(const base::FilePath& path);
-  void RemoveWatch(const base::FilePath& path);
+  void AddWatch(base::FilePath path);
+  void RemoveWatch(base::FilePath path);
 
  private:
+  ~DevToolsFileWatcher();  // Use Deleter to destroy objects of this type.
   class SharedFileWatcher;
   static SharedFileWatcher* s_shared_watcher_;
 
+  void Destroy() const { delete this; }
   void InitSharedWatcher();
-  void FileChanged(const base::FilePath&, int);
+  void AddWatchOnImpl(base::FilePath path);
+  void RemoveWatchOnImpl(base::FilePath path);
 
   scoped_refptr<SharedFileWatcher> shared_watcher_;
   WatchCallback callback_;
+  scoped_refptr<base::SequencedTaskRunner> client_task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(DevToolsFileWatcher);
 };
 

chrome/browser/devtools/devtools_file_watcher_unittest.cc

+// Copyright 2017 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.
+
+#include <set>
+
+#include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "chrome/browser/devtools/devtools_file_watcher.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class DevToolsFileWatcherTest : public testing::Test {
+ public:
+  void Callback(const std::vector<std::string>& changed_paths,
+                const std::vector<std::string>& added_paths,
+                const std::vector<std::string>& removed_paths) {
+    for (auto& p : changed_paths)
+      expected_changed_paths_.erase(p);
+    for (auto& p : added_paths)
+      ASSERT_EQ(1ul, expected_added_paths_.erase(p));
+    for (auto& p : removed_paths)
+      ASSERT_EQ(1ul, expected_removed_paths_.erase(p));
+  }
+
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    base_path_ = temp_dir_.GetPath();
+  }
+
+  base::ScopedTempDir temp_dir_;
+  base::FilePath base_path_;
+  base::test::ScopedTaskEnvironment task_environment_;
+  bool done_flag_ = false;
+
+  std::set<std::string> expected_changed_paths_;
+  std::set<std::string> expected_added_paths_;
+  std::set<std::string> expected_removed_paths_;
+};
+
+TEST_F(DevToolsFileWatcherTest, BasicUsage) {
+  std::unique_ptr<DevToolsFileWatcher, DevToolsFileWatcher::Deleter> watcher(
+      new DevToolsFileWatcher(base::Bind(&DevToolsFileWatcherTest::Callback,
+                                         base::Unretained(this)),
+                              base::SequencedTaskRunnerHandle::Get()));
+
+  base::FilePath changed_path = base_path_.Append(FILE_PATH_LITERAL("file1"));
+  base::WriteFile(changed_path, "test", 4);
+
+  watcher->AddWatch(base_path_);
+  expected_changed_paths_.insert(changed_path.AsUTF8Unsafe());
+
+  while (!expected_changed_paths_.empty()) {
+    task_environment_.RunUntilIdle();
+    base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+    // Just for the first operation, repeat it until we get the callback, as
+    // watcher may take some time to start on another thread.
+    base::WriteFile(changed_path, "test", 4);
+  }
+
+  base::FilePath added_path = base_path_.Append(FILE_PATH_LITERAL("file2"));
+  expected_added_paths_.insert(added_path.AsUTF8Unsafe());
+  base::WriteFile(added_path, "test", 4);
+  while (!expected_added_paths_.empty()) {
+    task_environment_.RunUntilIdle();
+    base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+  }
+
+  expected_removed_paths_.insert(added_path.AsUTF8Unsafe());
+  base::DeleteFile(added_path, false);
+  while (!expected_removed_paths_.empty()) {
+    task_environment_.RunUntilIdle();
+    base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());
+  }
+}

chrome/test/BUILD.gn

@@ -3722,6 +3722,7 @@ test("unit_tests") {
 
   if (!is_android) {
     sources += [
+      "../browser/devtools/devtools_file_watcher_unittest.cc",
       "../browser/devtools/devtools_ui_bindings_unittest.cc",
       "../browser/devtools/serialize_host_descriptions_unittest.cc",
       "../browser/download/download_dir_policy_handler_unittest.cc",