From c4a1a667e3f248aa4678ebd9c7a5b2aaef167f92 Mon Sep 17 00:00:00 2001 From: John Kleinschmidt Date: Thu, 21 Jul 2022 11:26:43 -0400 Subject: [PATCH] chore: Notify chooser context observers on shutdown Based on 2309652: [webhid] Notify chooser context observers on shutdown | https://chromium-review.googlesource.com/c/chromium/src/+/2309652 --- .../serial/electron_serial_delegate.cc | 33 ++++++++++++++++--- .../browser/serial/electron_serial_delegate.h | 17 +++++++++- .../browser/serial/serial_chooser_context.cc | 15 ++++----- shell/browser/serial/serial_chooser_context.h | 15 ++++++--- .../serial/serial_chooser_controller.cc | 4 +++ .../serial/serial_chooser_controller.h | 2 +- 6 files changed, 66 insertions(+), 20 deletions(-) diff --git a/shell/browser/serial/electron_serial_delegate.cc b/shell/browser/serial/electron_serial_delegate.cc index 1c4d534ba38e0..98a5a3f803a7a 100644 --- a/shell/browser/serial/electron_serial_delegate.cc +++ b/shell/browser/serial/electron_serial_delegate.cc @@ -71,15 +71,15 @@ device::mojom::SerialPortManager* ElectronSerialDelegate::GetPortManager( void ElectronSerialDelegate::AddObserver(content::RenderFrameHost* frame, Observer* observer) { - return GetChooserContext(frame)->AddPortObserver(observer); + observer_list_.AddObserver(observer); + auto* chooser_context = GetChooserContext(frame); + if (!port_observation_.IsObserving()) + port_observation_.Observe(chooser_context); } void ElectronSerialDelegate::RemoveObserver(content::RenderFrameHost* frame, Observer* observer) { - SerialChooserContext* serial_chooser_context = GetChooserContext(frame); - if (serial_chooser_context) { - return serial_chooser_context->RemovePortObserver(observer); - } + observer_list_.RemoveObserver(observer); } void ElectronSerialDelegate::RevokePortPermissionWebInitiated( @@ -120,4 +120,27 @@ void ElectronSerialDelegate::DeleteControllerForFrame( controller_map_.erase(render_frame_host); } +// SerialChooserContext::PortObserver: +void ElectronSerialDelegate::OnPortAdded( + const device::mojom::SerialPortInfo& port) { + for (auto& observer : observer_list_) + observer.OnPortAdded(port); +} + +void ElectronSerialDelegate::OnPortRemoved( + const device::mojom::SerialPortInfo& port) { + for (auto& observer : observer_list_) + observer.OnPortRemoved(port); +} + +void ElectronSerialDelegate::OnPortManagerConnectionError() { + port_observation_.Reset(); + for (auto& observer : observer_list_) + observer.OnPortManagerConnectionError(); +} + +void ElectronSerialDelegate::OnSerialChooserContextShutdown() { + port_observation_.Reset(); +} + } // namespace electron diff --git a/shell/browser/serial/electron_serial_delegate.h b/shell/browser/serial/electron_serial_delegate.h index 1153bcd8ad297..e7eace9a44892 100644 --- a/shell/browser/serial/electron_serial_delegate.h +++ b/shell/browser/serial/electron_serial_delegate.h @@ -11,13 +11,15 @@ #include "base/memory/weak_ptr.h" #include "content/public/browser/serial_delegate.h" +#include "shell/browser/serial/serial_chooser_context.h" #include "shell/browser/serial/serial_chooser_controller.h" namespace electron { class SerialChooserController; -class ElectronSerialDelegate : public content::SerialDelegate { +class ElectronSerialDelegate : public content::SerialDelegate, + public SerialChooserContext::PortObserver { public: ElectronSerialDelegate(); ~ElectronSerialDelegate() override; @@ -48,6 +50,12 @@ class ElectronSerialDelegate : public content::SerialDelegate { void DeleteControllerForFrame(content::RenderFrameHost* render_frame_host); + // SerialChooserContext::PortObserver: + void OnPortAdded(const device::mojom::SerialPortInfo& port) override; + void OnPortRemoved(const device::mojom::SerialPortInfo& port) override; + void OnPortManagerConnectionError() override; + void OnSerialChooserContextShutdown() override; + private: SerialChooserController* ControllerForFrame( content::RenderFrameHost* render_frame_host); @@ -56,6 +64,13 @@ class ElectronSerialDelegate : public content::SerialDelegate { std::vector filters, content::SerialChooser::Callback callback); + base::ScopedObservation + port_observation_{this}; + base::ObserverList observer_list_; + std::unordered_map> controller_map_; diff --git a/shell/browser/serial/serial_chooser_context.cc b/shell/browser/serial/serial_chooser_context.cc index abd819de76662..57798ccf5692f 100644 --- a/shell/browser/serial/serial_chooser_context.cc +++ b/shell/browser/serial/serial_chooser_context.cc @@ -91,13 +91,12 @@ SerialChooserContext::SerialChooserContext(ElectronBrowserContext* context) : browser_context_(context) {} SerialChooserContext::~SerialChooserContext() { - for (auto& observer : port_observer_list_) - port_observer_list_.RemoveObserver(&observer); -} - -void SerialChooserContext::OnPermissionRevoked(const url::Origin& origin) { - for (auto& observer : port_observer_list_) - observer.OnPermissionRevoked(origin); + // Notify observers that the chooser context is about to be destroyed. + // Observers must remove themselves from the observer lists. + for (auto& observer : port_observer_list_) { + observer.OnSerialChooserContextShutdown(); + DCHECK(!port_observer_list_.HasObserver(&observer)); + } } void SerialChooserContext::GrantPortPermission( @@ -130,8 +129,6 @@ void SerialChooserContext::RevokePortPermissionWebInitiated( auto it = port_info_.find(token); if (it == port_info_.end()) return; - - return OnPermissionRevoked(origin); } // static diff --git a/shell/browser/serial/serial_chooser_context.h b/shell/browser/serial/serial_chooser_context.h index a0426c8b8f0c4..552ffd322aeaa 100644 --- a/shell/browser/serial/serial_chooser_context.h +++ b/shell/browser/serial/serial_chooser_context.h @@ -46,7 +46,17 @@ extern const char kUsbDriverKey[]; class SerialChooserContext : public KeyedService, public device::mojom::SerialPortManagerClient { public: - using PortObserver = content::SerialDelegate::Observer; + class PortObserver : public base::CheckedObserver { + public: + // Events forwarded from SerialChooserContext::PortObserver: + virtual void OnPortAdded(const device::mojom::SerialPortInfo& port) = 0; + virtual void OnPortRemoved(const device::mojom::SerialPortInfo& port) = 0; + virtual void OnPortManagerConnectionError() = 0; + + // Called when the SerialChooserContext is shutting down. Observers must + // remove themselves before returning. + virtual void OnSerialChooserContextShutdown() = 0; + }; explicit SerialChooserContext(ElectronBrowserContext* context); ~SerialChooserContext() override; @@ -55,9 +65,6 @@ class SerialChooserContext : public KeyedService, SerialChooserContext(const SerialChooserContext&) = delete; SerialChooserContext& operator=(const SerialChooserContext&) = delete; - // ObjectPermissionContextBase::PermissionObserver: - void OnPermissionRevoked(const url::Origin& origin); - // Serial-specific interface for granting and checking permissions. void GrantPortPermission(const url::Origin& origin, const device::mojom::SerialPortInfo& port, diff --git a/shell/browser/serial/serial_chooser_controller.cc b/shell/browser/serial/serial_chooser_controller.cc index 211a7f95616cb..017d951d4c01b 100644 --- a/shell/browser/serial/serial_chooser_controller.cc +++ b/shell/browser/serial/serial_chooser_controller.cc @@ -118,6 +118,10 @@ void SerialChooserController::OnPortManagerConnectionError() { observation_.Reset(); } +void SerialChooserController::OnSerialChooserContextShutdown() { + observation_.Reset(); +} + void SerialChooserController::OnDeviceChosen(const std::string& port_id) { if (port_id.empty()) { RunCallback(/*port=*/nullptr); diff --git a/shell/browser/serial/serial_chooser_controller.h b/shell/browser/serial/serial_chooser_controller.h index 24a43995ca2a4..c64078bc8dbb0 100644 --- a/shell/browser/serial/serial_chooser_controller.h +++ b/shell/browser/serial/serial_chooser_controller.h @@ -47,7 +47,7 @@ class SerialChooserController final : public SerialChooserContext::PortObserver, void OnPortAdded(const device::mojom::SerialPortInfo& port) override; void OnPortRemoved(const device::mojom::SerialPortInfo& port) override; void OnPortManagerConnectionError() override; - void OnPermissionRevoked(const url::Origin& origin) override {} + void OnSerialChooserContextShutdown() override; private: api::Session* GetSession();