libservo: Make Servo (and servoshell) more resilient against extreme sizes (#39204)

Make several changes which should address panics and inconsistent
behavior around attempts to set extreme sizes:

1. Limit the minimum size of the `RenderingContext` to 1 pixel by 1
   pixel. This should address problems where users of the API try to
   directly set the size to a zero or negative dimension. In addition,
   improve the documentation around `WebView::resize` to mention this.
2. Clamp values sent in the `WebViewDelegate::request_resize_to` method
   to be at least 1x1. This prevents Servo from sending nonsense values
   to embedders. Improve documentation in this method.
3. In servoshell:
    - More consistently clamp inner and outer window size values.
    - Clamp all resize values to the available screen size, so that
      large screen sizes aren't processed directly.

Testing: This change fixes an existing WPT and adds two new API tests.
Fixes: #36763.
Fixes: #36841.
Fixes: #39141.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

components/servo/lib.rs

@@ -132,6 +132,7 @@ use crate::proxies::ConstellationProxy;
 use crate::responders::ServoErrorChannel;
 pub use crate::servo_delegate::{ServoDelegate, ServoError};
 use crate::webrender_api::FrameReadyParams;
+use crate::webview::MINIMUM_WEBVIEW_SIZE;
 pub use crate::webview::{WebView, WebViewBuilder};
 pub use crate::webview_delegate::{
     AllowOrDenyRequest, AuthenticationRequest, ColorPicker, FormControl, NavigationRequest,
@@ -693,7 +694,9 @@ impl Servo {
             },
             EmbedderMsg::ResizeTo(webview_id, size) => {
                 if let Some(webview) = self.get_webview_handle(webview_id) {
-                    webview.delegate().request_resize_to(webview, size);
+                    webview
+                        .delegate()
+                        .request_resize_to(webview, size.max(MINIMUM_WEBVIEW_SIZE));
                 }
             },
             EmbedderMsg::ShowSimpleDialog(webview_id, prompt_definition) => {

components/servo/tests/common/mod.rs

@@ -56,7 +56,7 @@ pub(crate) fn run_test(
 }
 
 pub struct ServoTest {
-    servo: Servo,
+    pub servo: Rc<Servo>,
     #[allow(dead_code)]
     pub rendering_context: Rc<dyn RenderingContext>,
 }
@@ -101,7 +101,7 @@ impl ServoTest {
         let builder = ServoBuilder::new(rendering_context.clone())
             .event_loop_waker(Box::new(EventLoopWakerImpl(user_event_triggered)));
         let builder = customize(builder);
-        let servo = builder.build();
+        let servo = Rc::new(builder.build());
         Self {
             servo,
             rendering_context,

components/servo/tests/webview.rs

@@ -11,16 +11,19 @@
 
 mod common;
 
+use std::cell::{Cell, RefCell};
 use std::rc::Rc;
 
 use anyhow::ensure;
 use common::{ServoTest, WebViewDelegateImpl, evaluate_javascript, run_api_tests};
-use euclid::Point2D;
+use dpi::PhysicalSize;
+use euclid::{Point2D, Size2D};
 use servo::{
     Cursor, InputEvent, JSValue, JavaScriptEvaluationError, LoadStatus, MouseLeftViewportEvent,
-    MouseMoveEvent, Theme, WebViewBuilder,
+    MouseMoveEvent, Servo, Theme, WebView, WebViewBuilder, WebViewDelegate,
 };
 use url::Url;
+use webrender_api::units::DeviceIntSize;
 
 fn test_create_webview(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
     let delegate = Rc::new(WebViewDelegateImpl::default());
@@ -199,6 +202,94 @@ fn test_cursor_change(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
     Ok(())
 }
 
+/// A test that ensure that negative resize requests do not get passed to the embedder.
+fn test_negative_resize_to_request(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
+    struct WebViewResizeTestDelegate {
+        servo: Rc<Servo>,
+        popup: RefCell<Option<WebView>>,
+        resize_request: Cell<Option<DeviceIntSize>>,
+    }
+
+    impl WebViewDelegate for WebViewResizeTestDelegate {
+        fn request_open_auxiliary_webview(&self, parent_webview: WebView) -> Option<WebView> {
+            let webview = WebViewBuilder::new_auxiliary(&self.servo)
+                .delegate(parent_webview.delegate())
+                .build();
+            self.popup.borrow_mut().replace(webview.clone());
+            Some(webview)
+        }
+
+        fn request_resize_to(&self, _: WebView, requested_outer_size: DeviceIntSize) {
+            self.resize_request.set(Some(requested_outer_size));
+        }
+    }
+
+    let delegate = Rc::new(WebViewResizeTestDelegate {
+        servo: servo_test.servo.clone(),
+        popup: None.into(),
+        resize_request: None.into(),
+    });
+
+    let webview = WebViewBuilder::new(servo_test.servo())
+        .delegate(delegate.clone())
+        .url(
+            Url::parse(
+                "data:text/html,<!DOCTYPE html><script>\
+                    let popup = window.open('about:blank');\
+                    popup.resizeTo(-100, -100);\
+                </script></body>",
+            )
+            .unwrap(),
+        )
+        .build();
+
+    let load_webview = webview.clone();
+    let _ = servo_test.spin(move || Ok(load_webview.load_status() != LoadStatus::Complete));
+
+    let popup = delegate
+        .popup
+        .borrow()
+        .clone()
+        .expect("Should have created popup");
+
+    let load_webview = popup.clone();
+    let _ = servo_test.spin(move || Ok(load_webview.load_status() != LoadStatus::Complete));
+
+    // Resize requests should be floored to 1.
+    ensure!(delegate.resize_request.get() == Some(DeviceIntSize::new(1, 1)));
+
+    // Ensure that the popup WebView is released before the end of the test.
+    *delegate.popup.borrow_mut() = None;
+
+    Ok(())
+}
+
+/// This test verifies that trying to set the WebView size to a negative value does
+/// not crash Servo.
+fn test_resize_webview_zero(servo_test: &ServoTest) -> Result<(), anyhow::Error> {
+    let delegate = Rc::new(WebViewDelegateImpl::default());
+    let webview = WebViewBuilder::new(servo_test.servo())
+        .delegate(delegate.clone())
+        .url(
+            Url::parse(
+                "data:text/html,<!DOCTYPE html><style> html { cursor: crosshair; margin: 0}</style><body>hello</body>",
+            )
+            .unwrap(),
+        )
+        .build();
+
+    webview.focus();
+    webview.show(true);
+
+    webview.move_resize(Size2D::new(-100.0, -100.0).into());
+    webview.resize(PhysicalSize::new(0, 0));
+
+    let load_webview = webview.clone();
+    let _ = servo_test.spin(move || Ok(load_webview.load_status() != LoadStatus::Complete));
+
+    Ok(())
+}
+
 fn main() {
     run_api_tests!(
         test_create_webview,
@@ -206,6 +297,8 @@ fn main() {
         test_evaluate_javascript_basic,
         test_evaluate_javascript_panic,
         test_theme_change,
+        test_negative_resize_to_request,
+        test_resize_webview_zero,
         // This test needs to be last, as it tests creating and dropping
         // a WebView right before shutdown.
         test_create_webview_and_immediately_drop_webview_before_shutdown

components/servo/webview.rs

@@ -27,6 +27,8 @@ use crate::javascript_evaluator::JavaScriptEvaluator;
 use crate::webview_delegate::{DefaultWebViewDelegate, WebViewDelegate};
 use crate::{ConstellationProxy, Servo, WebRenderDebugOption};
 
+pub(crate) const MINIMUM_WEBVIEW_SIZE: Size2D<i32, DevicePixel> = Size2D::new(1, 1);
+
 /// A handle to a Servo webview. If you clone this handle, it does not create a new webview,
 /// but instead creates a new handle to the webview. Once the last handle is dropped, Servo
 /// considers that the webview has closed and will clean up all associated resources related
@@ -343,6 +345,9 @@ impl WebView {
             return;
         }
 
+        let rect =
+            DeviceRect::from_origin_and_size(rect.min, rect.size().max(Size2D::new(1.0, 1.0)));
+
         self.inner_mut().rect = rect;
         self.inner()
             .compositor
@@ -350,7 +355,15 @@ impl WebView {
             .move_resize_webview(self.id(), rect);
     }
 
+    /// Request that the given [`WebView`]'s rendering area be resized. Note that the
+    /// minimum size for a WebView is 1 pixel by 1 pixel so any requested size will be
+    /// clamped by that value.
     pub fn resize(&self, new_size: PhysicalSize<u32>) {
+        let new_size = PhysicalSize {
+            width: new_size.width.max(MINIMUM_WEBVIEW_SIZE.width as u32),
+            height: new_size.height.max(MINIMUM_WEBVIEW_SIZE.height as u32),
+        };
+
         self.inner()
             .compositor
             .borrow_mut()

components/servo/webview_delegate.rs

@@ -474,7 +474,11 @@ pub trait WebViewDelegate {
     fn request_unload(&self, _webview: WebView, _unload_request: AllowOrDenyRequest) {}
     /// Move the window to a point.
     fn request_move_to(&self, _webview: WebView, _: DeviceIntPoint) {}
-    /// Try to resize the window that contains this [`WebView`] to the provided outer size.
+    /// Try to resize the window that contains this [`WebView`] to the provided outer
+    /// size. These resize requests can come from page content. Servo will ensure that the
+    /// values are greater than zero, but it is up to the embedder to limit the maximum
+    /// size. For instance, a reasonable limitation might be that the final size is no
+    /// larger than the screen size.
     fn request_resize_to(&self, _webview: WebView, _requested_outer_size: DeviceIntSize) {}
     /// Whether or not to allow script to open a new `WebView`. If not handled by the
     /// embedder, these requests are automatically denied.

ports/servoshell/desktop/headed_window.rs

@@ -49,12 +49,10 @@ use {
 use super::app_state::RunningAppState;
 use super::geometry::{winit_position_to_euclid_point, winit_size_to_euclid_size};
 use super::keyutils::{CMD_OR_ALT, keyboard_event_from_winit};
-use super::window_trait::{
+use super::window_trait::{LINE_HEIGHT, LINE_WIDTH, PIXEL_DELTA_FACTOR, WindowPortsMethods};
-    LINE_HEIGHT, LINE_WIDTH, MIN_INNER_HEIGHT, MIN_INNER_WIDTH, PIXEL_DELTA_FACTOR,
-    WindowPortsMethods,
-};
 use crate::desktop::accelerated_gl_media::setup_gl_accelerated_media;
 use crate::desktop::keyutils::CMD_OR_CONTROL;
+use crate::desktop::window_trait::MIN_WINDOW_INNER_SIZE;
 use crate::prefs::ServoShellPreferences;
 
 pub struct Window {
@@ -99,7 +97,10 @@ impl Window {
             .with_decorations(!no_native_titlebar)
             .with_transparent(no_native_titlebar)
             .with_inner_size(LogicalSize::new(inner_size.width, inner_size.height))
-            .with_min_inner_size(LogicalSize::new(MIN_INNER_WIDTH, MIN_INNER_HEIGHT))
+            .with_min_inner_size(LogicalSize::new(
+                MIN_WINDOW_INNER_SIZE.width,
+                MIN_WINDOW_INNER_SIZE.height,
+            ))
             // Must be invisible at startup; accesskit_winit setup needs to
             // happen before the window is shown for the first time.
             .with_visible(false);
@@ -489,26 +490,36 @@ impl WindowPortsMethods for Window {
     }
 
     fn request_resize(&self, _: &WebView, new_outer_size: DeviceIntSize) -> Option<DeviceIntSize> {
+        // Allocate space for the window deocrations, but do not let the inner size get
+        // smaller than `MIN_WINDOW_INNER_SIZE` or larger than twice the screen size.
+        let inner_size = self.winit_window.inner_size();
         let outer_size = self.winit_window.outer_size();
+        let decoration_size: DeviceIntSize = Size2D::new(
+            outer_size.height - inner_size.height,
+            outer_size.width - inner_size.width,
+        )
+        .cast();
+
+        let screen_size = (self.screen_size.to_f32() * self.hidpi_scale_factor()).to_i32();
+        let new_outer_size =
+            new_outer_size.clamp(MIN_WINDOW_INNER_SIZE + decoration_size, screen_size * 2);
+
         if outer_size.width == new_outer_size.width as u32 &&
             outer_size.height == new_outer_size.height as u32
         {
             return Some(new_outer_size);
         }
 
-        let inner_size = self.winit_window.inner_size();
+        let new_inner_size = new_outer_size - decoration_size;
-        let decoration_height = outer_size.height - inner_size.height;
-        let decoration_width = outer_size.width - inner_size.width;
-
         self.winit_window
-            .request_inner_size::<PhysicalSize<i32>>(PhysicalSize::new(
+            .request_inner_size(PhysicalSize::new(
-                new_outer_size.width - decoration_width as i32,
+                new_inner_size.width,
-                new_outer_size.height - decoration_height as i32,
+                new_inner_size.height,
             ))
             .map(|resulting_size| {
                 DeviceIntSize::new(
-                    (resulting_size.width + decoration_width) as i32,
+                    resulting_size.width as i32 + decoration_size.width,
-                    (resulting_size.height + decoration_height) as i32,
+                    resulting_size.height as i32 + decoration_size.height,
                 )
             })
     }
@@ -777,12 +788,6 @@ impl WindowPortsMethods for Window {
             return;
         }
         self.toolbar_height.set(height);
-        // Prevent the inner area from being 0 pixels wide or tall
-        // this prevents a crash in the compositor due to invalid surface size
-        self.winit_window.set_min_inner_size(Some(PhysicalSize::new(
-            MIN_INNER_WIDTH,
-            MIN_INNER_HEIGHT.max((self.toolbar_height() * self.hidpi_scale_factor()).0 as i32),
-        )));
     }
 
     fn rendering_context(&self) -> Rc<dyn RenderingContext> {

ports/servoshell/desktop/headless_window.rs

@@ -20,7 +20,7 @@ use servo::{RenderingContext, ScreenGeometry, SoftwareRenderingContext, WebView}
 use winit::dpi::PhysicalSize;
 
 use super::app_state::RunningAppState;
-use crate::desktop::window_trait::{MIN_INNER_HEIGHT, MIN_INNER_WIDTH, WindowPortsMethods};
+use crate::desktop::window_trait::{MIN_WINDOW_INNER_SIZE, WindowPortsMethods};
 use crate::prefs::ServoShellPreferences;
 
 pub struct Window {
@@ -87,15 +87,10 @@ impl WindowPortsMethods for Window {
         self.window_position.set(point);
     }
 
-    fn request_resize(
+    fn request_resize(&self, webview: &WebView, new_size: DeviceIntSize) -> Option<DeviceIntSize> {
-        &self,
+        // Do not let the window size get smaller than `MIN_WINDOW_INNER_SIZE` or larger
-        webview: &WebView,
+        // than twice the screen size.
-        outer_size: DeviceIntSize,
+        let new_size = new_size.clamp(MIN_WINDOW_INNER_SIZE, self.screen_size * 2);
-    ) -> Option<DeviceIntSize> {
-        let new_size = DeviceIntSize::new(
-            outer_size.width.max(MIN_INNER_WIDTH),
-            outer_size.height.max(MIN_INNER_HEIGHT),
-        );
         if self.inner_size.get() == new_size {
             return Some(new_size);
         }
@@ -105,10 +100,10 @@ impl WindowPortsMethods for Window {
         // Because we are managing the rendering surface ourselves, there will be no other
         // notification (such as from the display manager) that it has changed size, so we
         // must notify the compositor here.
-        webview.move_resize(outer_size.to_f32().into());
+        webview.move_resize(new_size.to_f32().into());
         webview.resize(PhysicalSize::new(
-            outer_size.width as u32,
+            new_size.width as u32,
-            outer_size.height as u32,
+            new_size.height as u32,
         ));
 
         Some(new_size)

ports/servoshell/desktop/window_trait.rs

@@ -24,8 +24,7 @@ pub(crate) const PIXEL_DELTA_FACTOR: f64 = 4.0;
 
 /// <https://github.com/web-platform-tests/wpt/blob/9320b1f724632c52929a3fdb11bdaf65eafc7611/webdriver/tests/classic/set_window_rect/set.py#L287-L290>
 /// "A window size of 10x10px shouldn't be supported by any browser."
-pub(crate) const MIN_INNER_WIDTH: i32 = 20;
+pub(crate) const MIN_WINDOW_INNER_SIZE: DeviceIntSize = DeviceIntSize::new(100, 100);
-pub(crate) const MIN_INNER_HEIGHT: i32 = 20;
 
 pub trait WindowPortsMethods {
     fn id(&self) -> winit::window::WindowId;

tests/wpt/meta/css/cssom-view/resizeTo-negative.html.ini

@@ -1,2 +0,0 @@
-[resizeTo-negative.html]
-  expected: CRASH