Fix usb event handler.

Currently the UsbEventHandler thread is not stopped gracefully and leads to crash upon browser exit.

BUG=223817
TEST=the new unit test in the CL.

Review URL: https://chromiumcodereview.appspot.com/19793009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@213232 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 63 insertions, 17 deletions

diff --git a/chrome/browser/usb/usb_service.cc b/chrome/browser/usb/usb_service.cc
index e5ed092..33c54dd 100644
--- a/chrome/browser/usb/usb_service.cc
+++ b/chrome/browser/usb/usb_service.cc
@@ -10,7 +10,9 @@
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/synchronization/waitable_event.h"
 #include "chrome/browser/usb/usb_device_handle.h"
+#include "third_party/libusb/src/libusb/interrupt.h"
 #include "third_party/libusb/src/libusb/libusb.h"
 
 #if defined(OS_CHROMEOS)
@@ -27,34 +29,38 @@ using std::vector;
 class UsbEventHandler : public base::PlatformThread::Delegate {
  public:
   explicit UsbEventHandler(PlatformUsbContext context)
-      : running_(true), context_(context) {
-    base::PlatformThread::CreateNonJoinable(0, this);
+      : running_(true),
+        context_(context),
+        thread_handle_(0),
+        started_event_(false, false) {
+    base::PlatformThread::Create(0, this, &thread_handle_);
+    started_event_.Wait();
   }
 
   virtual ~UsbEventHandler() {}
 
   virtual void ThreadMain() OVERRIDE {
     base::PlatformThread::SetName("UsbEventHandler");
-
-    DLOG(INFO) << "UsbEventHandler started.";
-    while (running_) {
+    VLOG(1) << "UsbEventHandler started.";
+    started_event_.Signal();
+    while (running_)
       libusb_handle_events(context_);
-    }
-    DLOG(INFO) << "UsbEventHandler shutting down.";
-    libusb_exit(context_);
-
-    delete this;
+    VLOG(1) << "UsbEventHandler shutting down.";
   }
 
   void Stop() {
     running_ = false;
+    base::subtle::MemoryBarrier();
+    libusb_interrupt_handle_event(context_);
+    base::PlatformThread::Join(thread_handle_);
   }
 
  private:
-  bool running_;
+  volatile bool running_;
   PlatformUsbContext context_;
-
-  DISALLOW_EVIL_CONSTRUCTORS(UsbEventHandler);
+  base::PlatformThreadHandle thread_handle_;
+  base::WaitableEvent started_event_;
+  DISALLOW_COPY_AND_ASSIGN(UsbEventHandler);
 };
 
 UsbService::UsbService() {
@@ -64,9 +70,11 @@ UsbService::UsbService() {
 
 UsbService::~UsbService() {}
 
-void UsbService::Cleanup() {
+void UsbService::Shutdown() {
   event_handler_->Stop();
-  event_handler_ = NULL;
+  delete event_handler_;
+  libusb_exit(context_);
+  context_ = NULL;
 }
 
 void UsbService::FindDevices(const uint16 vendor_id,
diff --git a/chrome/browser/usb/usb_service.h b/chrome/browser/usb/usb_service.h
index f91e28b..666a26c 100644
--- a/chrome/browser/usb/usb_service.h
+++ b/chrome/browser/usb/usb_service.h
@@ -27,9 +27,9 @@ class UsbService : public BrowserContextKeyedService {
   UsbService();
   virtual ~UsbService();
 
-  // Cleanup must be invoked before the service is destroyed. It interrupts the
+  // Shutdown must be invoked before the service is destroyed. It interrupts the
   // event handling thread and disposes of open devices.
-  void Cleanup();
+  virtual void Shutdown() OVERRIDE;
 
   // Find all of the devices attached to the system that are identified by
   // |vendor_id| and |product_id|, inserting them into |devices|. Clears
diff --git a/chrome/browser/usb/usb_service_unittest.cc b/chrome/browser/usb/usb_service_unittest.cc
new file mode 100644
index 0000000..b0eeb9b
--- /dev/null
+++ b/chrome/browser/usb/usb_service_unittest.cc
@@ -0,0 +1,34 @@
+// Copyright 2013 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/usb/usb_service.h"
+
+#include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+typedef testing::Test UsbServiceTest;
+
+#if defined(OS_LINUX)
+// Linux trybot does not support usb.
+#define MAYBE_GracefulShutdown DISABLED_GracefulShutdown
+#elif defined(OS_ANDROID)
+// Android build does not include usb support.
+#define MAYBE_GracefulShutdown DISABLED_GracefulShutdown
+#else
+#define MAYBE_GracefulShutdown GracefulShutdown
+#endif
+
+TEST_F(UsbServiceTest, MAYBE_GracefulShutdown) {
+  base::TimeTicks start = base::TimeTicks::Now();
+  scoped_ptr<UsbService> service(new UsbService());
+  service->Shutdown();
+  base::TimeDelta elapse = base::TimeTicks::Now() - start;
+  if (elapse > base::TimeDelta::FromSeconds(2)) {
+    FAIL();
+  }
+}
+
+}  // namespace
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 146523e..c59d4be 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -1601,6 +1601,7 @@
         'browser/upload_list_unittest.cc',
         'browser/chrome_content_browser_client_unittest.cc',
         'browser/undo/undo_manager_test.cc',
+        'browser/usb/usb_service_unittest.cc',
         'browser/user_style_sheet_watcher_unittest.cc',
         'browser/value_store/leveldb_value_store_unittest.cc',
         'browser/value_store/testing_value_store_unittest.cc',
@@ -2441,6 +2442,9 @@
 
             # The importer code is not used on Android.
             'common/importer/firefox_importer_utils_unittest.cc',
+
+            # USB service is not supported on Android.
+            'browser/usb/usb_service_unittest.cc',
           ],
           'sources/': [
             ['exclude', '^browser/captive_portal/'],