libservo: Notify delegates of send errors in request objects (#35668)

* libservo: Notify delegates of send errors in request objects

Signed-off-by: Delan Azabani <dazabani@igalia.com>

* Remove webview error sender for simplicity

Signed-off-by: Delan Azabani <dazabani@igalia.com>

* Remove error sender trait, now that there is only one impl

Signed-off-by: Delan Azabani <dazabani@igalia.com>

* Address review feedback

Signed-off-by: Delan Azabani <dazabani@igalia.com>

* Add unit tests

Signed-off-by: Delan Azabani <dazabani@igalia.com>

---------

Signed-off-by: Delan Azabani <dazabani@igalia.com>
This commit is contained in:
Delan Azabani 2025-03-19 14:41:14 +08:00 committed by GitHub
parent f19dd23641
commit a442a11330
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 357 additions and 27 deletions

1
Cargo.lock generated
View file

@ -4337,6 +4337,7 @@ dependencies = [
"gaol",
"gleam",
"gstreamer",
"http 1.3.1",
"ipc-channel",
"keyboard-types",
"layout_thread_2020",

View file

@ -124,6 +124,7 @@ gaol = "0.2.1"
webxr = { path = "../webxr", features = ["ipc", "glwindow", "headless", "openxr-api"] }
[dev-dependencies]
http = { workspace = true }
libservo = { path = ".", features = ["tracing"] }
rustls = { version = "0.23", default-features = false, features = ["aws-lc-rs"] }
tracing = { workspace = true }

View file

@ -19,6 +19,7 @@
mod clipboard_delegate;
mod proxies;
mod responders;
mod servo_delegate;
mod webview;
mod webview_delegate;
@ -118,6 +119,7 @@ pub use {
pub use {bluetooth, bluetooth_traits};
use crate::proxies::ConstellationProxy;
use crate::responders::ServoErrorChannel;
pub use crate::servo_delegate::{ServoDelegate, ServoError};
pub use crate::webview::WebView;
pub use crate::webview_delegate::{
@ -205,6 +207,7 @@ pub struct Servo {
/// When accessed, `Servo` will be reponsible for cleaning up the invalid `Weak`
/// references.
webviews: RefCell<HashMap<WebViewId, Weak<RefCell<WebViewInner>>>>,
servo_errors: ServoErrorChannel,
/// For single-process Servo instances, this field controls the initialization
/// and deinitialization of the JS Engine. Multiprocess Servo instances have their
/// own instance that exists in the content process instead.
@ -536,6 +539,7 @@ impl Servo {
embedder_receiver,
shutdown_state,
webviews: Default::default(),
servo_errors: ServoErrorChannel::default(),
_js_engine_setup: js_engine_setup,
}
}
@ -585,6 +589,7 @@ impl Servo {
self.compositor.borrow_mut().perform_updates();
self.send_new_frame_ready_messages();
self.handle_delegate_errors();
self.clean_up_destroyed_webview_handles();
if self.shutdown_state.get() == ShutdownState::FinishedShuttingDown {
@ -609,6 +614,12 @@ impl Servo {
}
}
fn handle_delegate_errors(&self) {
while let Some(error) = self.servo_errors.try_recv() {
self.delegate().notify_error(self, error);
}
}
fn clean_up_destroyed_webview_handles(&self) {
// Remove any webview handles that have been destroyed and would not be upgradable.
// Note that `retain` is O(capacity) because it visits empty buckets, so it may be worth
@ -758,7 +769,11 @@ impl Servo {
},
EmbedderMsg::AllowUnload(webview_id, response_sender) => {
if let Some(webview) = self.get_webview_handle(webview_id) {
let request = AllowOrDenyRequest::new(response_sender, AllowOrDeny::Allow);
let request = AllowOrDenyRequest::new(
response_sender,
AllowOrDeny::Allow,
self.servo_errors.sender(),
);
webview.delegate().request_unload(webview, request);
}
},
@ -824,12 +839,24 @@ impl Servo {
web_resource_request,
response_sender,
) => {
let web_resource_load = WebResourceLoad::new(web_resource_request, response_sender);
match webview_id.and_then(|webview_id| self.get_webview_handle(webview_id)) {
Some(webview) => webview
if let Some(webview) =
webview_id.and_then(|webview_id| self.get_webview_handle(webview_id))
{
let web_resource_load = WebResourceLoad::new(
web_resource_request,
response_sender,
self.servo_errors.sender(),
);
webview
.delegate()
.load_web_resource(webview, web_resource_load),
None => self.delegate().load_web_resource(web_resource_load),
.load_web_resource(webview, web_resource_load);
} else {
let web_resource_load = WebResourceLoad::new(
web_resource_request,
response_sender,
self.servo_errors.sender(),
);
self.delegate().load_web_resource(web_resource_load);
}
},
EmbedderMsg::Panic(webview_id, reason, backtrace) => {
@ -864,9 +891,13 @@ impl Servo {
}
},
EmbedderMsg::RequestAuthentication(webview_id, url, for_proxy, response_sender) => {
let authentication_request =
AuthenticationRequest::new(url.into_url(), for_proxy, response_sender);
if let Some(webview) = self.get_webview_handle(webview_id) {
let authentication_request = AuthenticationRequest::new(
url.into_url(),
for_proxy,
response_sender,
self.servo_errors.sender(),
);
webview
.delegate()
.request_authentication(webview, authentication_request);
@ -879,6 +910,7 @@ impl Servo {
allow_deny_request: AllowOrDenyRequest::new(
response_sender,
AllowOrDeny::Deny,
self.servo_errors.sender(),
),
};
webview
@ -921,7 +953,11 @@ impl Servo {
EmbedderMsg::RequestDevtoolsConnection(response_sender) => {
self.delegate().request_devtools_connection(
self,
AllowOrDenyRequest::new(response_sender, AllowOrDeny::Deny),
AllowOrDenyRequest::new(
response_sender,
AllowOrDeny::Deny,
self.servo_errors.sender(),
),
);
},
EmbedderMsg::PlayGamepadHapticEffect(

View file

@ -0,0 +1,56 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */
use crossbeam_channel::{Receiver, Sender, TryRecvError, unbounded};
use log::warn;
use crate::ServoError;
/// Sender for errors raised by delegate request objects.
///
/// This allows errors to be raised asynchronously.
pub(crate) struct ServoErrorSender {
sender: Sender<ServoError>,
}
impl ServoErrorSender {
pub(crate) fn raise_response_send_error(&self, error: bincode::Error) {
if let Err(error) = self.sender.send(ServoError::ResponseFailedToSend(error)) {
warn!("Failed to send Servo error: {error:?}");
}
}
}
/// Channel for errors raised by [`WebViewDelegate`] request objects.
///
/// This allows errors to be raised asynchronously.
pub(crate) struct ServoErrorChannel {
sender: Sender<ServoError>,
receiver: Receiver<ServoError>,
}
impl Default for ServoErrorChannel {
fn default() -> Self {
let (sender, receiver) = unbounded();
Self { sender, receiver }
}
}
impl ServoErrorChannel {
pub(crate) fn sender(&self) -> ServoErrorSender {
ServoErrorSender {
sender: self.sender.clone(),
}
}
pub(crate) fn try_recv(&self) -> Option<ServoError> {
match self.receiver.try_recv() {
Ok(result) => Some(result),
Err(error) => {
debug_assert_eq!(error, TryRecvError::Empty);
None
},
}
}
}

View file

@ -5,7 +5,7 @@
use crate::Servo;
use crate::webview_delegate::{AllowOrDenyRequest, WebResourceLoad};
#[derive(Clone, Copy, Debug, Hash, PartialEq)]
#[derive(Debug)]
pub enum ServoError {
/// The channel to the off-the-main-thread web engine has been lost. No further
/// attempts to communicate will happen. This is an unrecoverable error in Servo.
@ -13,6 +13,8 @@ pub enum ServoError {
/// The devtools server, used to expose pages to remote web inspectors has failed
/// to start.
DevtoolsFailedToStart,
/// Failed to send response to delegate request.
ResponseFailedToSend(bincode::Error),
}
pub trait ServoDelegate {

View file

@ -17,6 +17,7 @@ use serde::Serialize;
use url::Url;
use webrender_api::units::{DeviceIntPoint, DeviceIntRect, DeviceIntSize};
use crate::responders::ServoErrorSender;
use crate::{ConstellationProxy, WebView};
/// A request to navigate a [`WebView`] or one of its inner frames. This can be handled
@ -95,6 +96,8 @@ impl<T: Serialize> Drop for IpcResponder<T> {
.default_response
.take()
.expect("Guaranteed by inherent impl");
// Dont notify embedder about send errors for the default response,
// since they didnt send anything and probably dont care.
let _ = self.response_sender.send(response);
}
}
@ -122,22 +125,30 @@ impl PermissionRequest {
}
}
pub struct AllowOrDenyRequest(IpcResponder<AllowOrDeny>);
pub struct AllowOrDenyRequest(IpcResponder<AllowOrDeny>, ServoErrorSender);
impl AllowOrDenyRequest {
pub(crate) fn new(
response_sender: IpcSender<AllowOrDeny>,
default_response: AllowOrDeny,
error_sender: ServoErrorSender,
) -> Self {
Self(IpcResponder::new(response_sender, default_response))
Self(
IpcResponder::new(response_sender, default_response),
error_sender,
)
}
pub fn allow(mut self) {
let _ = self.0.send(AllowOrDeny::Allow);
if let Err(error) = self.0.send(AllowOrDeny::Allow) {
self.1.raise_response_send_error(error);
}
}
pub fn deny(mut self) {
let _ = self.0.send(AllowOrDeny::Deny);
if let Err(error) = self.0.send(AllowOrDeny::Deny) {
self.1.raise_response_send_error(error);
}
}
}
@ -148,6 +159,7 @@ pub struct AuthenticationRequest {
pub(crate) url: Url,
pub(crate) for_proxy: bool,
pub(crate) responder: IpcResponder<Option<AuthenticationResponse>>,
pub(crate) error_sender: ServoErrorSender,
}
impl AuthenticationRequest {
@ -155,11 +167,13 @@ impl AuthenticationRequest {
url: Url,
for_proxy: bool,
response_sender: IpcSender<Option<AuthenticationResponse>>,
error_sender: ServoErrorSender,
) -> Self {
Self {
url,
for_proxy,
responder: IpcResponder::new(response_sender, None),
error_sender,
}
}
@ -173,9 +187,12 @@ impl AuthenticationRequest {
}
/// Respond to the [`AuthenticationRequest`] with the given username and password.
pub fn authenticate(mut self, username: String, password: String) {
let _ = self
if let Err(error) = self
.responder
.send(Some(AuthenticationResponse { username, password }));
.send(Some(AuthenticationResponse { username, password }))
{
self.error_sender.raise_response_send_error(error);
}
}
}
@ -185,16 +202,19 @@ impl AuthenticationRequest {
pub struct WebResourceLoad {
pub request: WebResourceRequest,
pub(crate) responder: IpcResponder<WebResourceResponseMsg>,
pub(crate) error_sender: ServoErrorSender,
}
impl WebResourceLoad {
pub(crate) fn new(
web_resource_request: WebResourceRequest,
response_sender: IpcSender<WebResourceResponseMsg>,
error_sender: ServoErrorSender,
) -> Self {
Self {
request: web_resource_request,
responder: IpcResponder::new(response_sender, WebResourceResponseMsg::DoNotIntercept),
error_sender,
}
}
@ -205,11 +225,14 @@ impl WebResourceLoad {
/// Intercept this [`WebResourceLoad`] and control the response via the returned
/// [`InterceptedWebResourceLoad`].
pub fn intercept(mut self, response: WebResourceResponse) -> InterceptedWebResourceLoad {
let _ = self.responder.send(WebResourceResponseMsg::Start(response));
if let Err(error) = self.responder.send(WebResourceResponseMsg::Start(response)) {
self.error_sender.raise_response_send_error(error);
}
InterceptedWebResourceLoad {
request: self.request.clone(),
response_sender: self.responder.into_inner(),
finished: false,
error_sender: self.error_sender,
}
}
}
@ -223,28 +246,38 @@ pub struct InterceptedWebResourceLoad {
pub request: WebResourceRequest,
pub(crate) response_sender: IpcSender<WebResourceResponseMsg>,
pub(crate) finished: bool,
pub(crate) error_sender: ServoErrorSender,
}
impl InterceptedWebResourceLoad {
/// Send a chunk of response body data. It's possible to make subsequent calls to
/// this method when streaming body data.
pub fn send_body_data(&self, data: Vec<u8>) {
let _ = self
if let Err(error) = self
.response_sender
.send(WebResourceResponseMsg::SendBodyData(data));
.send(WebResourceResponseMsg::SendBodyData(data))
{
self.error_sender.raise_response_send_error(error);
}
}
/// Finish this [`InterceptedWebResourceLoad`] and complete the response.
pub fn finish(mut self) {
let _ = self
if let Err(error) = self
.response_sender
.send(WebResourceResponseMsg::FinishLoad);
.send(WebResourceResponseMsg::FinishLoad)
{
self.error_sender.raise_response_send_error(error);
}
self.finished = true;
}
/// Cancel this [`InterceptedWebResourceLoad`], which will trigger a network error.
pub fn cancel(mut self) {
let _ = self
if let Err(error) = self
.response_sender
.send(WebResourceResponseMsg::CancelLoad);
.send(WebResourceResponseMsg::CancelLoad)
{
self.error_sender.raise_response_send_error(error);
}
self.finished = true;
}
}
@ -252,9 +285,12 @@ impl InterceptedWebResourceLoad {
impl Drop for InterceptedWebResourceLoad {
fn drop(&mut self) {
if !self.finished {
let _ = self
if let Err(error) = self
.response_sender
.send(WebResourceResponseMsg::FinishLoad);
.send(WebResourceResponseMsg::FinishLoad)
{
self.error_sender.raise_response_send_error(error);
}
}
}
}
@ -430,3 +466,200 @@ pub trait WebViewDelegate {
pub(crate) struct DefaultWebViewDelegate;
impl WebViewDelegate for DefaultWebViewDelegate {}
#[test]
fn test_allow_deny_request() {
use ipc_channel::ipc;
use crate::ServoErrorChannel;
for default_response in [AllowOrDeny::Allow, AllowOrDeny::Deny] {
// Explicit allow yields allow and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AllowOrDenyRequest::new(sender, default_response, errors.sender());
request.allow();
assert_eq!(receiver.try_recv().ok(), Some(AllowOrDeny::Allow));
assert_eq!(receiver.try_recv().ok(), None);
assert!(errors.try_recv().is_none());
// Explicit deny yields deny and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AllowOrDenyRequest::new(sender, default_response, errors.sender());
request.deny();
assert_eq!(receiver.try_recv().ok(), Some(AllowOrDeny::Deny));
assert_eq!(receiver.try_recv().ok(), None);
assert!(errors.try_recv().is_none());
// No response yields default response and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AllowOrDenyRequest::new(sender, default_response, errors.sender());
drop(request);
assert_eq!(receiver.try_recv().ok(), Some(default_response));
assert_eq!(receiver.try_recv().ok(), None);
assert!(errors.try_recv().is_none());
// Explicit allow when receiver disconnected yields error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AllowOrDenyRequest::new(sender, default_response, errors.sender());
drop(receiver);
request.allow();
assert!(errors.try_recv().is_some());
// Explicit deny when receiver disconnected yields error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AllowOrDenyRequest::new(sender, default_response, errors.sender());
drop(receiver);
request.deny();
assert!(errors.try_recv().is_some());
// No response when receiver disconnected yields no error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AllowOrDenyRequest::new(sender, default_response, errors.sender());
drop(receiver);
drop(request);
assert!(errors.try_recv().is_none());
}
}
#[test]
fn test_authentication_request() {
use ipc_channel::ipc;
use crate::ServoErrorChannel;
let url = Url::parse("https://example.com").expect("Guaranteed by argument");
// Explicit response yields that response and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AuthenticationRequest::new(url.clone(), false, sender, errors.sender());
request.authenticate("diffie".to_owned(), "hunter2".to_owned());
assert_eq!(
receiver.try_recv().ok(),
Some(Some(AuthenticationResponse {
username: "diffie".to_owned(),
password: "hunter2".to_owned(),
}))
);
assert_eq!(receiver.try_recv().ok(), None);
assert!(errors.try_recv().is_none());
// No response yields None response and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AuthenticationRequest::new(url.clone(), false, sender, errors.sender());
drop(request);
assert_eq!(receiver.try_recv().ok(), Some(None));
assert_eq!(receiver.try_recv().ok(), None);
assert!(errors.try_recv().is_none());
// Explicit response when receiver disconnected yields error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AuthenticationRequest::new(url.clone(), false, sender, errors.sender());
drop(receiver);
request.authenticate("diffie".to_owned(), "hunter2".to_owned());
assert!(errors.try_recv().is_some());
// No response when receiver disconnected yields no error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = AuthenticationRequest::new(url.clone(), false, sender, errors.sender());
drop(receiver);
drop(request);
assert!(errors.try_recv().is_none());
}
#[test]
fn test_web_resource_load() {
use http::{HeaderMap, Method, StatusCode};
use ipc_channel::ipc;
use crate::ServoErrorChannel;
let web_resource_request = || WebResourceRequest {
method: Method::GET,
headers: HeaderMap::default(),
url: Url::parse("https://example.com").expect("Guaranteed by argument"),
is_for_main_frame: false,
is_redirect: false,
};
let web_resource_response = || {
WebResourceResponse::new(Url::parse("https://diffie.test").expect("Guaranteed by argument"))
.status_code(StatusCode::IM_A_TEAPOT)
};
// Explicit intercept with explicit cancel yields Start and Cancel and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = WebResourceLoad::new(web_resource_request(), sender, errors.sender());
request.intercept(web_resource_response()).cancel();
assert!(matches!(
receiver.try_recv(),
Ok(WebResourceResponseMsg::Start(_))
));
assert!(matches!(
receiver.try_recv(),
Ok(WebResourceResponseMsg::CancelLoad)
));
assert!(matches!(receiver.try_recv(), Err(_)));
assert!(errors.try_recv().is_none());
// Explicit intercept with no further action yields Start and FinishLoad and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = WebResourceLoad::new(web_resource_request(), sender, errors.sender());
drop(request.intercept(web_resource_response()));
assert!(matches!(
receiver.try_recv(),
Ok(WebResourceResponseMsg::Start(_))
));
assert!(matches!(
receiver.try_recv(),
Ok(WebResourceResponseMsg::FinishLoad)
));
assert!(matches!(receiver.try_recv(), Err(_)));
assert!(errors.try_recv().is_none());
// No response yields DoNotIntercept and nothing else
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = WebResourceLoad::new(web_resource_request(), sender, errors.sender());
drop(request);
assert!(matches!(
receiver.try_recv(),
Ok(WebResourceResponseMsg::DoNotIntercept)
));
assert!(matches!(receiver.try_recv(), Err(_)));
assert!(errors.try_recv().is_none());
// Explicit intercept with explicit cancel when receiver disconnected yields error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = WebResourceLoad::new(web_resource_request(), sender, errors.sender());
drop(receiver);
request.intercept(web_resource_response()).cancel();
assert!(errors.try_recv().is_some());
// Explicit intercept with no further action when receiver disconnected yields error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = WebResourceLoad::new(web_resource_request(), sender, errors.sender());
drop(receiver);
drop(request.intercept(web_resource_response()));
assert!(errors.try_recv().is_some());
// No response when receiver disconnected yields no error
let errors = ServoErrorChannel::default();
let (sender, receiver) = ipc::channel().expect("Failed to create IPC channel");
let request = WebResourceLoad::new(web_resource_request(), sender, errors.sender());
drop(receiver);
drop(request);
assert!(errors.try_recv().is_none());
}

View file

@ -150,7 +150,7 @@ pub enum SimpleDialog {
},
}
#[derive(Debug, Default, Deserialize, Serialize)]
#[derive(Debug, Default, Deserialize, PartialEq, Serialize)]
pub struct AuthenticationResponse {
/// Username for http request authentication
pub username: String,
@ -208,7 +208,7 @@ impl Default for PromptResponse {
}
/// A response to a request to allow or deny an action.
#[derive(Clone, Copy, Deserialize, PartialEq, Serialize)]
#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
pub enum AllowOrDeny {
Allow,
Deny,

View file

@ -165,6 +165,7 @@ class MachCommands(CommandBase):
"fonts",
"hyper_serde",
"layout_2020",
"libservo",
"net",
"net_traits",
"pixels",