From 3a079feb87ff6a1d3cea70cfdb20e77e9a5e3416 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Mon, 25 Jul 2022 16:36:59 -0400 Subject: [PATCH] fix: properly fire serial-port-added and serial-port-removed events (#35047) Based on 2309652: [webhid] Notify chooser context observers on shutdown | https://chromium-review.googlesource.com/c/chromium/src/+/2309652 Co-authored-by: John Kleinschmidt --- .../serial/electron_serial_delegate.cc | 33 ++++++++++++++++--- .../browser/serial/electron_serial_delegate.h | 18 +++++++++- .../browser/serial/serial_chooser_context.cc | 14 ++++---- shell/browser/serial/serial_chooser_context.h | 10 +++--- .../serial/serial_chooser_controller.cc | 10 +++--- .../serial/serial_chooser_controller.h | 7 ++++ 6 files changed, 71 insertions(+), 21 deletions(-) diff --git a/shell/browser/serial/electron_serial_delegate.cc b/shell/browser/serial/electron_serial_delegate.cc index 42b01a77f3d1b..bc3311f6e7a2e 100644 --- a/shell/browser/serial/electron_serial_delegate.cc +++ b/shell/browser/serial/electron_serial_delegate.cc @@ -70,15 +70,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( @@ -119,4 +119,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..5876d7e511041 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,13 @@ 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 OnPermissionRevoked(const url::Origin& origin) override {} + void OnSerialChooserContextShutdown() override; + private: SerialChooserController* ControllerForFrame( content::RenderFrameHost* render_frame_host); @@ -56,6 +65,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 356547f81a55f..57798ccf5692f 100644 --- a/shell/browser/serial/serial_chooser_context.cc +++ b/shell/browser/serial/serial_chooser_context.cc @@ -90,11 +90,13 @@ base::Value PortInfoToValue(const device::mojom::SerialPortInfo& port) { SerialChooserContext::SerialChooserContext(ElectronBrowserContext* context) : browser_context_(context) {} -SerialChooserContext::~SerialChooserContext() = default; - -void SerialChooserContext::OnPermissionRevoked(const url::Origin& origin) { - for (auto& observer : port_observer_list_) - observer.OnPermissionRevoked(origin); +SerialChooserContext::~SerialChooserContext() { + // 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( @@ -127,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..2424ed1e64f19 100644 --- a/shell/browser/serial/serial_chooser_context.h +++ b/shell/browser/serial/serial_chooser_context.h @@ -46,7 +46,12 @@ extern const char kUsbDriverKey[]; class SerialChooserContext : public KeyedService, public device::mojom::SerialPortManagerClient { public: - using PortObserver = content::SerialDelegate::Observer; + class PortObserver : public content::SerialDelegate::Observer { + public: + // 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 +60,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 5adfc5e63c583..145b12b613a43 100644 --- a/shell/browser/serial/serial_chooser_controller.cc +++ b/shell/browser/serial/serial_chooser_controller.cc @@ -77,13 +77,11 @@ SerialChooserController::SerialChooserController( DCHECK(chooser_context_); chooser_context_->GetPortManager()->GetDevices(base::BindOnce( &SerialChooserController::OnGetDevices, weak_factory_.GetWeakPtr())); + observation_.Observe(chooser_context_.get()); } SerialChooserController::~SerialChooserController() { RunCallback(/*port=*/nullptr); - if (chooser_context_) { - chooser_context_->RemovePortObserver(this); - } } api::Session* SerialChooserController::GetSession() { @@ -117,7 +115,11 @@ void SerialChooserController::OnPortRemoved( } void SerialChooserController::OnPortManagerConnectionError() { - // TODO(nornagon/jkleinsc): report event + observation_.Reset(); +} + +void SerialChooserController::OnSerialChooserContextShutdown() { + observation_.Reset(); } void SerialChooserController::OnDeviceChosen(const std::string& port_id) { diff --git a/shell/browser/serial/serial_chooser_controller.h b/shell/browser/serial/serial_chooser_controller.h index 5635ea8bed743..761065763fbdb 100644 --- a/shell/browser/serial/serial_chooser_controller.h +++ b/shell/browser/serial/serial_chooser_controller.h @@ -48,6 +48,7 @@ class SerialChooserController final : public SerialChooserContext::PortObserver, 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(); @@ -62,6 +63,12 @@ class SerialChooserController final : public SerialChooserContext::PortObserver, base::WeakPtr chooser_context_; + base::ScopedObservation + observation_{this}; + std::vector ports_; base::WeakPtr serial_delegate_;