Fixes three crashes

 * AppListViewDelegate was accessing deleted search_provider.
 * DeviceSocketListener::StopListening can happen after DeviceSocketManager is deleted
 * Explicitly delete FilePathWatcher. This was causing
   recursive callback to FilePathWatcherImpl::Cancel from
   FilePathWatcherImpl::CancelOnMessageLoopThread, which
   caused crash.


Clean ups
 * change OnIO to OnFILE as they run on FILE thread.
 * removed unused singleton related code.

BUG=None
TEST=Run athena_main on desktop and close window.

Review URL: https://codereview.chromium.org/490033003

Cr-Commit-Position: refs/heads/master@{#291214}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@291214 0039d316-1c4b-4281-b951-d872f2087c98

athena/home/home_card_impl.cc

@@ -486,6 +486,11 @@ HomeCardImpl::~HomeCardImpl() {
   if (activation_client_)
     activation_client_->RemoveObserver(this);
   home_card_widget_->CloseNow();
+
+  // Reset the view delegate first as it access search provider during
+  // shutdown.
+  view_delegate_.reset();
+  search_provider_.reset();
   instance = NULL;
 }
 

athena/system/device_socket_listener.cc

@@ -12,7 +12,6 @@
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
-#include "base/memory/singleton.h"
 #include "base/message_loop/message_loop.h"
 #include "base/observer_list.h"
 #include "base/stl_util.h"
@@ -55,17 +54,22 @@ DeviceSocketManager* device_socket_manager_instance_ = NULL;
 // A singleton instance for managing all connections to sockets.
 class DeviceSocketManager {
  public:
-  static void Create(scoped_refptr<base::TaskRunner> io_task_runner) {
-    device_socket_manager_instance_ =
-        new DeviceSocketManager(io_task_runner);
+  static void Create(scoped_refptr<base::TaskRunner> file_task_runner) {
+    device_socket_manager_instance_ = new DeviceSocketManager(file_task_runner);
   }
 
   static void Shutdown() {
     CHECK(device_socket_manager_instance_);
-    delete device_socket_manager_instance_;
+    device_socket_manager_instance_->ScheduleDelete();
+    // Once scheduled to be deleted, no-one should be
+    // able to access it.
     device_socket_manager_instance_ = NULL;
   }
 
+  static DeviceSocketManager* GetInstanceUnsafe() {
+    return device_socket_manager_instance_;
+  }
+
   static DeviceSocketManager* GetInstance() {
     CHECK(device_socket_manager_instance_);
     return device_socket_manager_instance_;
@@ -94,8 +98,6 @@ class DeviceSocketManager {
   void OnEOF(const std::string& socket_path);
 
  private:
-  friend struct DefaultSingletonTraits<DeviceSocketManager>;
-
   struct SocketData {
     SocketData()
         : fd(-1) {
@@ -107,26 +109,29 @@ class DeviceSocketManager {
     scoped_ptr<DeviceSocketReader> watcher;
   };
 
-  DeviceSocketManager(scoped_refptr<base::TaskRunner> io_task_runner)
-      : io_task_runner_(io_task_runner) {
-  }
+  static void DeleteOnFILE(DeviceSocketManager* manager) { delete manager; }
+
+  DeviceSocketManager(scoped_refptr<base::TaskRunner> file_task_runner)
+      : file_task_runner_(file_task_runner) {}
 
   ~DeviceSocketManager() {
     STLDeleteContainerPairSecondPointers(socket_data_.begin(),
                                          socket_data_.end());
   }
 
-  void StartListeningOnIO(const std::string& socket_path,
-                          size_t data_size,
-                          DeviceSocketListener* listener);
+  void ScheduleDelete();
+
+  void StartListeningOnFILE(const std::string& socket_path,
+                            size_t data_size,
+                            DeviceSocketListener* listener);
 
-  void StopListeningOnIO(const std::string& socket_path,
-                         DeviceSocketListener* listener);
+  void StopListeningOnFILE(const std::string& socket_path,
+                           DeviceSocketListener* listener);
 
   void CloseSocket(const std::string& socket_path);
 
   std::map<std::string, SocketData*> socket_data_;
-  scoped_refptr<base::TaskRunner> io_task_runner_;
+  scoped_refptr<base::TaskRunner> file_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceSocketManager);
 };
@@ -162,23 +167,31 @@ void DeviceSocketReader::OnFileCanWriteWithoutBlocking(int fd) {
 void DeviceSocketManager::StartListening(const std::string& socket_path,
                                          size_t data_size,
                                          DeviceSocketListener* listener) {
-  io_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&DeviceSocketManager::StartListeningOnIO,
-                 base::Unretained(this), socket_path, data_size, listener));
+  file_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DeviceSocketManager::StartListeningOnFILE,
+                 base::Unretained(this),
+                 socket_path,
+                 data_size,
+                 listener));
 }
 
 void DeviceSocketManager::StopListening(const std::string& socket_path,
                                         DeviceSocketListener* listener) {
-  io_task_runner_->PostTask(FROM_HERE,
-      base::Bind(&DeviceSocketManager::StopListeningOnIO,
-                 base::Unretained(this), socket_path, listener));
+  file_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DeviceSocketManager::StopListeningOnFILE,
+                 base::Unretained(this),
+                 socket_path,
+                 listener));
 }
 
 void DeviceSocketManager::OnDataAvailable(const std::string& socket_path,
                                           const void* data) {
   CHECK_GT(socket_data_.count(socket_path), 0UL);
   DeviceSocketListeners& listeners = socket_data_[socket_path]->observers;
-  FOR_EACH_OBSERVER(DeviceSocketListener, listeners, OnDataAvailableOnIO(data));
+  FOR_EACH_OBSERVER(
+      DeviceSocketListener, listeners, OnDataAvailableOnFILE(data));
 }
 
 void DeviceSocketManager::CloseSocket(const std::string& socket_path) {
@@ -202,10 +215,10 @@ void DeviceSocketManager::OnEOF(const std::string& socket_path) {
   CloseSocket(socket_path);
 }
 
-void DeviceSocketManager::StartListeningOnIO(const std::string& socket_path,
-                                             size_t data_size,
-                                             DeviceSocketListener* listener) {
-  CHECK(io_task_runner_->RunsTasksOnCurrentThread());
+void DeviceSocketManager::StartListeningOnFILE(const std::string& socket_path,
+                                               size_t data_size,
+                                               DeviceSocketListener* listener) {
+  CHECK(file_task_runner_->RunsTasksOnCurrentThread());
   SocketData* socket_data = NULL;
   if (!socket_data_.count(socket_path)) {
     int socket_fd = -1;
@@ -237,12 +250,12 @@ void DeviceSocketManager::StartListeningOnIO(const std::string& socket_path,
   socket_data->observers.AddObserver(listener);
 }
 
-void DeviceSocketManager::StopListeningOnIO(const std::string& socket_path,
-                                            DeviceSocketListener* listener) {
+void DeviceSocketManager::StopListeningOnFILE(const std::string& socket_path,
+                                              DeviceSocketListener* listener) {
   if (!socket_data_.count(socket_path))
     return;  // Happens if unable to create a socket.
 
-  CHECK(io_task_runner_->RunsTasksOnCurrentThread());
+  CHECK(file_task_runner_->RunsTasksOnCurrentThread());
   DeviceSocketListeners& listeners = socket_data_[socket_path]->observers;
   listeners.RemoveObserver(listener);
   if (!listeners.might_have_observers()) {
@@ -251,6 +264,14 @@ void DeviceSocketManager::StopListeningOnIO(const std::string& socket_path,
   }
 }
 
+void DeviceSocketManager::ScheduleDelete() {
+  // Schedule a task to delete on FILE thread because
+  // there may be a task scheduled on |file_task_runner_|.
+  file_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DeleteOnFILE, base::Unretained(this)));
+}
+
 }  // namespace
 
 DeviceSocketListener::DeviceSocketListener(const std::string& socket_path,
@@ -265,8 +286,8 @@ DeviceSocketListener::~DeviceSocketListener() {
 
 // static
 void DeviceSocketListener::CreateSocketManager(
-    scoped_refptr<base::TaskRunner> io_task_runner) {
-  DeviceSocketManager::Create(io_task_runner);
+    scoped_refptr<base::TaskRunner> file_task_runner) {
+  DeviceSocketManager::Create(file_task_runner);
 }
 
 // static
@@ -281,7 +302,9 @@ void DeviceSocketListener::StartListening() {
 }
 
 void DeviceSocketListener::StopListening() {
-  DeviceSocketManager::GetInstance()->StopListening(socket_path_, this);
+  DeviceSocketManager* instance = DeviceSocketManager::GetInstanceUnsafe();
+  if (instance)
+    instance->StopListening(socket_path_, this);
 }
 
 }  // namespace athena

athena/system/device_socket_listener.h

@@ -27,10 +27,10 @@ class DeviceSocketListener {
       scoped_refptr<base::TaskRunner> io_task_runner);
   static void ShutdownSocketManager();
 
-  // This is called on the IO thread when data is available on the socket.
+  // This is called on the FILE thread when data is available on the socket.
   // |data| is guaranteed to be of exactly |data_size| length. Note that the
   // implementation must not mutate or free the data.
-  virtual void OnDataAvailableOnIO(const void* data) = 0;
+  virtual void OnDataAvailableOnFILE(const void* data) = 0;
 
  protected:
   void StartListening();

athena/system/orientation_controller.cc

@@ -58,21 +58,37 @@ struct DeviceSensorEvent {
 
 OrientationController::OrientationController()
     : DeviceSocketListener(kSocketPath, sizeof(DeviceSensorEvent)),
-      last_orientation_change_time_(0) {
+      last_orientation_change_time_(0),
+      shutdown_(false) {
   CHECK(base::MessageLoopForUI::current());
   ui_task_runner_ = base::MessageLoopForUI::current()->task_runner();
 }
 
 void OrientationController::InitWith(
-    scoped_refptr<base::TaskRunner> io_task_runner) {
-  io_task_runner->PostTask(FROM_HERE, base::Bind(
-      &OrientationController::WatchForSocketPathOnIO, this));
+    scoped_refptr<base::TaskRunner> file_task_runner) {
+  file_task_runner_ = file_task_runner;
+  file_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&OrientationController::WatchForSocketPathOnFILE, this));
 }
 
 OrientationController::~OrientationController() {
 }
 
-void OrientationController::WatchForSocketPathOnIO() {
+void OrientationController::Shutdown() {
+  CHECK(file_task_runner_);
+  StopListening();
+  file_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OrientationController::ShutdownOnFILE, this));
+}
+
+void OrientationController::ShutdownOnFILE() {
+  shutdown_ = true;
+  watcher_.reset();
+}
+
+void OrientationController::WatchForSocketPathOnFILE() {
   CHECK(base::MessageLoopForIO::current());
   if (base::PathExists(base::FilePath(kSocketPath))) {
     ui_task_runner_->PostTask(FROM_HERE,
@@ -80,22 +96,23 @@ void OrientationController::WatchForSocketPathOnIO() {
   } else {
     watcher_.reset(new base::FilePathWatcher);
     watcher_->Watch(
-        base::FilePath(kSocketPath), false,
-        base::Bind(&OrientationController::OnFilePathChangedOnIO, this));
+        base::FilePath(kSocketPath),
+        false,
+        base::Bind(&OrientationController::OnFilePathChangedOnFILE, this));
   }
 }
 
-void OrientationController::OnFilePathChangedOnIO(const base::FilePath& path,
-                                                  bool error) {
+void OrientationController::OnFilePathChangedOnFILE(const base::FilePath& path,
+                                                    bool error) {
   watcher_.reset();
-  if (error)
+  if (error || shutdown_)
     return;
 
   ui_task_runner_->PostTask(FROM_HERE,
       base::Bind(&OrientationController::StartListening, this));
 }
 
-void OrientationController::OnDataAvailableOnIO(const void* data) {
+void OrientationController::OnDataAvailableOnFILE(const void* data) {
   const DeviceSensorEvent* event =
       static_cast<const DeviceSensorEvent*>(data);
   if (event->type != SENSOR_ACCELEROMETER)
@@ -131,7 +148,7 @@ void OrientationController::OnDataAvailableOnIO(const void* data) {
 
 void OrientationController::RotateOnUI(gfx::Display::Rotation rotation) {
   ScreenManager* screen_manager = ScreenManager::Get();
-  // Since this is called from the IO thread, the screen manager may no longer
+  // Since this is called from the FILE thread, the screen manager may no longer
   // exist.
   if (screen_manager)
     screen_manager->SetRotation(rotation);

athena/system/orientation_controller.h

@@ -28,19 +28,22 @@ class OrientationController
  public:
   OrientationController();
 
-  void InitWith(scoped_refptr<base::TaskRunner> io_task_runner);
+  void InitWith(scoped_refptr<base::TaskRunner> file_task_runner);
+
+  void Shutdown();
 
  private:
   friend class base::RefCountedThreadSafe<OrientationController>;
 
   virtual ~OrientationController();
 
-  // Watch for the socket path to be created, called on the IO thread.
-  void WatchForSocketPathOnIO();
-  void OnFilePathChangedOnIO(const base::FilePath& path, bool error);
+  void ShutdownOnFILE();
+  // Watch for the socket path to be created, called on the FILE thread.
+  void WatchForSocketPathOnFILE();
+  void OnFilePathChangedOnFILE(const base::FilePath& path, bool error);
 
   // Overridden from device::DeviceSocketListener:
-  virtual void OnDataAvailableOnIO(const void* data) OVERRIDE;
+  virtual void OnDataAvailableOnFILE(const void* data) OVERRIDE;
 
   // Rotates the display to |rotation|, called on the UI thread.
   void RotateOnUI(gfx::Display::Rotation rotation);
@@ -51,9 +54,17 @@ class OrientationController
   // The timestamp of the last applied orientation change.
   int64_t last_orientation_change_time_;
 
+  // True if the OrientaionController has already been shutdown.
+  // This is initialized on UI thread, but must be accessed / modified
+  // only on FILE thread.
+  bool shutdown_;
+
   // A task runner for the UI thread.
   scoped_refptr<base::TaskRunner> ui_task_runner_;
 
+  // A task runner for the FILE thread.
+  scoped_refptr<base::TaskRunner> file_task_runner_;
+
   // File path watcher used to detect when sensors are present.
   scoped_ptr<base::FilePathWatcher> watcher_;
 

athena/system/system_ui_impl.cc

@@ -18,13 +18,16 @@ SystemUI* instance = NULL;
 
 class SystemUIImpl : public SystemUI {
  public:
-  SystemUIImpl(scoped_refptr<base::TaskRunner> io_task_runner)
+  SystemUIImpl(scoped_refptr<base::TaskRunner> file_task_runner)
       : orientation_controller_(new OrientationController()),
         power_button_controller_(new PowerButtonController) {
-    orientation_controller_->InitWith(io_task_runner);
+    orientation_controller_->InitWith(file_task_runner);
   }
 
   virtual ~SystemUIImpl() {
+    // Stops file watching now if exists. Waiting until message loop shutdon
+    // leads to FilePathWatcher crash.
+    orientation_controller_->Shutdown();
   }
 
  private:
@@ -37,10 +40,9 @@ class SystemUIImpl : public SystemUI {
 }  // namespace
 
 // static
-SystemUI* SystemUI::Create(
-    scoped_refptr<base::TaskRunner> io_task_runner) {
-  DeviceSocketListener::CreateSocketManager(io_task_runner);
-  instance = new SystemUIImpl(io_task_runner);
+SystemUI* SystemUI::Create(scoped_refptr<base::TaskRunner> file_task_runner) {
+  DeviceSocketListener::CreateSocketManager(file_task_runner);
+  instance = new SystemUIImpl(file_task_runner);
   return instance;
 }