8000 Devtools: send error replies instead of ignoring messages (#37686) · servo/servo@71d97bd · GitHub
[go: up one dir, main page]
Skip to content

Commit 71d97bd

Browse files
atbrakhidelansimonwuelkerthe6p4c
authored
Devtools: send error replies instead of ignoring messages (#37686)
Client messages, which are always requests, are dispatched to Actor instances one at a time via Actor::handle_message. Each request must be paired with exactly one reply from the same actor the request was sent to, where a reply is a message with no type (if a message from the server has a type, it’s a notification, not a reply). Failing to reply to a request will almost always permanently break that actor, because either the client gets stuck waiting for a reply, or the client receives the reply for a subsequent request as if it was the reply for the current request. If an actor fails to reply to a request, we want the dispatcher (ActorRegistry::handle_message) to send an error of type `unrecognizedPacketType`, to keep the conversation for that actor in sync. Since replies come in all shapes and sizes, we want to allow Actor types to send replies without having to return them to the dispatcher. This patch adds a wrapper type around a client stream that guarantees request/reply invariants. It allows the dispatcher to check if a valid reply was sent, and guarantees that if the actor tries to send a reply, it’s actually a valid reply (see ClientRequest::is_valid_reply). It does not currently guarantee anything about messages sent via the TcpStream released via ClientRequest::try_clone_stream or the return value of ClientRequest::reply. We also send `unrecognizedPacketType`, `missingParameter`, `badParameterType`, and `noSuchActor` messages per the [protocol](https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#error-packets) [docs](https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#packets). Testing: automated tests all pass, and manual testing looks ok Fixes: #37683 and at least six bugs, plus one with a different root cause, plus three with zero impact --------- Signed-off-by: atbrakhi <atbrakhi@igalia.com> Signed-off-by: Delan Azabani <dazabani@igalia.com> Co-authored-by: delan azabani <dazabani@igalia.com> Co-authored-by: Simon Wülker <simon.wuelker@arcor.de> Co-authored-by: the6p4c <me@doggirl.gay>
1 parent fcb2a4c commit 71d97bd

36 files changed

+661
-637
lines changed

‎components/devtools/actor.rs‎

Lines changed: 44 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,37 @@ use std::sync::{Arc, Mutex};
1212
use base::cross_process_instant::CrossProcessInstant;
1313
use base::id::PipelineId;
1414
use log::debug;
15-
use serde_json::{Map, Value};
15+
use serde_json::{Map, Value, json};
1616

1717
use crate::StreamId;
18+
use crate::protocol::{ClientRequest, JsonPacketStream};
19+
20+
/// Error replies.
21+
///
22+
/// <https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#error-packets>
23+
#[derive(Debug)]
24+
pub enum ActorError {
25+
MissingParameter,
26+
BadParameterType,
27+
UnrecognizedPacketType,
28+
/// Custom errors, not defined in the protocol docs.
29+
/// This includes send errors, and errors that prevent Servo from sending a reply.
30+
Internal,
31+
}
1832

19-
#[derive(PartialEq)]
20-
pub enum ActorMessageStatus {
21-
Processed,
22-
Ignored,
33+
impl ActorError {
34+
pub fn name(&self) -> &'static str {
35+
match self {
36+
ActorError::MissingParameter => "missingParameter",
37+
ActorError::BadParameterType => "badParameterType",
38+
ActorError::UnrecognizedPacketType => "unrecognizedPacketType",
39+
// The devtools frontend always checks for specific protocol errors by catching a JS exception `e` whose
40+
// message contains the error name, and checking `e.message.includes("someErrorName")`. As a result, the
41+
// only error name we can safely use for custom errors is the empty string, because any other error name we
42+
// use may be a substring of some upstream error name.
43+
ActorError::Internal => "",
44+
}
45+
}
2346
}
2447

2548
/// A common trait for all devtools actors that encompasses an immutable name
@@ -28,12 +51,12 @@ pub enum ActorMessageStatus {
2851
pub(crate) trait Actor: Any + ActorAsAny {
2952
fn handle_message(
3053
&self,
54+
request: ClientRequest,
3155
registry: &ActorRegistry,
3256
msg_type: &str,
3357
msg: &Map<String, Value>,
34-
stream: &mut TcpStream,
3558
stream_id: StreamId,
36-
) -> Result<ActorMessageStatus, ()>;
59+
) -> Result<(), ActorError>;
3760
fn name(&self) -> String;
3861
fn cleanup(&self, _id: StreamId) {}
3962
}
@@ -169,8 +192,8 @@ impl ActorRegistry {
169192
actor.actor_as_any_mut().downcast_mut::<T>().unwrap()
170193
}
171194

172-
/// Attempt to process a message as directed by its `to` property. If the actor is not
173-
/// found or does not indicate that it knew how to process the message, ignore the failure.
195+
/// Attempt to process a message as directed by its `to` property. If the actor is not found, does not support the
196+
/// message, or failed to handle the message, send an error reply instead.
174197
pub(crate) fn handle_message(
175198
&mut self,
176199
msg: &Map<String, Value>,
@@ -186,17 +209,20 @@ impl ActorRegistry {
186209
};
187210

188211
match self.actors.get(to) {
189-
None => log::warn!("message received for unknown actor \"{}\"", to),
212+
None => {
213+
// <https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#packets>
214+
let msg = json!({ "from": to, "error": "noSuchActor" });
215+
let _ = stream.write_json_packet(&msg);
216+
},
190217
Some(actor) => {
191218
let msg_type = msg.get("type").unwrap().as_str().unwrap();
192-
if actor.handle_message(self, msg_type, msg, stream, stream_id)? !=
193-
ActorMessageStatus::Processed
194-
{
195-
log::warn!(
196-
"unexpected message type \"{}\" found for actor \"{}\"",
197-
msg_type,
198-
to
199-
);
219+
if let Err(error) = ClientRequest::handle(stream, to, |req| {
220+
actor.handle_message(req, self, msg_type, msg, stream_id)
221+
}) {
222+
// <https://firefox-source-docs.mozilla.org/devtools/backend/protocol.html#error-packets>
223+
let _ = stream.write_json_packet(&json!({
224+
"from": actor.name(), "error": error.name()
225+
}));
200226
}
201227
},
202228
}

‎components/devtools/actors/breakpoint.rs‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55
use serde::Serialize;
66

77
use crate::EmptyReplyMsg;
8-
use crate::actor::{Actor, ActorMessageStatus};
9-
use crate::protocol::JsonPacketStream;
8+
use crate::actor::{Actor, ActorError};
9+
use crate::protocol::ClientRequest;
1010

1111
#[derive(Serialize)]
1212
pub struct BreakpointListActorMsg {
@@ -24,27 +24,24 @@ impl Actor for BreakpointListActor {
2424

2525
fn handle_message(
2626
&self,
27+
request: ClientRequest,
2728
_registry: &crate::actor::ActorRegistry,
2829
msg_type: &str,
2930
_msg: &serde_json::Map<String, serde_json::Value>,
30-
stream: &mut std::net::TcpStream,
3131
_stream_id: crate::StreamId,
32-
) -> Result<crate::actor::ActorMessageStatus, ()> {
33-
Ok(match msg_type {
32+
) -> Result<(), ActorError> {
33+
match msg_type {
3434
"setBreakpoint" => {
3535
let msg = EmptyReplyMsg { from: self.name() };
36-
let _ = stream.write_json_packet(&msg);
37-
38-
ActorMessageStatus::Processed
36+
request.reply_final(&msg)?
3937
},
4038
"setActiveEventBreakpoints" => {
4139
let msg = EmptyReplyMsg { from: self.name() };
42-
let _ = stream.write_json_packet(&msg);
43-
44-
ActorMessageStatus::Processed
40+
request.reply_final(&msg)?
4541
},
46-
_ => ActorMessageStatus::Ignored,
47-
})
42+
_ => return Err(ActorError::UnrecognizedPacketType),
43+
};
44+
Ok(())
4845
}
4946
}
5047

‎components/devtools/actors/browsing_context.rs‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use ipc_channel::ipc::{self, IpcSender};
2020
use serde::Serialize;
2121
use serde_json::{Map, Value};
2222

23-
use crate::actor::{Actor, ActorMessageStatus, ActorRegistry};
23+
use crate::actor::{Actor, ActorError, ActorRegistry};
2424
use crate::actors::inspector::InspectorActor;
2525
use crate::actors::inspector::accessibility::AccessibilityActor;
2626
use crate::actors::inspector::css_properties::CssPropertiesActor;
@@ -30,7 +30,7 @@ use crate::actors::tab::TabDescriptorActor;
3030
use crate::actors::thread::ThreadActor;
3131
use crate::actors::watcher::{SessionContext, SessionContextType, WatcherActor};
3232
use crate::id::{DevtoolsBrowserId, DevtoolsBrowsingContextId, DevtoolsOuterWindowId, IdMap};
33-
use crate::protocol::JsonPacketStream;
33+
use crate::protocol::{ClientRequest, JsonPacketStream};
3434
use crate::resource::ResourceAvailable;
3535
use crate::{EmptyReplyMsg, StreamId};
3636

@@ -166,29 +166,28 @@ impl Actor for BrowsingContextActor {
166166

167167
fn handle_message(
168168
&self,
169+
request: ClientRequest,
169170
_registry: &ActorRegistry,
170171
msg_type: &str,
171172
_msg: &Map<String, Value>,
172-
stream: &mut TcpStream,
173173
_id: StreamId,
174-
) -> Result<ActorMessageStatus, ()> {
175-
Ok(match msg_type {
174+
) -> Result<(), ActorError> {
175+
match msg_type {
176176
"listFrames" => {
177177
// TODO: Find out what needs to be listed here
178178
let msg = EmptyReplyMsg { from: self.name() };
179-
let _ = stream.write_json_packet(&msg);
180-
ActorMessageStatus::Processed
179+
request.reply_final(&msg)?
181180
},
182181
"listWorkers" => {
183-
let _ = stream.write_json_packet(&ListWorkersReply {
182+
request.reply_final(&ListWorkersReply {
184183
from: self.name(),
185184
// TODO: Find out what needs to be listed here
186185
workers: vec![],
187-
});
188-
ActorMessageStatus::Processed
186+
})?
189187
},
190-
_ => ActorMessageStatus::Ignored,
191-
})
188+
_ => return Err(ActorError::UnrecognizedPacketType),
189+
};
190+
Ok(())
192191
}
193192

194193
fn cleanup(&self, id: StreamId) {
@@ -353,8 +352,8 @@ impl BrowsingContextActor {
353352
*self.title.borrow_mut() = title;
354353
}
355354

356-
pub(crate) fn frame_update(&self, stream: &mut TcpStream) {
357-
let _ = stream.write_json_packet(&FrameUpdateReply {
355+
pub(crate) fn frame_update(&self, request: &mut ClientRequest) {
356+
let _ = request.write_json_packet(&FrameUpdateReply {
358357
from: self.name(),
359358
type_: "frameUpdate".into(),
360359
frames: vec![FrameUpdateMsg {

‎components/devtools/actors/console.rs‎

Lines changed: 19 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,11 @@ use serde::Serialize;
2525
use serde_json::{self, Map, Number, Value};
2626
use uuid::Uuid;
2727

28-
use crate::actor::{Actor, ActorMessageStatus, ActorRegistry};
28+
use crate::actor::{Actor, ActorError, ActorRegistry};
2929
use crate::actors::browsing_context::BrowsingContextActor;
3030
use crate::actors::object::ObjectActor;
3131
use crate::actors::worker::WorkerActor;
32-
use crate::protocol::JsonPacketStream;
32+
use crate::protocol::{ClientRequest, JsonPacketStream};
3333
use crate::resource::{ResourceArrayType, ResourceAvailable};
3434
use crate::{StreamId, UniqueId};
3535

@@ -304,18 +304,19 @@ impl Actor for ConsoleActor {
304304

305305
fn handle_message(
306306
&self,
307+
request: ClientRequest,
307308
registry: &ActorRegistry,
308309
msg_type: &str,
309310
msg: &Map<String, Value>,
310-
stream: &mut TcpStream,
311311
_id: StreamId,
312-
) -> Result<ActorMessageStatus, ()> {
313-
Ok(match msg_type {
312+
) -> Result<(), ActorError> {
313+
match msg_type {
314314
"clearMessagesCache" => {
315315
self.cached_events
316316
.borrow_mut()
317317
.remove(&self.current_unique_id(registry));
318-
ActorMessageStatus::Processed
318+
// FIXME: need to send a reply here!
319+
return Err(ActorError::UnrecognizedPacketType);
319320
},
320321

321322
"getCachedMessages" => {
@@ -368,8 +369,7 @@ impl Actor for ConsoleActor {
368369
from: self.name(),
369370
messages,
370371
};
371-
let _ = stream.write_json_packet(&msg);
372-
ActorMessageStatus::Processed
372+
request.reply_final(&msg)?
373373
},
374374

375375
"startListeners" => {
@@ -384,8 +384,7 @@ impl Actor for ConsoleActor {
384384
.collect(),
385385
traits: StartedListenersTraits,
386386
};
387-
let _ = stream.write_json_packet(&msg);
388-
ActorMessageStatus::Processed
387+
request.reply_final(&msg)?
389388
},
390389

391390
"stopListeners" => {
@@ -401,8 +400,7 @@ impl Actor for ConsoleActor {
401400
.map(|listener| listener.as_str().unwrap().to_owned())
402401
.collect(),
403402
};
404-
let _ = stream.write_json_packet(&msg);
405-
ActorMessageStatus::Processed
403+
request.reply_final(&msg)?
406404
},
407405

408406
//TODO: implement autocompletion like onAutocomplete in
@@ -413,14 +411,12 @@ impl Actor for ConsoleActor {
413411
matches: vec![],
414412
match_prop: "".to_owned(),
415413
};
416-
let _ = stream.write_json_packet(&msg);
417-
ActorMessageStatus::Processed
414+
request.reply_final(&msg)?
418415
},
419416

420417
"evaluateJS" => {
421418
let msg = self.evaluate_js(registry, msg);
422-
let _ = stream.write_json_packet(&msg);
423-
ActorMessageStatus::Processed
419+
request.reply_final(&msg)?
424420
},
425421

426422
"evaluateJSAsync" => {
@@ -431,14 +427,12 @@ impl Actor for ConsoleActor {
431427
};
432428
// Emit an eager reply so that the client starts listening
433429
// for an async event with the resultID
434-
if stream.write_json_packet(&early_reply).is_err() {
435-
return Ok(ActorMessageStatus::Processed);
436-
}
430+
let stream = request.reply(&early_reply)?;
437431

438432
if msg.get("eager").and_then(|v| v.as_bool()).unwrap_or(false) {
439433
// We don't support the side-effect free evaluation that eager evalaution
440434
// really needs.
441-
return Ok(ActorMessageStatus::Processed);
435+
return Ok(());
442436
}
443437

444438
let reply = self.evaluate_js(registry, msg).unwrap();
@@ -454,20 +448,19 @@ impl Actor for ConsoleActor {
454448
helper_result: reply.helper_result,
455449
};
456450
// Send the data from evaluateJS along with a resultID
457-
let _ = stream.write_json_packet(&msg);
458-
ActorMessageStatus::Processed
451+
stream.write_json_packet(&msg)?
459452
},
460453

461454
"setPreferences" => {
462455
let msg = SetPreferencesReply {
463456
from: self.name(),
464457
updated: vec![],
465458
};
466-
let _ = stream.write_json_packet(&msg);
467-
ActorMessageStatus::Processed
459+
request.reply_final(&msg)?
468460
},
469461

470-
_ => ActorMessageStatus::Ignored,
471-
})
462+
_ => return Err(ActorError::UnrecognizedPacketType),
463+
};
464+
Ok(())
472465
}
473466
}

‎components/devtools/actors/device.rs‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,12 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
44

5-
use std::net::TcpStream;
6-
75
use serde::Serialize;
86
use serde_json::{Map, Value};
97

108
use crate::StreamId;
11-
use crate::actor::{Actor, ActorMessageStatus, ActorRegistry};
12-
use crate::protocol::{ActorDescription, JsonPacketStream, Method};
9+
use crate::actor::{Actor, ActorError, ActorRegistry};
10+
use crate::protocol::{ActorDescription, ClientRequest, Method};
1311

1412
#[derive(Serialize)]
1513
struct GetDescriptionReply {
@@ -46,13 +44,13 @@ impl Actor for DeviceActor {
4644
}
4745
fn handle_message(
4846
&self,
47+
request: ClientRequest,
4948
_registry: &ActorRegistry,
5049
msg_type: &str,
5150
_msg: &Map<String, Value>,
52-
stream: &mut TcpStream,
5351
_id: StreamId,
54-
) -> Result<ActorMessageStatus, ()> {
55-
Ok(match msg_type {
52+
) -> Result<(), ActorError> {
53+
match msg_type {
5654
"getDescription" => {
5755
let msg = GetDescriptionReply {
5856
from: self.name(),
@@ -64,12 +62,12 @@ impl Actor for DeviceActor {
6462
brand_name: "Servo".to_string(),
6563
},
6664
};
67-
let _ = stream.write_json_packet(&msg);
68-
ActorMessageStatus::Processed
65+
request.reply_final(&msg)?
6966
},
7067

71-
_ => ActorMessageStatus::Ignored,
72-
})
68+
_ => return Err(ActorError::UnrecognizedPacketType),
69+
};
70+
Ok(())
7371
}
7472
}
7573

0 commit comments

Comments
 (0)
0