From fd5e2feff6ea25bed371a99cc732b3968d621505 Mon Sep 17 00:00:00 2001 From: Helen Kwok <107752876+helenkwok@users.noreply.github.com> Date: Fri, 6 Nov 2026 10:54:07 -0500 Subject: [PATCH] web-api: drop a timed-out call, and give screenshot() one data field (#620) A call that timed out left the app's request future running, so an app that releases a request when its future is dropped (the desktop ControlRequest abandons a request nobody waits for) ran the call late, after the page had been told it failed. call() now drives the request future itself and drops it when the timer wins. The desktop control protocols name the base64 PNG of ui.screenshot differently (pngBase64, png_base64, png, base64), so screenshot() gave the page a different shape in each app. It now also puts the PNG in data and leaves the app's own field where it is. The timer handle is kept as the environment returned it: Node returns an object, not a number, so a numeric id never cleared the timer and it fired after its closure was dropped. --- crates/web-api/README.md | 6 ++- crates/web-api/src/lib.rs | 2 + crates/web-api/src/page.rs | 95 ++++++++++++++++++++---------------- crates/web-api/src/reply.rs | 68 ++++++++++++++++++++++++++ crates/web-api/tests/page.rs | 55 ++++++++++++++++++++- 5 files changed, 180 insertions(+), 46 deletions(-) create mode 100644 crates/web-api/src/reply.rs diff --git a/crates/web-api/README.md b/crates/web-api/README.md index d14bbfb1..e2d17d13 100644 --- a/crates/web-api/README.md +++ b/crates/web-api/README.md @@ -9,7 +9,9 @@ control channel. replacing an existing `window.`: missing functions are added, an app that already has its own keeps them. Each function returns a Promise that resolves with the method's `result` or rejects with an `Error` carrying `error`. Every call has a timeout (30 s by default, `PageApi::with_timeout`); -when the app does not answer in time, the error says so. +when the app does not answer in time, the error says so and the app's request future is dropped, so an app that releases a request when its future goes away (the desktop `ControlRequest` has `abandon`) never runs a call the page was told had failed. + +`screenshot()` resolves with the reply the app's control method sends, plus `data` when the reply carries the PNG inline: the base64 PNG, whatever field the app names it (`pngBase64`, `png_base64`, `png` or `base64`). A reply with no inline PNG, such as one the app wrote to a `path`, has no `data` and is unchanged. Which functions are installed follows the app's `Methods` table: only the functions whose control method the table provides are installed; `request` is always there. @@ -21,7 +23,7 @@ method the table provides are installed; `request` is always there. | `commands` | `commands()` | `commands` | `engine.commands {}` | | `docInspect` | `docInspect(params)` | `doc_inspect` | `document.inspect` | | `uiInspect` | `uiInspect(params)` | `ui_inspect` | `ui.inspect` | -| `screenshot` | `screenshot(params)` | `screenshot` | `ui.screenshot` (with `data: true`) | +| `screenshot` | `screenshot(params)` | `screenshot` | `ui.screenshot` (with `data: true`); an inline PNG is always also in `data` | `Methods::default()` is the desktop control-server method names; an app whose control channel names a method differently, or lacks one, says so in its table (`Methods::NONE` is the empty table). diff --git a/crates/web-api/src/lib.rs b/crates/web-api/src/lib.rs index 434d5fdb..f02f75b7 100644 --- a/crates/web-api/src/lib.rs +++ b/crates/web-api/src/lib.rs @@ -13,6 +13,7 @@ //! `screenshot(params)`; each function other than `request` is installed only when the app's //! [`methods::Methods`] table provides its control method. //! - [`methods`] is the target-independent table naming the control method behind each function. +//! - [`reply`] shapes replies without a browser: `screenshot()` carries an inline PNG as `data` in every app. //! //! The app hands the crate its request function (its control channel) and a [`methods::Methods`] //! table; the crate never knows the app's internals. @@ -35,6 +36,7 @@ //! ``` pub mod methods; +pub mod reply; #[cfg(target_arch = "wasm32")] pub mod page; diff --git a/crates/web-api/src/page.rs b/crates/web-api/src/page.rs index 538601f8..b59b4829 100644 --- a/crates/web-api/src/page.rs +++ b/crates/web-api/src/page.rs @@ -4,16 +4,19 @@ //! Every function goes through one path, [`PageApi::call`], with the page's timeout; replies //! resolve with the method's `result` or reject with an `Error` carrying `error`. +use std::cell::{Cell, RefCell}; use std::future::Future; use std::pin::Pin; use std::rc::Rc; +use std::task::{Poll, Waker}; use serde_json::{Value, json}; use wasm_bindgen::JsCast; use wasm_bindgen::prelude::{Closure, JsValue}; -use wasm_bindgen_futures::{JsFuture, future_to_promise}; +use wasm_bindgen_futures::future_to_promise; use crate::methods::Methods; +use crate::reply::screenshot_data; /// One control request: method and params in, the protocol's reply object out (`{ok, result}` or /// `{ok: false, error}`). The app builds this from its control channel — send a `ControlRequest`, @@ -126,7 +129,7 @@ impl PageApi { Closure:: js_sys::Promise>::new(move |params: JsValue| { let page = page.clone(); let params = screenshot_params(from_js(¶ms)); - future_to_promise(async move { request_promise_outcome(&page.call(method, params).await) }) + future_to_promise(async move { request_promise_outcome(&screenshot_data(page.call(method, params).await)) }) }) .into_js_value(), )?; @@ -136,30 +139,35 @@ impl PageApi { } /// One control request: the single path through the app's channel that every function takes, - /// with the page's timeout. If the app does not answer within the timeout, the error says so — - /// a hidden tab answers when it is shown again. + /// with the page's timeout. If the app does not answer within the timeout, the error says so + /// (a hidden tab answers when it is shown again) and the app's request future is dropped: an app + /// that releases a request when its future goes away never runs, late, a call the page was told + /// had failed. async fn call(&self, method: &str, params: Value) -> Value { let timeout = Timeout::new(self.timeout_ms); - let request = self.request.clone(); - let method = method.to_string(); - let sent = method.clone(); - let reply = future_to_promise(async move { - let reply = request(sent, params).await; - Ok(to_js(&reply)) - }); - let raced = js_sys::Promise::race(&js_sys::Array::of2(&reply, &timeout.promise)); - let outcome = JsFuture::from(raced).await; + let mut work = (self.request)(method.to_string(), params); + let reply = std::future::poll_fn(|cx| { + if let Poll::Ready(reply) = work.as_mut().poll(cx) { + return Poll::Ready(Some(reply)); + } + if timeout.fired.get() { + return Poll::Ready(None); + } + *timeout.waker.borrow_mut() = Some(cx.waker().clone()); + Poll::Pending + }) + .await; + drop(work); // an unanswered request is released here, before the error goes back drop(timeout); // clear the timer and free its callback once the race is over - match outcome { - Err(_) => json!({ + reply.unwrap_or_else(|| { + json!({ "ok": false, "error": format!( "{} did not answer {} within {} s (a hidden tab answers when it is shown again)", self.app, method, self.timeout_ms / 1000.0 ), - }), - Ok(value) => from_js(&value), - } + }) + }) } } @@ -175,41 +183,43 @@ fn screenshot_params(mut params: Value) -> Value { } } -/// A promise that rejects `ms` milliseconds from now. Dropping it clears the timer (harmless once -/// fired) and frees the timer's callback, so a call leaves nothing behind. The timer comes from -/// `setTimeout` on the global object, reached through `Reflect` — no web-sys, so the crate keeps -/// working in any JS realm. +/// A timer for one call: `fired` turns true `ms` milliseconds from now and wakes the call. Dropping +/// it clears the timer (harmless once fired) and frees the timer's callback, so a call leaves +/// nothing behind. The timer comes from `setTimeout` on the global object, reached through +/// `Reflect` — no web-sys, so the crate keeps working in any JS realm. struct Timeout { - promise: js_sys::Promise, - id: f64, - _fire: Option>, + fired: Rc>, + waker: Rc>>, + id: JsValue, + _fire: Closure, } impl Timeout { fn new(ms: f64) -> Self { - let mut fire = None; - let mut id = f64::INFINITY; - // `Promise::new` runs its executor before returning, so `fire` and `id` are set here. - let promise = js_sys::Promise::new(&mut |_resolve: js_sys::Function, reject: js_sys::Function| { - let callback = Closure::::new(move || { - let _ = reject.call1(&JsValue::NULL, &JsValue::NULL); - }); - id = set_timeout(callback.as_ref().clone(), ms); - fire = Some(callback); + let fired = Rc::new(Cell::new(false)); + let waker: Rc>> = Rc::default(); + let (set, wake) = (fired.clone(), waker.clone()); + let callback = Closure::::new(move || { + set.set(true); + if let Some(waker) = wake.borrow_mut().take() { + waker.wake(); + } }); - Self { promise, id, _fire: fire } + let id = set_timeout(callback.as_ref().clone(), ms); + Self { fired, waker, id, _fire: callback } } } impl Drop for Timeout { fn drop(&mut self) { - clear_timeout(self.id); + clear_timeout(&self.id); } } /// The `setTimeout` global, reached through the global object + `Reflect` (no web-sys). Returns the -/// timer id; a missing `setTimeout` yields an id of infinity, which [`clear_timeout`] ignores. -fn set_timeout(callback: JsValue, ms: f64) -> f64 { +/// timer handle as the environment gave it, to hand back to [`clear_timeout`]: a number in a +/// browser, an object in Node. A missing `setTimeout` yields `undefined`, which `clearTimeout` ignores. +fn set_timeout(callback: JsValue, ms: f64) -> JsValue { let g = global(); get_prop(&g, "setTimeout") .filter(|v| v.is_function()) @@ -217,16 +227,15 @@ fn set_timeout(callback: JsValue, ms: f64) -> f64 { let f: js_sys::Function = f.unchecked_into(); f.call2(&g, &callback, &JsValue::from_f64(ms)).ok() }) - .and_then(|v| v.as_f64()) - .unwrap_or(f64::INFINITY) + .unwrap_or(JsValue::UNDEFINED) } -/// Clear a `setTimeout` id, if the environment has `clearTimeout`. -fn clear_timeout(id: f64) { +/// Clear a timer by the handle [`set_timeout`] returned, if the environment has `clearTimeout`. +fn clear_timeout(id: &JsValue) { let g = global(); if let Some(f) = get_prop(&g, "clearTimeout").filter(|v| v.is_function()) { let f: js_sys::Function = f.unchecked_into(); - let _ = f.call1(&g, &JsValue::from_f64(id)); + let _ = f.call1(&g, id); } } diff --git a/crates/web-api/src/reply.rs b/crates/web-api/src/reply.rs new file mode 100644 index 00000000..0a00a9a7 --- /dev/null +++ b/crates/web-api/src/reply.rs @@ -0,0 +1,68 @@ +//! Reply shaping that needs no browser, so it is natively tested. + +use serde_json::Value; + +/// The names the desktop control protocols give the base64 PNG of a screenshot reply, in the order +/// they are looked for. VectorCraft sends `pngBase64` (when asked with `data: true`), SoundCraft +/// `png_base64`, WordCraft `png`, GridCraft `base64`. +const PNG_FIELDS: [&str; 4] = ["pngBase64", "png_base64", "png", "base64"]; + +/// A successful `screenshot` reply with the PNG also under `data`, whatever the app calls it, so a +/// page reads `const { data } = await window..screenshot()` in every app. The app's own field +/// stays where it is. A reply with no inline PNG (the app wrote it to a `path`), a reply that +/// already has a `data` key of any type, and an error reply are returned as they are. +pub fn screenshot_data(mut reply: Value) -> Value { + if reply.get("ok").and_then(Value::as_bool) == Some(false) { + return reply; + } + let Some(result) = reply.get_mut("result").and_then(Value::as_object_mut) else { + return reply; + }; + if result.contains_key("data") { + return reply; + } + if let Some(png) = PNG_FIELDS.iter().find_map(|field| result.get(*field).filter(|v| v.is_string()).cloned()) { + result.insert("data".to_string(), png); + } + reply +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::screenshot_data; + + #[test] + fn every_apps_field_name_becomes_data_and_stays_where_it_was() { + for field in ["pngBase64", "png_base64", "png", "base64"] { + let reply = json!({"ok": true, "result": {field: "UE5H", "width": 2}}); + let out = screenshot_data(reply); + assert_eq!(out["result"]["data"], "UE5H", "{field}"); + assert_eq!(out["result"][field], "UE5H", "{field} is kept"); + assert_eq!(out["result"]["width"], 2); + } + } + + #[test] + fn a_reply_with_its_own_data_a_path_only_or_an_error_is_left_alone() { + for reply in [ + json!({"ok": true, "result": {"data": "own", "png": "other"}}), + json!({"ok": true, "result": {"data": null, "png": "UE5H"}}), + json!({"ok": true, "result": {"data": {"n": 1}, "png": "UE5H"}}), + json!({"ok": true, "result": {"path": "/tmp/a.png", "width": 2}}), + json!({"ok": false, "error": "boom"}), + json!({"ok": false, "error": "boom", "result": {"png": "UE5H"}}), + json!({"ok": true, "result": null}), + json!({"ok": true, "result": {"png": 7}}), + ] { + assert_eq!(screenshot_data(reply.clone()), reply); + } + } + + #[test] + fn the_first_field_in_the_list_wins() { + let out = screenshot_data(json!({"ok": true, "result": {"png": "b", "pngBase64": "a"}})); + assert_eq!(out["result"]["data"], "a"); + } +} diff --git a/crates/web-api/tests/page.rs b/crates/web-api/tests/page.rs index 092fa296..72fa81e1 100644 --- a/crates/web-api/tests/page.rs +++ b/crates/web-api/tests/page.rs @@ -2,7 +2,7 @@ //! Node tests for the page part: a canned control channel that records what it is asked, and the //! `window.` functions called the way a page calls them. No browser. -use std::cell::RefCell; +use std::cell::{Cell, RefCell}; use std::rc::Rc; use serde_json::{Value, json}; @@ -159,3 +159,56 @@ async fn timeout_gives_an_error_saying_so() { let message = String::from(err.unchecked_into::().message()); assert!(message.contains("vectorcraft did not answer engine.execute within 0.01 s"), "{message}"); } + +thread_local! { + /// Set when the fake control channel's request future is dropped before it answered. + static RELEASED: Cell = const { Cell::new(false) }; +} + +struct ReleaseOnDrop; + +impl Drop for ReleaseOnDrop { + fn drop(&mut self) { + RELEASED.with(|r| r.set(true)); + } +} + +#[wasm_bindgen_test] +async fn a_timed_out_call_drops_the_apps_request_so_the_app_can_release_it() { + reset(); + RELEASED.with(|r| r.set(false)); + let never: Request = Rc::new(|_method: String, _params: Value| { + Box::pin(async { + let _release = ReleaseOnDrop; + let _ = JsFuture::from(js_sys::Promise::new(&mut |_, _| {})).await; + Value::Null + }) + }); + Rc::new(PageApi::new("vectorcraft", Methods::default(), never).with_timeout(10)).install().expect("install"); + assert!(call("commands", &[]).await.is_err(), "the call times out"); + assert!(RELEASED.with(Cell::get), "the request future was dropped when the call timed out"); +} + +#[wasm_bindgen_test] +async fn an_answered_call_does_not_look_timed_out() { + reset(); + RELEASED.with(|r| r.set(false)); + let quick: Request = Rc::new(|_method: String, _params: Value| { + Box::pin(async { + let _release = ReleaseOnDrop; + json!({"ok": true, "result": {"fine": true}}) + }) + }); + Rc::new(PageApi::new("vectorcraft", Methods::default(), quick).with_timeout(10)).install().expect("install"); + let out = call("commands", &[]).await.expect("an answered call resolves"); + assert_eq!(to_value(&out), json!({"fine": true})); +} + +#[wasm_bindgen_test] +async fn screenshot_carries_the_png_as_data_whatever_the_app_calls_it() { + reset(); + let reply = json!({"ok": true, "result": {"base64": "UE5H", "width": 2}}); + Rc::new(PageApi::new("vectorcraft", Methods::default(), canned_request(reply))).install().expect("install"); + let out = call("screenshot", &[]).await.expect("screenshot"); + assert_eq!(to_value(&out), json!({"base64": "UE5H", "width": 2, "data": "UE5H"})); +}