arc: Fix thread unsafe behavior of ArcDownloadsWatcherService

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 17 matching lines @@
 
 namespace arc {
 
 namespace {
 
 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(
+std::vector<std::string> CollectChangedPaths(

[hashimoto, 2016/09/01 07:34:20]

Semantically, base::FilePath suits better than std::string here, but
base::FilePath is not movable so it's bad for performance.
Which do you prefer?

[hidehiko, 2016/09/01 17:05:45]

Is it measurable? If so, it's good to note.
If not, I prefer cleaner semantic code, so base:FilePath, personally, but not
strong opinion in this case and either is fine.

Alternatively, you could add no-except-move-ctor for chrome-wide
vector<base::FilePath> better performance.

[hashimoto, 2016/09/02 03:56:00]

I just thought it might be nice to do as there is a performance issue in this
class, but I didn't measure the performance. 
If your preference is base::FilePath, let's stick to it as the performance
difference shouldn't be huge.

     const TimestampMap& timestamp_map_a,
     const TimestampMap& timestamp_map_b) {
-  std::vector<base::FilePath> changed_paths;
+  std::vector<std::string> changed_paths;
 
   TimestampMap::const_iterator iter_a = timestamp_map_a.begin();
   TimestampMap::const_iterator iter_b = timestamp_map_b.begin();
   while (iter_a != timestamp_map_a.end() && iter_b != timestamp_map_b.end()) {
     if (iter_a->first == iter_b->first) {
       if (iter_a->second != iter_b->second) {
-        changed_paths.emplace_back(iter_a->first);
+        changed_paths.emplace_back(iter_a->first.value());
       }
       ++iter_a;
       ++iter_b;
     } else if (iter_a->first < iter_b->first) {
-      changed_paths.emplace_back(iter_a->first);
+      changed_paths.emplace_back(iter_a->first.value());
       ++iter_a;
     } else {  // iter_a->first > iter_b->first
-      changed_paths.emplace_back(iter_b->first);
+      changed_paths.emplace_back(iter_b->first.value());
       ++iter_b;
     }
   }
 
   while (iter_a != timestamp_map_a.end()) {
-    changed_paths.emplace_back(iter_a->first);
+    changed_paths.emplace_back(iter_a->first.value());
     ++iter_a;
   }
   while (iter_b != timestamp_map_b.end()) {
-    changed_paths.emplace_back(iter_b->first);
+    changed_paths.emplace_back(iter_b->first.value());
     ++iter_b;
   }
 
   return changed_paths;
 }
 
 }  // namespace
 
 // The core part of ArcDownloadsWatcherService to watch for file changes in
 // Downloads directory.
 class ArcDownloadsWatcherService::DownloadsWatcher {
  public:
   using Callback =
-      base::Callback<void(const std::vector<base::FilePath>& paths)>;
+      base::Callback<void(std::unique_ptr<mojo::Array<mojo::String>> paths)>;
 
   explicit DownloadsWatcher(const Callback& callback);
   ~DownloadsWatcher();
 
   // Starts watching Downloads directory.
   void Start();
 
  private:
   // Called by base::FilePathWatcher to notify file changes.
   void OnFilePathChanged(const base::FilePath& path, bool error);
@@ skipping 44 matching lines @@
 
 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);
 
   TimestampMap current_timestamp_map = BuildTimestampMap();
 
-  std::vector<base::FilePath> changed_paths =
+  std::vector<std::string> changed_paths =
       CollectChangedPaths(last_timestamp_map_, current_timestamp_map);
 
   last_timestamp_map_ = std::move(current_timestamp_map);
 
-  callback_.Run(changed_paths);
+  auto mojo_paths =
+      base::MakeUnique<mojo::Array<mojo::String>>(changed_paths.size());

[hidehiko, 2016/09/01 17:05:45]

Is it necessary to be unique_ptr? I thought mojo::Array supports std::move so it
should work with base::Passed()?

[hashimoto, 2016/09/02 03:56:00]

Thank you for pointing this out.
I wasn't aware of the fact that base::Passed can work with classes other than
scoped_ptr (which no longer exists).

+  for (size_t i = 0; i < changed_paths.size(); ++i) {
+    (*mojo_paths)[i] = std::move(changed_paths[i]);
+  }
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(callback_, base::Passed(std::move(mojo_paths))));
 }
 
 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 49 matching lines @@
 
 void ArcDownloadsWatcherService::StopWatchingDownloads() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (watcher_) {
     BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE,
                               watcher_.release());
   }
 }
 
 void ArcDownloadsWatcherService::OnDownloadsChanged(
-    const std::vector<base::FilePath>& paths) {
-  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+    std::unique_ptr<mojo::Array<mojo::String>> paths) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   auto* instance = arc_bridge_service()->file_system()->instance();
   if (!instance)
     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));
+  instance->RequestMediaScan(std::move(*paths));
 }
 
 }  // namespace arc