Skip to content

Commit af3ea50

Browse files
jkleinscschetle
jkleinsc
authored and
schetle
committed
fix: properly fire serial-port-added and serial-port-removed events (electron#34958)
Based on 2309652: [webhid] Notify chooser context observers on shutdown | https://chromium-review.googlesource.com/c/chromium/src/+/2309652
1 parent f978eab commit af3ea50

6 files changed

Lines changed: 71 additions & 21 deletions

‎shell/browser/serial/electron_serial_delegate.cc‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,15 +71,15 @@ device::mojom::SerialPortManager* ElectronSerialDelegate::GetPortManager(
7171

7272
void ElectronSerialDelegate::AddObserver(content::RenderFrameHost* frame,
7373
Observer* observer) {
74-
return GetChooserContext(frame)->AddPortObserver(observer);
74+
observer_list_.AddObserver(observer);
75+
auto* chooser_context = GetChooserContext(frame);
76+
if (!port_observation_.IsObserving())
77+
port_observation_.Observe(chooser_context);
7578
}
7679

7780
void ElectronSerialDelegate::RemoveObserver(content::RenderFrameHost* frame,
7881
Observer* observer) {
79-
SerialChooserContext* serial_chooser_context = GetChooserContext(frame);
80-
if (serial_chooser_context) {
81-
return serial_chooser_context->RemovePortObserver(observer);
82-
}
82+
observer_list_.RemoveObserver(observer);
8383
}
8484

8585
void ElectronSerialDelegate::RevokePortPermissionWebInitiated(
@@ -120,4 +120,27 @@ void ElectronSerialDelegate::DeleteControllerForFrame(
120120
controller_map_.erase(render_frame_host);
121121
}
122122

123+
// SerialChooserContext::PortObserver:
124+
void ElectronSerialDelegate::OnPortAdded(
125+
const device::mojom::SerialPortInfo& port) {
126+
for (auto& observer : observer_list_)
127+
observer.OnPortAdded(port);
128+
}
129+
130+
void ElectronSerialDelegate::OnPortRemoved(
131+
const device::mojom::SerialPortInfo& port) {
132+
for (auto& observer : observer_list_)
133+
observer.OnPortRemoved(port);
134+
}
135+
136+
void ElectronSerialDelegate::OnPortManagerConnectionError() {
137+
port_observation_.Reset();
138+
for (auto& observer : observer_list_)
139+
observer.OnPortManagerConnectionError();
140+
}
141+
142+
void ElectronSerialDelegate::OnSerialChooserContextShutdown() {
143+
port_observation_.Reset();
144+
}
145+
123146
} // namespace electron

‎shell/browser/serial/electron_serial_delegate.h‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@
1111

1212
#include "base/memory/weak_ptr.h"
1313
#include "content/public/browser/serial_delegate.h"
14+
#include "shell/browser/serial/serial_chooser_context.h"
1415
#include "shell/browser/serial/serial_chooser_controller.h"
1516

1617
namespace electron {
1718

1819
class SerialChooserController;
1920

20-
class ElectronSerialDelegate : public content::SerialDelegate {
21+
class ElectronSerialDelegate : public content::SerialDelegate,
22+
public SerialChooserContext::PortObserver {
2123
public:
2224
ElectronSerialDelegate();
2325
~ElectronSerialDelegate() override;
@@ -48,6 +50,13 @@ class ElectronSerialDelegate : public content::SerialDelegate {
4850

4951
void DeleteControllerForFrame(content::RenderFrameHost* render_frame_host);
5052

53+
// SerialChooserContext::PortObserver:
54+
void OnPortAdded(const device::mojom::SerialPortInfo& port) override;
55+
void OnPortRemoved(const device::mojom::SerialPortInfo& port) override;
56+
void OnPortManagerConnectionError() override;
57+
void OnPermissionRevoked(const url::Origin& origin) override {}
58+
void OnSerialChooserContextShutdown() override;
59+
5160
private:
5261
SerialChooserController* ControllerForFrame(
5362
content::RenderFrameHost* render_frame_host);
@@ -56,6 +65,13 @@ class ElectronSerialDelegate : public content::SerialDelegate {
5665
std::vector<blink::mojom::SerialPortFilterPtr> filters,
5766
content::SerialChooser::Callback callback);
5867

68+
base::ScopedObservation<SerialChooserContext,
69+
SerialChooserContext::PortObserver,
70+
&SerialChooserContext::AddPortObserver,
71+
&SerialChooserContext::RemovePortObserver>
72+
port_observation_{this};
73+
base::ObserverList<content::SerialDelegate::Observer> observer_list_;
74+
5975
std::unordered_map<content::RenderFrameHost*,
6076
std::unique_ptr<SerialChooserController>>
6177
controller_map_;

‎shell/browser/serial/serial_chooser_context.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,13 @@ base::Value PortInfoToValue(const device::mojom::SerialPortInfo& port) {
9090
SerialChooserContext::SerialChooserContext(ElectronBrowserContext* context)
9191
: browser_context_(context) {}
9292

93-
SerialChooserContext::~SerialChooserContext() = default;
94-
95-
void SerialChooserContext::OnPermissionRevoked(const url::Origin& origin) {
96-
for (auto& observer : port_observer_list_)
97-
observer.OnPermissionRevoked(origin);
93+
SerialChooserContext::~SerialChooserContext() {
94+
// Notify observers that the chooser context is about to be destroyed.
95+
// Observers must remove themselves from the observer lists.
96+
for (auto& observer : port_observer_list_) {
97+
observer.OnSerialChooserContextShutdown();
98+
DCHECK(!port_observer_list_.HasObserver(&observer));
99+
}
98100
}
99101

100102
void SerialChooserContext::GrantPortPermission(
@@ -127,8 +129,6 @@ void SerialChooserContext::RevokePortPermissionWebInitiated(
127129
auto it = port_info_.find(token);
128130
if (it == port_info_.end())
129131
return;
130-
131-
return OnPermissionRevoked(origin);
132132
}
133133

134134
// static

‎shell/browser/serial/serial_chooser_context.h‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,12 @@ extern const char kUsbDriverKey[];
4646
class SerialChooserContext : public KeyedService,
4747
public device::mojom::SerialPortManagerClient {
4848
public:
49-
using PortObserver = content::SerialDelegate::Observer;
49+
class PortObserver : public content::SerialDelegate::Observer {
50+
public:
51+
// Called when the SerialChooserContext is shutting down. Observers must
52+
// remove themselves before returning.
53+
virtual void OnSerialChooserContextShutdown() = 0;
54+
};
5055

5156
explicit SerialChooserContext(ElectronBrowserContext* context);
5257
~SerialChooserContext() override;
@@ -55,9 +60,6 @@ class SerialChooserContext : public KeyedService,
5560
SerialChooserContext(const SerialChooserContext&) = delete;
5661
SerialChooserContext& operator=(const SerialChooserContext&) = delete;
5762

58-
// ObjectPermissionContextBase::PermissionObserver:
59-
void OnPermissionRevoked(const url::Origin& origin);
60-
6163
// Serial-specific interface for granting and checking permissions.
6264
void GrantPortPermission(const url::Origin& origin,
6365
const device::mojom::SerialPortInfo& port,

‎shell/browser/serial/serial_chooser_controller.cc‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,13 +77,11 @@ SerialChooserController::SerialChooserController(
7777
DCHECK(chooser_context_);
7878
chooser_context_->GetPortManager()->GetDevices(base::BindOnce(
7979
&SerialChooserController::OnGetDevices, weak_factory_.GetWeakPtr()));
80+
observation_.Observe(chooser_context_.get());
8081
}
8182

8283
SerialChooserController::~SerialChooserController() {
8384
RunCallback(/*port=*/nullptr);
84-
if (chooser_context_) {
85-
chooser_context_->RemovePortObserver(this);
86-
}
8785
}
8886

8987
api::Session* SerialChooserController::GetSession() {
@@ -117,7 +115,11 @@ void SerialChooserController::OnPortRemoved(
117115
}
118116

119117
void SerialChooserController::OnPortManagerConnectionError() {
120-
// TODO(nornagon/jkleinsc): report event
118+
observation_.Reset();
119+
}
120+
121+
void SerialChooserController::OnSerialChooserContextShutdown() {
122+
observation_.Reset();
121123
}
122124

123125
void SerialChooserController::OnDeviceChosen(const std::string& port_id) {

‎shell/browser/serial/serial_chooser_controller.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ class SerialChooserController final : public SerialChooserContext::PortObserver,
4848
void OnPortRemoved(const device::mojom::SerialPortInfo& port) override;
4949
void OnPortManagerConnectionError() override;
5050
void OnPermissionRevoked(const url::Origin& origin) override {}
51+
void OnSerialChooserContextShutdown() override;
5152

5253
private:
5354
api::Session* GetSession();
@@ -62,6 +63,12 @@ class SerialChooserController final : public SerialChooserContext::PortObserver,
6263

6364
base::WeakPtr<SerialChooserContext> chooser_context_;
6465

66+
base::ScopedObservation<SerialChooserContext,
67+
SerialChooserContext::PortObserver,
68+
&SerialChooserContext::AddPortObserver,
69+
&SerialChooserContext::RemovePortObserver>
70+
observation_{this};
71+
6572
std::vector<device::mojom::SerialPortInfoPtr> ports_;
6673

6774
base::WeakPtr<ElectronSerialDelegate> serial_delegate_;

0 commit comments

Comments
 (0)