drive: Fix two instances of madness in FileBrowserEventRouter

chrome/browser/chromeos/extensions/file_browser_event_router.cc

 // Copyright (c) 2012 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/extensions/file_browser_event_router.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/json/json_writer.h"
 #include "base/message_loop.h"
@@ skipping 210 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   content::BrowserThread::PostBlockingPoolTask(
       FROM_HERE,
       base::Bind(&DirectoryExistsOnBlockingPool,
                  directory_path,
                  success_callback,
                  failure_callback));
 };
 
+// Creates a base::FilePathWatcher and starts watching at |watch_path| with
+// |callback|. Returns NULL on failure.
+base::FilePathWatcher* CreateAndStartFilePathWatcher(
+    const base::FilePath& watch_path,
+    const base::FilePathWatcher::Callback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(!callback.is_null());
+
+  base::FilePathWatcher* watcher(new base::FilePathWatcher);
+  if (!watcher->Watch(watch_path, false /* recursive */, callback)) {
+    delete watcher;
+    return NULL;
+  }
+
+  return watcher;
+}
+
 }  // namespace
 
 FileBrowserEventRouter::FileBrowserEventRouter(
     Profile* profile)
     : weak_factory_(this),
       notifications_(new FileBrowserNotifications(profile)),
       pref_change_registrar_(new PrefChangeRegistrar),
       profile_(profile),
       num_remote_update_requests_(0),
       shift_pressed_(false) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // Bind a weak reference back to |this| to avoid memory errors in case we
-  // shut down while a callback is in flight.
   file_watcher_callback_ =
-      base::Bind(&RelayFileWatcherCallbackToUIThread,
-          base::Bind(&FileBrowserEventRouter::HandleFileWatchNotification,
-                     weak_factory_.GetWeakPtr()));
+      base::Bind(&FileBrowserEventRouter::HandleFileWatchNotification,
+                 weak_factory_.GetWeakPtr());
 
   // Listen for the Shift modifier's state changes.
   chromeos::SystemKeyEventListener* key_event_listener =
       chromeos::SystemKeyEventListener::GetInstance();
   if (key_event_listener)
     key_event_listener->AddModifiersObserver(this);
 }
 
 FileBrowserEventRouter::~FileBrowserEventRouter() {
 }
@@ skipping 58 matching lines @@
     chromeos::ConnectivityStateHelper::Get()->
         AddNetworkManagerObserver(this);
   }
   suspend_state_delegate_.reset(new SuspendStateDelegateImpl());
 
   pref_change_registrar_->Init(profile_->GetPrefs());
 
   pref_change_registrar_->Add(
       prefs::kExternalStorageDisabled,
       base::Bind(&FileBrowserEventRouter::OnExternalStorageDisabledChanged,
                  base::Unretained(this)));

[satorux1, 2013/04/09 01:24:38]

replaced this with a weak ptr.

 
   base::Closure callback =
       base::Bind(&FileBrowserEventRouter::OnFileBrowserPrefsChanged,
                  base::Unretained(this));

[satorux1, 2013/04/09 01:24:38]

