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 <jkleinsc@electronjs.org>

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

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<blink::mojom::SerialPortFilterPtr> filters,
       content::SerialChooser::Callback callback);
 
+  base::ScopedObservation<SerialChooserContext,
+                          SerialChooserContext::PortObserver,
+                          &SerialChooserContext::AddPortObserver,
+                          &SerialChooserContext::RemovePortObserver>
+      port_observation_{this};
+  base::ObserverList<content::SerialDelegate::Observer> observer_list_;
+
   std::unordered_map<content::RenderFrameHost*,
                      std::unique_ptr<SerialChooserController>>
       controller_map_;

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

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,

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) {

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<SerialChooserContext> chooser_context_;
 
+  base::ScopedObservation<SerialChooserContext,
+                          SerialChooserContext::PortObserver,
+                          &SerialChooserContext::AddPortObserver,
+                          &SerialChooserContext::RemovePortObserver>
+      observation_{this};
+
   std::vector<device::mojom::SerialPortInfoPtr> ports_;
 
   base::WeakPtr<ElectronSerialDelegate> serial_delegate_;