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.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291214

R=flackr@chromium.org, mukai@chromium.org

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

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

11 files changed, 139 insertions, 66 deletions

diff --git a/athena/home/home_card_impl.cc b/athena/home/home_card_impl.cc
index 04b649a..042ff6c 100644
--- a/athena/home/home_card_impl.cc
+++ b/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;
 }
 
diff --git a/athena/main/athena_launcher.cc b/athena/main/athena_launcher.cc
index de4dbc7..3810c04 100644
--- a/athena/main/athena_launcher.cc
+++ b/athena/main/athena_launcher.cc
@@ -22,7 +22,6 @@
 #include "athena/wm/public/window_manager.h"
 #include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
-#include "content/public/browser/browser_thread.h"
 #include "ui/app_list/app_list_switches.h"
 #include "ui/aura/window_property.h"
 #include "ui/keyboard/keyboard_controller.h"
@@ -92,7 +91,8 @@ class AthenaViewsDelegate : public views::ViewsDelegate {
 };
 
 void StartAthenaEnv(aura::Window* root_window,
-                    athena::ScreenManagerDelegate* delegate) {
+                    athena::ScreenManagerDelegate* delegate,
+                    scoped_refptr<base::TaskRunner> file_runner) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
 
   // Force showing in the experimental app-list view.
@@ -113,9 +113,7 @@ void StartAthenaEnv(aura::Window* root_window,
   aura::client::SetVisibilityClient(root_window,
                                     env_state->visibility_client.get());
 
-  athena::SystemUI::Create(
-      content::BrowserThread::GetMessageLoopProxyForThread(
-          content::BrowserThread::FILE));
+  athena::SystemUI::Create(file_runner);
   athena::InputManager::Create()->OnRootWindowCreated(root_window);
   athena::ScreenManager::Create(delegate, root_window);
   athena::WindowManager::Create();
diff --git a/athena/main/athena_launcher.h b/athena/main/athena_launcher.h
index 66a853c..b912e9d 100644
--- a/athena/main/athena_launcher.h
+++ b/athena/main/athena_launcher.h
@@ -5,6 +5,12 @@
 #ifndef ATHENA_MAIN_ATHENA_LAUNCHER_H_
 #define ATHENA_MAIN_ATHENA_LAUNCHER_H_
 
+#include "base/memory/ref_counted.h"
+
+namespace base {
+class TaskRunner;
+}
+
 namespace aura {
 class Window;
 }
@@ -20,7 +26,8 @@ class ScreenManagerDelegate;
 
 // Starts/shuts down the athena shell environment.
 void StartAthenaEnv(aura::Window* root_window,
-                    ScreenManagerDelegate* screen_manager_delegate);
+                    ScreenManagerDelegate* screen_manager_delegate,
+                    scoped_refptr<base::TaskRunner> file_runner);
 
 void StartAthenaSessionWithContext(content::BrowserContext* context);
 
diff --git a/athena/main/athena_main.cc b/athena/main/athena_main.cc
index 87982cc..3d5408e 100644
--- a/athena/main/athena_main.cc
+++ b/athena/main/athena_main.cc
@@ -11,6 +11,7 @@
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "content/public/app/content_main.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/shell/app/shell_main_delegate.h"
 #include "extensions/shell/browser/shell_browser_main_delegate.h"
 #include "extensions/shell/browser/shell_content_browser_client.h"
@@ -94,7 +95,9 @@ class AthenaBrowserMainDelegate : public extensions::ShellBrowserMainDelegate {
     screen_manager_delegate_.reset(
         new AthenaScreenManagerDelegate(desktop_controller));
     athena::StartAthenaEnv(desktop_controller->host()->window(),
-                           screen_manager_delegate_.get());
+                           screen_manager_delegate_.get(),
+                           content::BrowserThread::GetMessageLoopProxyForThread(
+                               content::BrowserThread::FILE));
     athena::StartAthenaSessionWithContext(context);
   }
 
diff --git a/athena/system/device_socket_listener.cc b/athena/system/device_socket_listener.cc
index e041e44c..032b3f5 100644
--- a/athena/system/device_socket_listener.cc
+++ b/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
diff --git a/athena/system/device_socket_listener.h b/athena/system/device_socket_listener.h
index aa37701..6bdb649 100644
--- a/athena/system/device_socket_listener.h
+++ b/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();
diff --git a/athena/system/orientation_controller.cc b/athena/system/orientation_controller.cc
index 8996d7d..511e0ca 100644
--- a/athena/system/orientation_controller.cc
+++ b/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);
diff --git a/athena/system/orientation_controller.h b/athena/system/orientation_controller.h
index 3a92913..9388788 100644
--- a/athena/system/orientation_controller.h
+++ b/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_;
 
diff --git a/athena/system/system_ui_impl.cc b/athena/system/system_ui_impl.cc
index 39867ad..b3bacb5 100644
--- a/athena/system/system_ui_impl.cc
+++ b/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;
 }
 
diff --git a/athena/test/athena_test_helper.cc b/athena/test/athena_test_helper.cc
index 6a9121a..b65abc0 100644
--- a/athena/test/athena_test_helper.cc
+++ b/athena/test/athena_test_helper.cc
@@ -12,6 +12,7 @@
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/threading/thread.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "ui/app_list/app_list_switches.h"
 #include "ui/aura/client/aura_constants.h"
@@ -50,6 +51,9 @@ AthenaTestHelper::~AthenaTestHelper() {
 
 void AthenaTestHelper::SetUp(ui::ContextFactory* context_factory) {
   setup_called_ = true;
+  file_thread_.reset(new base::Thread("FileThread"));
+  base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
+  file_thread_->StartWithOptions(options);
 
   // Force showing in the experimental app-list view.
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -90,7 +94,8 @@ void AthenaTestHelper::SetUp(ui::ContextFactory* context_factory) {
   // Ensure width != height so tests won't confuse them.
   host()->SetBounds(gfx::Rect(host_size));
 
-  athena::StartAthenaEnv(root_window(), screen_manager_delegate_.get());
+  athena::StartAthenaEnv(root_window(), screen_manager_delegate_.get(),
+                         file_thread_->message_loop_proxy());
   athena::StartAthenaSession(new SampleActivityFactory(),
                              new TestAppModelBuilder());
 }
diff --git a/athena/test/athena_test_helper.h b/athena/test/athena_test_helper.h
index a2bff53..0e15599 100644
--- a/athena/test/athena_test_helper.h
+++ b/athena/test/athena_test_helper.h
@@ -11,6 +11,7 @@
 
 namespace base {
 class MessageLoopForUI;
+class Thread;
 }
 
 namespace ui {
@@ -68,6 +69,7 @@ class AthenaTestHelper {
   scoped_ptr<aura::client::FocusClient> focus_client_;
   scoped_ptr< ::wm::InputMethodEventFilter> input_method_filter_;
   scoped_ptr<ui::ScopedAnimationDurationScaleMode> zero_duration_mode_;
+  scoped_ptr<base::Thread> file_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(AthenaTestHelper);
 };