replaced this with a weak ptr.

   pref_change_registrar_->Add(prefs::kDisableDriveOverCellular, callback);
   pref_change_registrar_->Add(prefs::kDisableDriveHostedFiles, callback);
   pref_change_registrar_->Add(prefs::kDisableDrive, callback);
   pref_change_registrar_->Add(prefs::kUse24HourClock, callback);
 }
 
 // File watch setup routines.
 bool FileBrowserEventRouter::AddFileWatch(
     const base::FilePath& local_path,
     const base::FilePath& virtual_path,
-    const std::string& extension_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    const std::string& extension_id,
+    const BoolCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
-  base::AutoLock lock(lock_);
   base::FilePath watch_path = local_path;
   bool is_remote_watch = false;
   // Tweak watch path for remote sources - we need to drop leading /special
   // directory from there in order to be able to pair these events with
   // their change notifications.
   if (drive::util::GetSpecialRemoteRootPath().IsParent(watch_path)) {
     watch_path = drive::util::ExtractDrivePath(watch_path);
     is_remote_watch = true;
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&FileBrowserEventRouter::HandleRemoteUpdateRequestOnUIThread,
-                   this, true));
+    HandleRemoteUpdateRequestOnUIThread(true /* start */);
   }
 
   WatcherMap::iterator iter = file_watchers_.find(watch_path);
   if (iter == file_watchers_.end()) {
     scoped_ptr<FileWatcherExtensions>
         watch(new FileWatcherExtensions(virtual_path,
                                         extension_id,
                                         is_remote_watch));
-
-    if (watch->Watch(watch_path, file_watcher_callback_))
-      file_watchers_[watch_path] = watch.release();
-    else
-      return false;
+    watch->Watch(watch_path,
+                 file_watcher_callback_,
+                 callback);
+    file_watchers_[watch_path] = watch.release();
   } else {
     iter->second->AddExtension(extension_id);
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+                                                base::Bind(callback, true));
   }
   return true;
 }
 
 void FileBrowserEventRouter::RemoveFileWatch(
     const base::FilePath& local_path,
     const std::string& extension_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  base::AutoLock lock(lock_);
   base::FilePath watch_path = local_path;
   // Tweak watch path for remote sources - we need to drop leading /special
   // directory from there in order to be able to pair these events with
   // their change notifications.
   if (drive::util::GetSpecialRemoteRootPath().IsParent(watch_path)) {
     watch_path = drive::util::ExtractDrivePath(watch_path);
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&FileBrowserEventRouter::HandleRemoteUpdateRequestOnUIThread,
-                   this, false));
+    HandleRemoteUpdateRequestOnUIThread(false /* start */);
   }
   WatcherMap::iterator iter = file_watchers_.find(watch_path);
   if (iter == file_watchers_.end())
     return;
   // Remove the renderer process for this watch.
   iter->second->RemoveExtension(extension_id);
   if (iter->second->GetRefCount() == 0) {
-    delete iter->second;
+    BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, iter->second);

[hashimoto, 2013/04/08 11:10:26]

Why do you delete FileWatcherExtensions on FILE thread while having
"DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));" in the dtor of the
class?

[satorux1, 2013/04/09 01:24:38]

oops. Good catch! I was confused. FilePathWatcher needs to be deleted on FILE
thread, but FileWatcherExtensions does not. Fixed.

     file_watchers_.erase(iter);
   }
 }
 
 void FileBrowserEventRouter::MountDrive(
     const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Pass back the gdata mount point path as source path.
   const std::string& gdata_path = drive::util::GetDriveMountPointPathAsString();
@@ skipping 237 matching lines @@
       extensions::event_names::kOnFileBrowserDriveConnectionStatusChanged,
       scoped_ptr<ListValue>(new ListValue())));
   extensions::ExtensionSystem::Get(profile_)->event_router()->
       BroadcastEvent(event.Pass());
 }
 
 void FileBrowserEventRouter::HandleFileWatchNotification(
     const base::FilePath& local_path, bool got_error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   base::AutoLock lock(lock_);

[hashimoto, 2013/04/08 11:10:26]

Can't we remove this AutoLock to eliminate |lock_|?

[satorux1, 2013/04/09 01:24:38]

oops. I thought I removed lock_ completely. Removed.

   WatcherMap::const_iterator iter = file_watchers_.find(local_path);
   if (iter == file_watchers_.end()) {
     return;
   }
   DispatchDirectoryChangeEvent(iter->second->GetVirtualPath(), got_error,
                                iter->second->GetExtensions());
 }
 
 void FileBrowserEventRouter::DispatchDirectoryChangeEvent(
     const base::FilePath& virtual_path,
@@ skipping 225 matching lines @@
                                                chromeos::MOUNT_TYPE_DEVICE);
   } else {
     notifications_->HideNotification(FileBrowserNotifications::FORMAT_START,
                                      device_path);
     notifications_->ShowNotification(FileBrowserNotifications::FORMAT_FAIL,
                                      device_path);
   }
 }
 
 FileBrowserEventRouter::FileWatcherExtensions::FileWatcherExtensions(
-    const base::FilePath& path, const std::string& extension_id,
+    const base::FilePath& virtual_path,
+    const std::string& extension_id,
     bool is_remote_file_system)
