Observe UsbService from the FILE thread in DevicePermissionsManager.

As in the UsbEventRouter the UsbService must be observed from the FILE
thread (until bug 427985 is resolved, which will be soon). This change
fixes a DCHECK(CalledOnValidThread()) in UsbService::AddObserver when
permission to access an ephemeral (no serial number) device is added and
the DevicePermissionsManager starts listening for disconnection of that
device.

BUG=452652

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

Cr-Commit-Position: refs/heads/master@{#313548}

chrome/browser/extensions/api/device_permissions_manager_unittest.cc

@@ -223,7 +223,9 @@ TEST_F(DevicePermissionsManagerTest, SuspendExtension) {
   ASSERT_FALSE(FindEntry(device_permissions.get(), device3_).get());
 }
 
-TEST_F(DevicePermissionsManagerTest, DisconnectDevice) {
+// TODO(reillyg): Until crbug.com/427985 is resolved device removal
+// notifications are delivered asynchronously and so this test must be disabled.
+TEST_F(DevicePermissionsManagerTest, DISABLED_DisconnectDevice) {
   DevicePermissionsManager* manager =
       DevicePermissionsManager::Get(env_.profile());
   AllowUsbDevice(manager, extension_, device0_);

extensions/browser/api/device_permissions_manager.cc

@@ -4,12 +4,14 @@
 
 #include "extensions/browser/api/device_permissions_manager.h"
 
+#include "base/bind.h"
 #include "base/memory/singleton.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "content/public/browser/browser_thread.h"
 #include "device/core/device_client.h"
 #include "device/usb/usb_device.h"
 #include "device/usb/usb_ids.h"
@@ -24,7 +26,9 @@
 namespace extensions {
 
 using content::BrowserContext;
+using content::BrowserThread;
 using device::UsbDevice;
+using device::UsbService;
 using extensions::APIPermission;
 using extensions::Extension;
 using extensions::ExtensionHost;
@@ -377,6 +381,35 @@ DevicePermissions::DevicePermissions(const DevicePermissions* original)
       ephemeral_devices_(original->ephemeral_devices_) {
 }
 
+class DevicePermissionsManager::FileThreadHelper : public UsbService::Observer {
+ public:
+  FileThreadHelper(
+      base::WeakPtr<DevicePermissionsManager> device_permissions_manager)
+      : device_permissions_manager_(device_permissions_manager),
+        observer_(this) {}
+  virtual ~FileThreadHelper() {}
+
+  void Start() {
+    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+    UsbService* service = device::DeviceClient::Get()->GetUsbService();
+    if (service) {
+      observer_.Add(service);
+    }
+  }
+
+ private:
+  void OnDeviceRemoved(scoped_refptr<UsbDevice> device) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&DevicePermissionsManager::OnDeviceRemoved,
+                   device_permissions_manager_, device));
+  }
+
+  base::WeakPtr<DevicePermissionsManager> device_permissions_manager_;
+  ScopedObserver<UsbService, UsbService::Observer> observer_;
+};
+
 // static
 DevicePermissionsManager* DevicePermissionsManager::Get(
     BrowserContext* context) {
@@ -441,11 +474,13 @@ void DevicePermissionsManager::AllowUsbDevice(
     // Only start observing when an ephemeral device has been added so that
     // UsbService is not automatically initialized on profile creation (which it
     // would be if this call were in the constructor).
-    device::UsbService* usb_service =
-        device::DeviceClient::Get()->GetUsbService();
-    DCHECK(usb_service);
-    if (!usb_service_observer_.IsObserving(usb_service)) {
-      usb_service_observer_.Add(usb_service);
+    if (!helper_) {
+      helper_ = new FileThreadHelper(weak_factory_.GetWeakPtr());
+      // base::Unretained is safe because any task to delete helper_ will be
+      // executed after this call.
+      BrowserThread::PostTask(
+          BrowserThread::FILE, FROM_HERE,
+          base::Bind(&FileThreadHelper::Start, base::Unretained(helper_)));
     }
   }
 }
@@ -490,7 +525,8 @@ DevicePermissionsManager::DevicePermissionsManager(
     content::BrowserContext* context)
     : context_(context),
       process_manager_observer_(this),
-      usb_service_observer_(this) {
+      helper_(nullptr),
+      weak_factory_(this) {
   process_manager_observer_.Add(ProcessManager::Get(context));
 }
 
@@ -499,6 +535,10 @@ DevicePermissionsManager::~DevicePermissionsManager() {
     DevicePermissions* device_permissions = map_entry.second;
     delete device_permissions;
   }
+  if (helper_) {
+    BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, helper_);
+    helper_ = nullptr;
+  }
 }
 
 DevicePermissions* DevicePermissionsManager::Get(
@@ -540,6 +580,7 @@ void DevicePermissionsManager::OnBackgroundHostClose(
 
 void DevicePermissionsManager::OnDeviceRemoved(
     scoped_refptr<UsbDevice> device) {
+  DCHECK(CalledOnValidThread());
   for (const auto& map_entry : extension_id_to_device_permissions_) {
     // An ephemeral device cannot be identified if it is reconnected and so
     // permission to access it is cleared on disconnect.

extensions/browser/api/device_permissions_manager.h

@@ -18,6 +18,7 @@
 #include "base/threading/thread_checker.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "content/public/browser/browser_thread.h"
 #include "device/usb/usb_service.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/browser/process_manager_observer.h"
@@ -134,8 +135,7 @@ class DevicePermissions {
 // Manages saved device permissions for all extensions.
 class DevicePermissionsManager : public KeyedService,
                                  public base::NonThreadSafe,
-                                 public ProcessManagerObserver,
-                                 public device::UsbService::Observer {
+                                 public ProcessManagerObserver {
  public:
   static DevicePermissionsManager* Get(content::BrowserContext* context);
 
@@ -171,6 +171,8 @@ class DevicePermissionsManager : public KeyedService,
   void Clear(const std::string& extension_id);
 
  private:
+  class FileThreadHelper;
+
   friend class DevicePermissionsManagerFactory;
   FRIEND_TEST_ALL_PREFIXES(DevicePermissionsManagerTest, SuspendExtension);
 
@@ -179,19 +181,18 @@ class DevicePermissionsManager : public KeyedService,
 
   DevicePermissions* Get(const std::string& extension_id) const;
   DevicePermissions* GetOrInsert(const std::string& extension_id);
+  void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device);
 
   // ProcessManagerObserver implementation
   void OnBackgroundHostClose(const std::string& extension_id) override;
 
-  // device::UsbService::Observer implementation
-  void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override;
-
   content::BrowserContext* context_;
   std::map<std::string, DevicePermissions*> extension_id_to_device_permissions_;
   ScopedObserver<ProcessManager, ProcessManagerObserver>
       process_manager_observer_;
-  ScopedObserver<device::UsbService, device::UsbService::Observer>
-      usb_service_observer_;
+  FileThreadHelper* helper_;
+
+  base::WeakPtrFactory<DevicePermissionsManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DevicePermissionsManager);
 };