usb: Add DCHECKs for UsbChooserContext observers
This CL updates UsbChooserContext to require that DeviceObservers
stop observing when the UsbChooserContext is about to be destroyed.
Change-Id: I848bec9c8d5649ed17477f1e9ec767405f13ea29
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3759869
Commit-Queue: Matt Reynolds <[email protected]>
Reviewed-by: Reilly Grant <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1030860}
diff --git a/chrome/browser/usb/chrome_usb_delegate.cc b/chrome/browser/usb/chrome_usb_delegate.cc
index 7aedd97..f61b9a75 100644
--- a/chrome/browser/usb/chrome_usb_delegate.cc
+++ b/chrome/browser/usb/chrome_usb_delegate.cc
@@ -225,6 +225,11 @@
observer.OnDeviceManagerConnectionError();
}
+void ChromeUsbDelegate::OnBrowserContextShutdown() {
+ device_observation_.Reset();
+ permission_observation_.Reset();
+}
+
bool ChromeUsbDelegate::HasDevicePermission(
RenderFrameHost& frame,
const device::mojom::UsbDeviceInfo& device) {
diff --git a/chrome/browser/usb/chrome_usb_delegate.h b/chrome/browser/usb/chrome_usb_delegate.h
index 047cc23..09d3fd1 100644
--- a/chrome/browser/usb/chrome_usb_delegate.h
+++ b/chrome/browser/usb/chrome_usb_delegate.h
@@ -68,6 +68,7 @@
void OnDeviceAdded(const device::mojom::UsbDeviceInfo&) override;
void OnDeviceRemoved(const device::mojom::UsbDeviceInfo&) override;
void OnDeviceManagerConnectionError() override;
+ void OnBrowserContextShutdown() override;
private:
base::ScopedObservation<UsbChooserContext,
diff --git a/chrome/browser/usb/usb_chooser_context.cc b/chrome/browser/usb/usb_chooser_context.cc
index 72cace0..84e48e4d 100644
--- a/chrome/browser/usb/usb_chooser_context.cc
+++ b/chrome/browser/usb/usb_chooser_context.cc
@@ -237,6 +237,11 @@
UsbChooserContext::~UsbChooserContext() {
OnDeviceManagerConnectionError();
+ for (auto& observer : device_observer_list_) {
+ observer.OnBrowserContextShutdown();
+ DCHECK(!device_observer_list_.HasObserver(&observer));
+ }
+ DCHECK(permission_observer_list_.empty());
}
std::vector<std::unique_ptr<permissions::ObjectPermissionContextBase::Object>>
diff --git a/chrome/browser/usb/usb_chooser_context.h b/chrome/browser/usb/usb_chooser_context.h
index 725ce009..55fd657 100644
--- a/chrome/browser/usb/usb_chooser_context.h
+++ b/chrome/browser/usb/usb_chooser_context.h
@@ -45,6 +45,10 @@
virtual void OnDeviceAdded(const device::mojom::UsbDeviceInfo&);
virtual void OnDeviceRemoved(const device::mojom::UsbDeviceInfo&);
virtual void OnDeviceManagerConnectionError();
+
+ // Called when the BrowserContext is shutting down. Observers must remove
+ // themselves before returning.
+ virtual void OnBrowserContextShutdown() = 0;
};
static base::Value DeviceInfoToValue(
diff --git a/chrome/browser/usb/usb_chooser_context_mock_device_observer.h b/chrome/browser/usb/usb_chooser_context_mock_device_observer.h
index 6adb667..c697a6f 100644
--- a/chrome/browser/usb/usb_chooser_context_mock_device_observer.h
+++ b/chrome/browser/usb/usb_chooser_context_mock_device_observer.h
@@ -17,6 +17,7 @@
MOCK_METHOD1(OnDeviceAdded, void(const device::mojom::UsbDeviceInfo&));
MOCK_METHOD1(OnDeviceRemoved, void(const device::mojom::UsbDeviceInfo&));
MOCK_METHOD0(OnDeviceManagerConnectionError, void());
+ MOCK_METHOD0(OnBrowserContextShutdown, void());
};
#endif // CHROME_BROWSER_USB_USB_CHOOSER_CONTEXT_MOCK_DEVICE_OBSERVER_H_
diff --git a/chrome/browser/usb/usb_chooser_context_unittest.cc b/chrome/browser/usb/usb_chooser_context_unittest.cc
index 92fbdd2..ce3bf946a3 100644
--- a/chrome/browser/usb/usb_chooser_context_unittest.cc
+++ b/chrome/browser/usb/usb_chooser_context_unittest.cc
@@ -5,11 +5,13 @@
#include <vector>
#include "base/callback_helpers.h"
+#include "base/containers/flat_map.h"
#include "base/json/json_reader.h"
#include "base/no_destructor.h"
#include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/bind.h"
+#include "base/test/test_future.h"
#include "base/values.h"
#include "build/chromeos_buildflags.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
@@ -30,7 +32,8 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
-using device::mojom::UsbDeviceInfoPtr;
+using ::base::test::TestFuture;
+using ::device::mojom::UsbDeviceInfoPtr;
using ::testing::_;
using ::testing::AnyNumber;
using ::testing::NiceMock;
@@ -48,15 +51,19 @@
public:
UsbChooserContextTest() {}
~UsbChooserContextTest() override {
- // When UsbChooserContext is destroyed, OnDeviceManagerConnectionError
- // should be called on the observers and OnPermissionRevoked will be called
- // for any ephemeral device permissions that are active.
- EXPECT_CALL(mock_device_observer_, OnDeviceManagerConnectionError())
- .Times(AnyNumber());
- EXPECT_CALL(mock_permission_observer_, OnPermissionRevoked(_))
- .Times(AnyNumber());
- EXPECT_CALL(mock_permission_observer_, OnObjectPermissionChanged(_, _))
- .Times(AnyNumber());
+ // When UsbChooserContext is destroyed, OnDeviceManagerConnectionError will
+ // be called for each device observer.
+ for (const auto& entry : mock_device_observers_) {
+ EXPECT_CALL(*entry.second, OnDeviceManagerConnectionError)
+ .Times(AnyNumber());
+ }
+
+ // OnPermissionRevoked and OnObjectPermissionChanged will be called for any
+ // ephemeral device permissions that are active.
+ for (const auto& entry : mock_permission_observers_) {
+ EXPECT_CALL(*entry.second, OnPermissionRevoked).Times(AnyNumber());
+ EXPECT_CALL(*entry.second, OnObjectPermissionChanged).Times(AnyNumber());
+ }
}
protected:
@@ -71,21 +78,42 @@
// 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());
- base::RunLoop().RunUntilIdle();
+ TestFuture<std::vector<device::mojom::UsbDeviceInfoPtr>> devices_future;
+ chooser_context->GetDevices(devices_future.GetCallback());
+ EXPECT_TRUE(devices_future.Wait());
// Add observers
+ EXPECT_FALSE(base::Contains(mock_permission_observers_, profile));
+ EXPECT_FALSE(base::Contains(mock_device_observers_, profile));
+ mock_permission_observers_.emplace(
+ profile,
+ std::make_unique<NiceMock<permissions::MockPermissionObserver>>());
+ mock_device_observers_.emplace(profile,
+ std::make_unique<MockDeviceObserver>());
+ NiceMock<permissions::MockPermissionObserver>* permission_observer =
+ mock_permission_observers_[profile].get();
chooser_context->permissions::ObjectPermissionContextBase::AddObserver(
- &mock_permission_observer_);
- chooser_context->AddObserver(&mock_device_observer_);
+ permission_observer);
+ MockDeviceObserver* device_observer = mock_device_observers_[profile].get();
+ chooser_context->AddObserver(device_observer);
+ EXPECT_CALL(*device_observer, OnBrowserContextShutdown)
+ .WillOnce([chooser_context, permission_observer, device_observer]() {
+ chooser_context
+ ->permissions::ObjectPermissionContextBase::RemoveObserver(
+ permission_observer);
+ chooser_context->RemoveObserver(device_observer);
+ });
return chooser_context;
}
device::FakeUsbDeviceManager device_manager_;
// Mock observers
- NiceMock<permissions::MockPermissionObserver> mock_permission_observer_;
- MockDeviceObserver mock_device_observer_;
+ base::flat_map<Profile*,
+ std::unique_ptr<NiceMock<permissions::MockPermissionObserver>>>
+ mock_permission_observers_;
+ base::flat_map<Profile*, std::unique_ptr<MockDeviceObserver>>
+ mock_device_observers_;
private:
content::BrowserTaskEnvironment task_environment_;
@@ -108,7 +136,7 @@
object.SetStringKey(kSerialNumberKey, "123ABC");
EXPECT_FALSE(store->HasDevicePermission(origin, *device_info));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -127,11 +155,12 @@
EXPECT_EQ(object, all_origin_objects[0]->value);
EXPECT_FALSE(all_origin_objects[0]->incognito);
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
- EXPECT_CALL(mock_permission_observer_, OnPermissionRevoked(origin));
+ EXPECT_CALL(*mock_permission_observers_[profile()],
+ OnPermissionRevoked(origin));
store->RevokeObjectPermission(origin, objects[0]->value);
EXPECT_FALSE(store->HasDevicePermission(origin, *device_info));
@@ -160,7 +189,7 @@
object.SetIntKey(kProductIdKey, device_info->product_id);
EXPECT_FALSE(store->HasDevicePermission(origin, *device_info));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -181,11 +210,12 @@
EXPECT_EQ(object, all_origin_objects[0]->value);
EXPECT_FALSE(all_origin_objects[0]->incognito);
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
- EXPECT_CALL(mock_permission_observer_, OnPermissionRevoked(origin));
+ EXPECT_CALL(*mock_permission_observers_[profile()],
+ OnPermissionRevoked(origin));
store->RevokeObjectPermission(origin, objects[0]->value);
EXPECT_FALSE(store->HasDevicePermission(origin, *device_info));
@@ -205,7 +235,7 @@
UsbChooserContext* store = GetChooserContext(profile());
EXPECT_FALSE(store->HasDevicePermission(origin, *device_info));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -221,7 +251,7 @@
store->GetAllGrantedObjects();
EXPECT_EQ(1u, all_origin_objects.size());
- EXPECT_CALL(mock_device_observer_, OnDeviceRemoved(_));
+ EXPECT_CALL(*mock_device_observers_[profile()], OnDeviceRemoved(_));
device_manager_.RemoveDevice(device_info->guid);
base::RunLoop().RunUntilIdle();
@@ -250,7 +280,7 @@
UsbChooserContext* store = GetChooserContext(profile());
EXPECT_FALSE(store->HasDevicePermission(origin, *device_info));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -266,11 +296,11 @@
store->GetAllGrantedObjects();
EXPECT_EQ(1u, all_origin_objects.size());
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
- EXPECT_CALL(mock_device_observer_, OnDeviceRemoved(_));
+ EXPECT_CALL(*mock_device_observers_[profile()], OnDeviceRemoved(_));
device_manager_.RemoveDevice(device_info->guid);
base::RunLoop().RunUntilIdle();
@@ -298,10 +328,11 @@
UsbDeviceInfoPtr device_info_2 =
device_manager_.CreateAndAddDevice(0, 0, "Google", "Gizmo", "");
UsbChooserContext* store = GetChooserContext(profile());
- UsbChooserContext* incognito_store = GetChooserContext(
- profile()->GetPrimaryOTRProfile(/*create_if_needed=*/true));
+ auto* otr_profile =
+ profile()->GetPrimaryOTRProfile(/*create_if_needed=*/true);
+ UsbChooserContext* incognito_store = GetChooserContext(otr_profile);
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -310,7 +341,7 @@
EXPECT_TRUE(store->HasDevicePermission(origin, *device_info_1));
EXPECT_FALSE(incognito_store->HasDevicePermission(origin, *device_info_1));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[otr_profile],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -356,7 +387,7 @@
kFooUrl, kFooUrl, ContentSettingsType::USB_GUARD, CONTENT_SETTING_BLOCK);
auto* store = GetChooserContext(profile());
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA))
@@ -975,7 +1006,7 @@
auto* store = GetChooserContext(profile());
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA))
@@ -1046,7 +1077,7 @@
UsbDeviceInfoPtr persistent_device_info = device_manager_.CreateAndAddDevice(
6353, 5678, "Specific", "Product", "123ABC");
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -1103,7 +1134,7 @@
profile()->GetPrefs()->Set(prefs::kManagedWebUsbAllowDevicesForUrls,
*base::JSONReader::ReadDeprecated(kPolicySetting));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
@@ -1156,7 +1187,7 @@
profile()->GetPrefs()->Set(prefs::kManagedWebUsbAllowDevicesForUrls,
*base::JSONReader::ReadDeprecated(kPolicySetting));
- EXPECT_CALL(mock_permission_observer_,
+ EXPECT_CALL(*mock_permission_observers_[profile()],
OnObjectPermissionChanged(
absl::make_optional(ContentSettingsType::USB_GUARD),
ContentSettingsType::USB_CHOOSER_DATA));
diff --git a/chrome/browser/usb/usb_chooser_controller.cc b/chrome/browser/usb/usb_chooser_controller.cc
index 00578e1..a735644 100644
--- a/chrome/browser/usb/usb_chooser_controller.cc
+++ b/chrome/browser/usb/usb_chooser_controller.cc
@@ -234,7 +234,7 @@
}
}
-void UsbChooserController::OnDeviceManagerConnectionError() {
+void UsbChooserController::OnBrowserContextShutdown() {
observation_.Reset();
}
diff --git a/chrome/browser/usb/usb_chooser_controller.h b/chrome/browser/usb/usb_chooser_controller.h
index 515933e5..e6d9631 100644
--- a/chrome/browser/usb/usb_chooser_controller.h
+++ b/chrome/browser/usb/usb_chooser_controller.h
@@ -55,7 +55,7 @@
void OnDeviceAdded(const device::mojom::UsbDeviceInfo& device_info) override;
void OnDeviceRemoved(
const device::mojom::UsbDeviceInfo& device_info) override;
- void OnDeviceManagerConnectionError() override;
+ void OnBrowserContextShutdown() override;
private:
void GotUsbDeviceList(std::vector<device::mojom::UsbDeviceInfoPtr> devices);
