Don't block FILE thread for media scanning

chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc

 // Copyright 2016 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 "chrome/browser/chromeos/arc/arc_downloads_watcher_service.h"
 
 #include <map>
 #include <memory>
 #include <utility>
 
@@ skipping 12 matching lines @@
 
 using content::BrowserThread;
 
 // Mapping from Android file paths to last modified timestamps.
 using TimestampMap = std::map<base::FilePath, base::Time>;
 
 namespace arc {
 
 namespace {
 
+// ID used to ensure sequential execution of HandleFilePathChanged.
+// This is sequential to prevent two instances from updating last_update_time_.
+const char kSequenceId[] = "arc_downloads_watcher";
+
 const base::FilePath::CharType kAndroidDownloadDir[] =
     FILE_PATH_LITERAL("/storage/emulated/0/Download");
 
 // Compares two TimestampMaps and returns the list of file paths added/removed
 // or whose timestamp have changed.
 std::vector<base::FilePath> CollectChangedPaths(
     const TimestampMap& timestamp_map_a,
     const TimestampMap& timestamp_map_b) {
   std::vector<base::FilePath> changed_paths;
 
@@ skipping 37 matching lines @@
       base::Callback<void(const std::vector<base::FilePath>& paths)>;
 
   explicit DownloadsWatcher(const Callback& callback);
   ~DownloadsWatcher();
 
   // Starts watching Downloads directory.
   void Start();
 
  private:
   // Called by base::FilePathWatcher to notify file changes.
+  // Wrapper around HandleFilePathChanged to pass the notification timestamp
+  // and move the work off the FILE thread.
   void OnFilePathChanged(const base::FilePath& path, bool error);
 
+  // If there has not been a full scan of updated files since notify_time,
+  // update the last_timestamp_map_ and send any new/changed files to the
+  // android media scanner.
+  void HandleFilePathChanged(base::Time notify_time);
+
   // Scans files under |downloads_dir_| recursively and builds a map from file
   // paths (in Android filesystem) to last modified timestamps.
   TimestampMap BuildTimestampMap() const;
 
   Callback callback_;
   base::FilePath downloads_dir_;
   std::unique_ptr<base::FilePathWatcher> watcher_;
   TimestampMap last_timestamp_map_;
+  base::Time last_update_time_;
 
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate the weak pointers before any other members are destroyed.
   base::WeakPtrFactory<DownloadsWatcher> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadsWatcher);
 };
 
 ArcDownloadsWatcherService::DownloadsWatcher::DownloadsWatcher(
     const Callback& callback)
-    : callback_(callback), weak_ptr_factory_(this) {
+    : callback_(callback),
+      last_update_time_(base::Time()),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   downloads_dir_ = DownloadPrefs(ProfileManager::GetActiveUserProfile())
                        .GetDefaultDownloadDirectoryForProfile()
                        .StripTrailingSeparators();
 }
 
 ArcDownloadsWatcherService::DownloadsWatcher::~DownloadsWatcher() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
 }
@@ skipping 11 matching lines @@
                   base::Bind(&DownloadsWatcher::OnFilePathChanged,
                              weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcDownloadsWatcherService::DownloadsWatcher::OnFilePathChanged(
     const base::FilePath& path,
     bool error) {
   // On Linux, |error| is always false. Also, |path| is always the same path
   // as one given to FilePathWatcher::Watch().
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  BrowserThread::PostBlockingPoolSequencedTask(
+      kSequenceId, FROM_HERE,
+      base::Bind(&DownloadsWatcher::HandleFilePathChanged,
+                 weak_ptr_factory_.GetWeakPtr(), base::Time::Now()));

[Shuhei Takahashi, 2016/09/05 09:28:23]

Weak pointers can be dereferenced only on FILE thread, but this callback would
cause it to be dereferenced on the blocking pool task runner. I guess DCHECK
will fail.

See: https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?l=49

[dspaid, 2016/09/07 01:16:56]

Thanks for the reminder.  I couldn't really find a thread safe way to get this
working so I've moved it back to the FILE thread.  However the rest of the
optimizations should still keep it from getting too bad.

+}
+
+void ArcDownloadsWatcherService::DownloadsWatcher::HandleFilePathChanged(
+    base::Time notify_time) {
+  if (notify_time < last_update_time_) {

[Shuhei Takahashi, 2016/09/05 09:28:23]

Please add a comment about the purpose of this check and how it works, this is a
bit tricky :)

[dspaid, 2016/09/07 01:16:56]

Done.

+    return;
+  }
+  last_update_time_ = base::Time::Now();
 
   TimestampMap current_timestamp_map = BuildTimestampMap();
 
   std::vector<base::FilePath> changed_paths =
       CollectChangedPaths(last_timestamp_map_, current_timestamp_map);
 
   last_timestamp_map_ = std::move(current_timestamp_map);
 
   callback_.Run(changed_paths);

[Shuhei Takahashi, 2016/09/05 09:28:23]

|callback_| should be run on FILE thread.

[dspaid, 2016/09/07 01:16:56]

Looks like this was fixed in a recent CL by hashimoto@

 }
 
 TimestampMap ArcDownloadsWatcherService::DownloadsWatcher::BuildTimestampMap()
     const {
   DCHECK(!downloads_dir_.EndsWithSeparator());
   TimestampMap timestamp_map;
 
   // Enumerate normal files only; directories and symlinks are skipped.
   base::FileEnumerator enumerator(downloads_dir_, true,
                                   base::FileEnumerator::FILES);
@@ skipping 64 matching lines @@
     return;
 
   mojo::Array<mojo::String> mojo_paths(paths.size());
   for (size_t i = 0; i < paths.size(); ++i) {
     mojo_paths[i] = paths[i].value();
   }
   instance->RequestMediaScan(std::move(mojo_paths));
 }
 
 }  // namespace arc