webdriver: Add a 10 second timeout for screenshots (servo#40290)

This change adds a 10 second timeout for screenshots. Without this
timeout, tests that fail take a screenshot in a reasonable amount of
time cause the the WebDriver driver to kill the process leading to a
CRASH test result. This causes the results to differ and to obscure what
is really a TIMEOUT.

This behavior is unspecified, but it's still an improvement, as it
properly classifies failures when the WPT is run with WebDriver.

Testing: This causes some WPT tests run with WebDriver to have
their expected result.

Signed-off-by: Martin Robinson <[email protected]>

Author: mrobinson

components/webdriver_server/lib.rs

@@ -27,7 +27,7 @@ use base::id::{BrowsingContextId, WebViewId};
 use base64::Engine;
 use capabilities::ServoCapabilities;
 use cookie::{CookieBuilder, Expiration, SameSite};
-use crossbeam_channel::{Receiver, Sender, after, select, unbounded};
+use crossbeam_channel::{Receiver, RecvTimeoutError, Sender, after, select, unbounded};
 use embedder_traits::{
     EventLoopWaker, ImeEvent, InputEvent, JSValue, JavaScriptEvaluationError,
     JavaScriptEvaluationResultSerializationError, MouseButton, WebDriverCommandMsg,
@@ -75,7 +75,7 @@ use webdriver::server::{self, Session, SessionTeardownKind, WebDriverHandler};
 
 use crate::actions::{InputSourceState, PointerInputState};
 use crate::session::{PageLoadStrategy, WebDriverSession};
-use crate::timeout::DEFAULT_PAGE_LOAD_TIMEOUT;
+use crate::timeout::{DEFAULT_PAGE_LOAD_TIMEOUT, SCREENSHOT_TIMEOUT};
 
 fn extension_routes() -> Vec<(Method, &'static str, ServoExtensionRoute)> {
     vec![
@@ -2307,18 +2307,25 @@ impl Handler {
                 "The requested `rect` has zero width and/or height",
             ));
         }
+
         let webview_id = self.webview_id()?;
         let (sender, receiver) = crossbeam_channel::unbounded();
         self.send_message_to_embedder(WebDriverCommandMsg::TakeScreenshot(
             webview_id, rect, sender,
         ))?;
 
-        let Ok(result) = receiver.recv() else {
-            return Err(WebDriverError::new(
+        let result = match receiver.recv_timeout(SCREENSHOT_TIMEOUT) {
+            Ok(result) => Ok(result),
+            Err(RecvTimeoutError::Timeout) => Err(WebDriverError::new(
+                ErrorStatus::Timeout,
+                "Timed out waiting to take screenshot. Test likely didn't finish.",
+            )),
+            Err(RecvTimeoutError::Disconnected) => Err(WebDriverError::new(
                 ErrorStatus::UnknownError,
-                "Failed to receive TakeScreenshot response",
-            ));
-        };
+                "Could not take screenshot because channel disconnected.",
+            )),
+        }?;
+
         let image = result.map_err(|error| {
             WebDriverError::new(
                 ErrorStatus::UnknownError,

components/webdriver_server/timeout.rs

@@ -2,6 +2,8 @@
  * 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 std::time::Duration;
+
 use serde_json::Value;
 use webdriver::error::{ErrorStatus, WebDriverError, WebDriverResult};
 
@@ -17,6 +19,11 @@ pub(crate) const DEFAULT_PAGE_LOAD_TIMEOUT: u64 = 300_000;
 /// <https://w3c.github.io/webdriver/#dfn-timeouts-configuration>.
 pub(crate) const DEFAULT_IMPLICIT_WAIT: u64 = 0;
 
+/// An amount of time to wait before considering that a screenshot has timed out.
+/// If after 10 seconds the screenshot cannot be taken, assume that the test has
+/// timed out.
+pub(crate) const SCREENSHOT_TIMEOUT: Duration = Duration::from_secs(10);
+
 pub(crate) struct TimeoutsConfiguration {
     pub(crate) script: Option<u64>,
     pub(crate) page_load: Option<u64>,