-    : ref_count_(0),
-      is_remote_file_system_(is_remote_file_system) {
-  if (!is_remote_file_system_)
-    file_watcher_.reset(new base::FilePathWatcher());
+    : file_watcher_(NULL),
+      virtual_path_(virtual_path),
+      ref_count_(0),
+      is_remote_file_system_(is_remote_file_system),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  virtual_path_ = path;
   AddExtension(extension_id);
 }
 
-FileBrowserEventRouter::FileWatcherExtensions::~FileWatcherExtensions() {}
+FileBrowserEventRouter::FileWatcherExtensions::~FileWatcherExtensions() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, file_watcher_);
+}
 
 void FileBrowserEventRouter::FileWatcherExtensions::AddExtension(
     const std::string& extension_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   ExtensionUsageRegistry::iterator it = extensions_.find(extension_id);
   if (it != extensions_.end()) {
     it->second++;
   } else {
     extensions_.insert(ExtensionUsageRegistry::value_type(extension_id, 1));
   }
 
   ref_count_++;
 }
 
 void FileBrowserEventRouter::FileWatcherExtensions::RemoveExtension(
     const std::string& extension_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   ExtensionUsageRegistry::iterator it = extensions_.find(extension_id);
 
   if (it != extensions_.end()) {
     // If entry found - decrease it's count and remove if necessary
     if (0 == it->second--) {
       extensions_.erase(it);
     }
 
     ref_count_--;
   } else {
     // Might be reference counting problem - e.g. if some component of
     // extension subscribes/unsubscribes correctly, but other component
     // only unsubscribes, developer of first one might receive this message
     LOG(FATAL) << " Extension [" << extension_id
         << "] tries to unsubscribe from folder [" << local_path_.value()
         << "] it isn't subscribed";
   }
 }
 
 const FileBrowserEventRouter::ExtensionUsageRegistry&
 FileBrowserEventRouter::FileWatcherExtensions::GetExtensions() const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return extensions_;
 }
 
 unsigned int
 FileBrowserEventRouter::FileWatcherExtensions::GetRefCount() const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return ref_count_;
 }
 
 const base::FilePath&
 FileBrowserEventRouter::FileWatcherExtensions::GetVirtualPath() const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   return virtual_path_;
 }
 
 drive::DriveFileSystemInterface*
 FileBrowserEventRouter::GetRemoteFileSystem() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DriveSystemService* system_service =
       DriveSystemServiceFactory::GetForProfile(profile_);
   return (system_service ? system_service->file_system() : NULL);
 }
 
-bool FileBrowserEventRouter::FileWatcherExtensions::Watch(
-    const base::FilePath& path,
-    const base::FilePathWatcher::Callback& callback) {
-  if (is_remote_file_system_)
-    return true;
+void FileBrowserEventRouter::FileWatcherExtensions::Watch(
+    const base::FilePath& local_path,
+    const base::FilePathWatcher::Callback& file_watcher_callback,
+    const BoolCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+  DCHECK(!file_watcher_);
 
-  return file_watcher_->Watch(path, false, callback);
+  local_path_ = local_path;  // For error message in RemoveExtension().
+
+  if (is_remote_file_system_) {
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+                                                base::Bind(callback, true));
+    return;
+  }
+
+  BrowserThread::PostTaskAndReplyWithResult(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&CreateAndStartFilePathWatcher,
+                 local_path,
+                 base::Bind(&RelayFileWatcherCallbackToUIThread,
+                            file_watcher_callback)),
+      base::Bind(
+          &FileBrowserEventRouter::FileWatcherExtensions::OnWatcherStarted,
+          weak_ptr_factory_.GetWeakPtr(),
+          callback));
 }
+
+void FileBrowserEventRouter::FileWatcherExtensions::OnWatcherStarted(
+    const BoolCallback& callback,
+    base::FilePathWatcher* file_watcher) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (file_watcher) {
+    file_watcher_ = file_watcher;
+    callback.Run(true);
+  } else {
+    callback.Run(false);
+  }
+}