Reduce UsbDeviceInfoPtr storage in //chrome/browser/usb.

As UsbChooserContext stores all device infos available to the device
manager, other storages could be reduced by only keeping guid and
getting the details from UsbChooserContext by GetDeviceInfo() method.

Bug: 699790
Change-Id: I42f19a530fcf030cb896b3b26f218fe019b77e33
Reviewed-on: https://chromium-review.googlesource.com/c/1296313
Commit-Queue: Donna Wu <donna.wu@intel.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604906}

chrome/browser/usb/usb_browsertest.cc

@@ -78,7 +78,13 @@ class FakeUsbChooser : public WebUsbChooser {
   ~FakeUsbChooser() override {}
 
   void ShowChooser(std::unique_ptr<UsbChooserController> controller) override {
-    new FakeChooserView(std::move(controller));
+    // Device list initialization in UsbChooserController may completed before
+    // having a valid view in which case OnOptionsInitialized() has no chance to
+    // be triggered, so select the first option directly if options are ready.
+    if (controller->NumOptions())
+      controller->Select({0});
+    else
+      new FakeChooserView(std::move(controller));
   }
 
   base::WeakPtr<WebUsbChooser> GetWeakPtr() override {

chrome/browser/usb/usb_chooser_context.cc

@@ -49,24 +49,24 @@ bool CanStorePersistentEntry(const device::mojom::UsbDeviceInfo& device_info) {
   return !device_info.serial_number->empty();
 }
 
-base::DictionaryValue DeviceInfoToDictValue(
+std::unique_ptr<base::DictionaryValue> DeviceInfoToDictValue(
     const device::mojom::UsbDeviceInfo& device_info) {
-  base::DictionaryValue device_dict;
-  device_dict.SetKey(kDeviceNameKey,
-                     device_info.product_name
-                         ? base::Value(*device_info.product_name)
-                         : base::Value(""));
+  auto device_dict = std::make_unique<base::DictionaryValue>();
+  device_dict->SetKey(kDeviceNameKey,
+                      device_info.product_name
+                          ? base::Value(*device_info.product_name)
+                          : base::Value(""));
 
   if (!CanStorePersistentEntry(device_info)) {
-    device_dict.SetKey(kGuidKey, base::Value(device_info.guid));
+    device_dict->SetKey(kGuidKey, base::Value(device_info.guid));
     return device_dict;
   }
-  device_dict.SetKey(kVendorIdKey, base::Value(device_info.vendor_id));
-  device_dict.SetKey(kProductIdKey, base::Value(device_info.product_id));
-  device_dict.SetKey(kSerialNumberKey,
-                     device_info.serial_number
-                         ? base::Value(*device_info.serial_number)
-                         : base::Value(""));
+  device_dict->SetKey(kVendorIdKey, base::Value(device_info.vendor_id));
+  device_dict->SetKey(kProductIdKey, base::Value(device_info.product_id));
+  device_dict->SetKey(kSerialNumberKey,
+                      device_info.serial_number
+                          ? base::Value(*device_info.serial_number)
+                          : base::Value(""));
   return device_dict;
 }
 
@@ -96,11 +96,22 @@ UsbChooserContext::UsbChooserContext(Profile* profile)
 }
 
 void UsbChooserContext::InitDeviceList(
-    std::vector<::device::mojom::UsbDeviceInfoPtr> devices) {
+    std::vector<device::mojom::UsbDeviceInfoPtr> devices) {
   for (auto& device_info : devices) {
     DCHECK(device_info);
     devices_.insert(std::make_pair(device_info->guid, std::move(device_info)));
   }
+  is_initialized_ = true;
+
+  while (!pending_get_devices_requests_.empty()) {
+    std::vector<device::mojom::UsbDeviceInfoPtr> device_list;
+    for (const auto& entry : devices_) {
+      device_list.push_back(entry.second->Clone());
+    }
+    std::move(pending_get_devices_requests_.front())
+        .Run(std::move(device_list));
+    pending_get_devices_requests_.pop();
+  }
 }
 
 void UsbChooserContext::EnsureConnectionWithDeviceManager() {
@@ -147,9 +158,14 @@ UsbChooserContext::GetGrantedObjects(const GURL& requesting_origin,
         std::make_pair(requesting_origin, embedding_origin));
     if (it != ephemeral_devices_.end()) {
       for (const std::string& guid : it->second) {
-        auto dict_it = ephemeral_dicts_.find(guid);
-        DCHECK(dict_it != ephemeral_dicts_.end());
-        objects.push_back(dict_it->second.CreateDeepCopy());
+        // |devices_| should be initialized when |ephemeral_devices_| is filled.
+        // Because |ephemeral_devices_| is filled by GrantDevicePermission()
+        // which is called in UsbChooserController::Select(), this method will
+        // always be called after device initialization in UsbChooserController
+        // which always returns after the device list initialization in this
+        // class.
+        DCHECK(base::ContainsKey(devices_, guid));
+        objects.push_back(DeviceInfoToDictValue(*devices_[guid]));
       }
     }
   }
@@ -170,11 +186,9 @@ UsbChooserContext::GetAllGrantedObjects() {
       continue;
 
     for (const std::string& guid : map_entry.second) {
-      auto dict_it = ephemeral_dicts_.find(guid);
-      DCHECK(dict_it != ephemeral_dicts_.end());
-      // ChooserContextBase::Object constructor will swap the object, so
-      // a deep copy is needed here.
-      auto object = dict_it->second.CreateDeepCopy();
+      DCHECK(base::ContainsKey(devices_, guid));
+      // ChooserContextBase::Object constructor will swap the object.
+      auto object = DeviceInfoToDictValue(*devices_[guid]);
       objects.push_back(std::make_unique<ChooserContextBase::Object>(
           requesting_origin, embedding_origin, object.get(), "preference",
           is_incognito_));
@@ -215,15 +229,10 @@ void UsbChooserContext::GrantDevicePermission(
     const device::mojom::UsbDeviceInfo& device_info) {
   if (CanStorePersistentEntry(device_info)) {
     GrantObjectPermission(requesting_origin, embedding_origin,
-                          std::make_unique<base::DictionaryValue>(
-                              DeviceInfoToDictValue(device_info)));
+                          DeviceInfoToDictValue(device_info));
   } else {
     ephemeral_devices_[std::make_pair(requesting_origin, embedding_origin)]
         .insert(device_info.guid);
-    if (!base::ContainsKey(ephemeral_dicts_, device_info.guid)) {
-      ephemeral_dicts_.insert(
-          std::make_pair(device_info.guid, DeviceInfoToDictValue(device_info)));
-    }
   }
 }
 
@@ -271,8 +280,15 @@ bool UsbChooserContext::HasDevicePermission(
 
 void UsbChooserContext::GetDevices(
     device::mojom::UsbDeviceManager::GetDevicesCallback callback) {
-  EnsureConnectionWithDeviceManager();
-  device_manager_->GetDevices(nullptr, std::move(callback));
+  if (is_initialized_) {
+    std::vector<device::mojom::UsbDeviceInfoPtr> device_list;
+    for (const auto& pair : devices_) {
+      device_list.push_back(pair.second->Clone());
+    }
+    std::move(callback).Run(std::move(device_list));
+  } else {
+    pending_get_devices_requests_.push(std::move(callback));
+  }
 }
 
 void UsbChooserContext::GetDevice(
@@ -286,6 +302,7 @@ void UsbChooserContext::GetDevice(
 
 const device::mojom::UsbDeviceInfo* UsbChooserContext::GetDeviceInfo(
     const std::string& guid) {
+  DCHECK(is_initialized_);
   auto it = devices_.find(guid);
   return it == devices_.end() ? nullptr : it->second.get();
 }
@@ -343,16 +360,14 @@ void UsbChooserContext::OnDeviceRemoved(
 
   for (auto& map_entry : ephemeral_devices_)
     map_entry.second.erase(device_info->guid);
-
-  ephemeral_dicts_.erase(device_info->guid);
 }
 
 void UsbChooserContext::OnDeviceManagerConnectionError() {
   device_manager_.reset();
   client_binding_.Close();
   devices_.clear();
+  is_initialized_ = false;
   ephemeral_devices_.clear();
-  ephemeral_dicts_.clear();
 
   // Notify all observers.
   for (auto& observer : observer_list_)

chrome/browser/usb/usb_chooser_context.h

@@ -12,6 +12,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/containers/queue.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/values.h"
@@ -91,8 +92,11 @@ class UsbChooserContext : public ChooserContextBase,
   void SetUpDeviceManagerConnection();
 
   bool is_incognito_;
+  bool is_initialized_ = false;
+  base::queue<device::mojom::UsbDeviceManager::GetDevicesCallback>
+      pending_get_devices_requests_;
+
   std::map<std::pair<GURL, GURL>, std::set<std::string>> ephemeral_devices_;
-  std::map<std::string, base::DictionaryValue> ephemeral_dicts_;
   std::map<std::string, device::mojom::UsbDeviceInfoPtr> devices_;
 
   std::unique_ptr<UsbPolicyAllowedDevices> usb_policy_allowed_devices_;

chrome/browser/usb/usb_chooser_context_unittest.cc

@@ -4,6 +4,7 @@
 
 #include <vector>
 
+#include "base/bind_helpers.h"
 #include "base/json/json_reader.h"
 #include "base/run_loop.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ -36,6 +37,12 @@ class UsbChooserContextTest : public testing::Test {
     device::mojom::UsbDeviceManagerPtr device_manager_ptr;
     device_manager_.AddBinding(mojo::MakeRequest(&device_manager_ptr));
     chooser_context->SetDeviceManagerForTesting(std::move(device_manager_ptr));
+
+    // Call GetDevices once to make sure the connection with DeviceManager has
+    // been set up, so that it can be notified when device is removed.
+    chooser_context->GetDevices(
+        base::DoNothing::Once<std::vector<UsbDeviceInfoPtr>>());
+    base::RunLoop().RunUntilIdle();
     return chooser_context;
   }
 
@@ -46,18 +53,6 @@ class UsbChooserContextTest : public testing::Test {
   TestingProfile profile_;
 };
 
-void ExpectDevicesAndThen(
-    const std::set<std::string>& expected_guids,
-    base::OnceClosure continuation,
-    std::vector<device::mojom::UsbDeviceInfoPtr> results) {
-  EXPECT_EQ(expected_guids.size(), results.size());
-  std::set<std::string> actual_guids;
-  for (size_t i = 0; i < results.size(); ++i)
-    actual_guids.insert(results[i]->guid);
-  EXPECT_EQ(expected_guids, actual_guids);
-  std::move(continuation).Run();
-}
-
 }  // namespace
 
 TEST_F(UsbChooserContextTest, CheckGrantAndRevokePermission) {
@@ -138,16 +133,6 @@ TEST_F(UsbChooserContextTest, DisconnectDeviceWithPermission) {
       device_manager_.CreateAndAddDevice(0, 0, "Google", "Gizmo", "123ABC");
 
   UsbChooserContext* store = GetChooserContext(profile());
-  {
-    // Call GetDevices once to make sure the connection with DeviceManager has
-    // been set up, so that it can be notified when device is removed.
-    std::set<std::string> guids;
-    guids.insert(device_info->guid);
-    base::RunLoop loop;
-    store->GetDevices(
-        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
-    loop.Run();
-  }
 
   EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   store->GrantDevicePermission(origin, origin, *device_info);
@@ -185,16 +170,6 @@ TEST_F(UsbChooserContextTest, DisconnectDeviceWithEphemeralPermission) {
       device_manager_.CreateAndAddDevice(0, 0, "Google", "Gizmo", "");
 
   UsbChooserContext* store = GetChooserContext(profile());
-  {
-    // Call GetDevices once to make sure the connection with DeviceManager has
-    // been set up, so that it can be notified when device is removed.
-    std::set<std::string> guids;
-    guids.insert(device_info->guid);
-    base::RunLoop loop;
-    store->GetDevices(
-        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
-    loop.Run();
-  }
 
   EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
   store->GrantDevicePermission(origin, origin, *device_info);
@@ -228,14 +203,13 @@ TEST_F(UsbChooserContextTest, DisconnectDeviceWithEphemeralPermission) {
 
 TEST_F(UsbChooserContextTest, GrantPermissionInIncognito) {
   GURL origin("https://www.google.com");
-  UsbChooserContext* store = GetChooserContext(profile());
-  UsbChooserContext* incognito_store =
-      GetChooserContext(profile()->GetOffTheRecordProfile());
-
   UsbDeviceInfoPtr device_info_1 =
       device_manager_.CreateAndAddDevice(0, 0, "Google", "Gizmo", "");
   UsbDeviceInfoPtr device_info_2 =
       device_manager_.CreateAndAddDevice(0, 0, "Google", "Gizmo", "");
+  UsbChooserContext* store = GetChooserContext(profile());
+  UsbChooserContext* incognito_store =
+      GetChooserContext(profile()->GetOffTheRecordProfile());
 
   store->GrantDevicePermission(origin, origin, *device_info_1);
   EXPECT_TRUE(store->HasDevicePermission(origin, origin, *device_info_1));
@@ -377,11 +351,11 @@ TEST_F(UsbChooserContextTest,
   const std::vector<GURL> kInvalidRequestingOrigins = {
       GURL("https://gadget.com"), GURL("https://cool.com")};
 
-  auto* store = UsbChooserContextFactory::GetForProfile(profile());
-
   UsbDeviceInfoPtr specific_device_info = device_manager_.CreateAndAddDevice(
       1234, 5678, "Google", "Gizmo", "ABC123");
 
+  auto* store = GetChooserContext(profile());
+
   ExpectNoPermissions(store, *specific_device_info);
 
   profile()->GetPrefs()->Set(prefs::kManagedWebUsbAllowDevicesForUrls,
@@ -400,12 +374,12 @@ TEST_F(UsbChooserContextTest,
       GURL("https://product.vendor.com"), GURL("https://gadget.com"),
       GURL("https://cool.com")};
 
-  auto* store = UsbChooserContextFactory::GetForProfile(profile());
-
   UsbDeviceInfoPtr vendor_related_device_info =
       device_manager_.CreateAndAddDevice(1234, 8765, "Google", "Widget",
                                          "XYZ987");
 
+  auto* store = GetChooserContext(profile());
+
   ExpectNoPermissions(store, *vendor_related_device_info);
 
   profile()->GetPrefs()->Set(prefs::kManagedWebUsbAllowDevicesForUrls,
@@ -426,11 +400,11 @@ TEST_F(UsbChooserContextTest,
   const GURL kGadgetOrigin("https://gadget.com");
   const GURL& kCoolOrigin = kInvalidRequestingOrigins[2];
 
-  auto* store = UsbChooserContextFactory::GetForProfile(profile());
-
   UsbDeviceInfoPtr unrelated_device_info = device_manager_.CreateAndAddDevice(
       2468, 1357, "Cool", "Gadget", "4W350M3");
 
+  auto* store = GetChooserContext(profile());
+
   ExpectNoPermissions(store, *unrelated_device_info);
 
   profile()->GetPrefs()->Set(prefs::kManagedWebUsbAllowDevicesForUrls,
@@ -454,13 +428,13 @@ TEST_F(UsbChooserContextTest,
   const GURL kGadgetOrigin("https://gadget.com");
   const GURL kCoolOrigin("https://cool.com");
 
-  auto* store = UsbChooserContextFactory::GetForProfile(profile());
-
   UsbDeviceInfoPtr specific_device_info = device_manager_.CreateAndAddDevice(
       1234, 5678, "Google", "Gizmo", "ABC123");
   UsbDeviceInfoPtr unrelated_device_info = device_manager_.CreateAndAddDevice(
       2468, 1357, "Cool", "Gadget", "4W350M3");
 
+  auto* store = GetChooserContext(profile());
+
   ExpectNoPermissions(store, *specific_device_info);
   ExpectNoPermissions(store, *unrelated_device_info);
 

chrome/browser/usb/usb_chooser_controller.cc

@@ -113,20 +113,27 @@ base::string16 UsbChooserController::GetOption(size_t index) const {
   if (it->second == 1)
     return device_name;
 
+  if (!chooser_context_)
+    return device_name;
+
+  auto* device_info = chooser_context_->GetDeviceInfo(devices_[index].first);
+  if (!device_info || !device_info->serial_number)
+    return device_name;
+
   return l10n_util::GetStringFUTF16(IDS_DEVICE_CHOOSER_DEVICE_NAME_WITH_ID,
-                                    device_name,
-                                    devices_[index].first->serial_number
-                                        ? *devices_[index].first->serial_number
-                                        : base::string16());
+                                    device_name, *device_info->serial_number);
 }
 
 bool UsbChooserController::IsPaired(size_t index) const {
-  auto& device_info = *devices_[index].first;
   if (!chooser_context_)
     return false;
 
+  auto* device_info = chooser_context_->GetDeviceInfo(devices_[index].first);
+  if (!device_info)
+    return false;
+
   return chooser_context_->HasDevicePermission(requesting_origin_,
-                                               embedding_origin_, device_info);
+                                               embedding_origin_, *device_info);
 }
 
 void UsbChooserController::Select(const std::vector<size_t>& indices) {
@@ -134,15 +141,20 @@ void UsbChooserController::Select(const std::vector<size_t>& indices) {
   size_t index = indices[0];
   DCHECK_LT(index, devices_.size());
 
-  if (chooser_context_) {
-    chooser_context_->GrantDevicePermission(
-        requesting_origin_, embedding_origin_, *devices_[index].first);
+  if (!chooser_context_) {
+    // Return nullptr for GetPermissionCallback.
+    std::move(callback_).Run(nullptr);
+    return;
   }
 
-  std::move(callback_).Run(devices_[index].first.Clone());
+  auto* device_info = chooser_context_->GetDeviceInfo(devices_[index].first);
+  DCHECK(device_info);
+  chooser_context_->GrantDevicePermission(requesting_origin_, embedding_origin_,
+                                          *device_info);
+  std::move(callback_).Run(device_info->Clone());
 
   RecordWebUsbChooserClosure(
-      devices_[index].first->serial_number->empty()
+      device_info->serial_number->empty()
           ? WEBUSB_CHOOSER_CLOSED_EPHEMERAL_PERMISSION_GRANTED
           : WEBUSB_CHOOSER_CLOSED_PERMISSION_GRANTED);
 }
@@ -166,7 +178,7 @@ void UsbChooserController::OnDeviceAdded(
     const device::mojom::UsbDeviceInfo& device_info) {
   if (DisplayDevice(device_info)) {
     base::string16 device_name = FormatUsbDeviceName(device_info);
-    devices_.push_back(std::make_pair(device_info.Clone(), device_name));
+    devices_.push_back(std::make_pair(device_info.guid, device_name));
     ++device_name_map_[device_name];
     if (view())
       view()->OnOptionAdded(devices_.size() - 1);
@@ -176,7 +188,7 @@ void UsbChooserController::OnDeviceAdded(
 void UsbChooserController::OnDeviceRemoved(
     const device::mojom::UsbDeviceInfo& device_info) {
   for (auto it = devices_.begin(); it != devices_.end(); ++it) {
-    if (it->first->guid == device_info.guid) {
+    if (it->first == device_info.guid) {
       size_t index = it - devices_.begin();
       DCHECK_GT(device_name_map_[it->second], 0);
       if (--device_name_map_[it->second] == 0)
@@ -201,7 +213,7 @@ void UsbChooserController::GotUsbDeviceList(
     DCHECK(device_info);
     if (DisplayDevice(*device_info)) {
       base::string16 device_name = FormatUsbDeviceName(*device_info);
-      devices_.push_back(std::make_pair(std::move(device_info), device_name));
+      devices_.push_back(std::make_pair(device_info->guid, device_name));
       ++device_name_map_[device_name];
     }
   }

chrome/browser/usb/usb_chooser_controller.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_USB_USB_CHOOSER_CONTROLLER_H_
 #define CHROME_BROWSER_USB_USB_CHOOSER_CONTROLLER_H_
 
+#include <string>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -65,9 +66,8 @@ class UsbChooserController : public ChooserController,
   base::WeakPtr<UsbChooserContext> chooser_context_;
   ScopedObserver<UsbChooserContext, UsbChooserContext::Observer> observer_;
 
-  // Each pair is a (device, device name).
-  std::vector<std::pair<device::mojom::UsbDeviceInfoPtr, base::string16>>
-      devices_;
+  // Each pair is a (device guid, device name).
+  std::vector<std::pair<std::string, base::string16>> devices_;
   // Maps from device name to number of devices.
   std::unordered_map<base::string16, int> device_name_map_;
   base::WeakPtrFactory<UsbChooserController> weak_factory_;

chrome/browser/usb/web_usb_service_impl.h

@@ -6,7 +6,9 @@
 #define CHROME_BROWSER_USB_WEB_USB_SERVICE_IMPL_H_
 
 #include <map>
+#include <memory>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #include "base/macros.h"

chrome/browser/usb/web_usb_service_impl_unittest.cc

@@ -297,6 +297,15 @@ TEST_F(WebUsbServiceImplTest, RevokeDevicePermission) {
 
   WebUsbServicePtr web_usb_service;
   ConnectToService(mojo::MakeRequest(&web_usb_service));
+  base::RunLoop().RunUntilIdle();
+  {
+    std::set<std::string> guids;
+    base::RunLoop loop;
+    web_usb_service->GetDevices(
+        base::BindOnce(&ExpectDevicesAndThen, guids, loop.QuitClosure()));
+    loop.Run();
+  }
+
   context->GrantDevicePermission(origin, origin, *device_info);
 
   device::mojom::UsbDevicePtr device_ptr;