Fix crash on `request_animation_frame` when destroying web runner (#4169)
Previously, any frames in flight (`requestAnimationFrame`) on web were not being cancelled (`cancelAnimationFrame`) when `WebRunner::destroy` was called. If a user called `destroy`, then immediately removed the canvas from the DOM, eframe could panic with a "failed to find (canvas) element by id" error message. This PR changes two things: - The canvas element is directly referenced everywhere it's needed instead of being looked up by `canvas_id`[^1] - The RAF handle is stored in `WebRunner` and `cancelAnimationFrame` is called on it inside of `WebRunner::destroy`[^2] [^1]: The WebGL/WGPU backends were already holding onto the canvas (and associated GPU context), so the change is just converting all the `get_element_by_id` lookups to retrieve the canvas from the web runner handle. [^2]: There is only ever one frame in flight, so we store it directly as a scalar field.
This commit is contained in:
Jan Procházka
GitHub
parent
00e8ce6d7e
commit
c5eaba43cd
|
|
@ -165,8 +165,8 @@ impl AppRunner {
|
|||
self.last_save_time = now_sec();
|
||||
}
|
||||
|
||||
pub fn canvas_id(&self) -> &str {
|
||||
self.painter.canvas_id()
|
||||
pub fn canvas(&self) -> &web_sys::HtmlCanvasElement {
|
||||
self.painter.canvas()
|
||||
}
|
||||
|
||||
pub fn destroy(mut self) {
|
||||
|
|
@ -182,8 +182,8 @@ impl AppRunner {
|
|||
///
|
||||
/// The result can be painted later with a call to [`Self::run_and_paint`] or [`Self::paint`].
|
||||
pub fn logic(&mut self) {
|
||||
super::resize_canvas_to_screen_size(self.canvas_id(), self.web_options.max_size_points);
|
||||
let canvas_size = super::canvas_size_in_points(self.canvas_id());
|
||||
super::resize_canvas_to_screen_size(self.canvas(), self.web_options.max_size_points);
|
||||
let canvas_size = super::canvas_size_in_points(self.canvas());
|
||||
let raw_input = self.input.new_frame(canvas_size);
|
||||
|
||||
let full_output = self.egui_ctx.run(raw_input, |egui_ctx| {
|
||||
|
|
@ -268,7 +268,7 @@ impl AppRunner {
|
|||
self.mutable_text_under_cursor = mutable_text_under_cursor;
|
||||
|
||||
if self.ime != ime {
|
||||
super::text_agent::move_text_cursor(ime, self.canvas_id());
|
||||
super::text_agent::move_text_cursor(ime, self.canvas());
|
||||
self.ime = ime;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -5,12 +5,12 @@ use super::*;
|
|||
/// Calls `request_animation_frame` to schedule repaint.
|
||||
///
|
||||
/// It will only paint if needed, but will always call `request_animation_frame` immediately.
|
||||
fn paint_and_schedule(runner_ref: &WebRunner) -> Result<(), JsValue> {
|
||||
pub(crate) fn paint_and_schedule(runner_ref: &WebRunner) -> Result<(), JsValue> {
|
||||
// Only paint and schedule if there has been no panic
|
||||
if let Some(mut runner_lock) = runner_ref.try_lock() {
|
||||
paint_if_needed(&mut runner_lock);
|
||||
drop(runner_lock);
|
||||
request_animation_frame(runner_ref.clone())?;
|
||||
runner_ref.request_animation_frame()?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -45,14 +45,6 @@ fn paint_if_needed(runner: &mut AppRunner) {
|
|||
runner.auto_save_if_needed();
|
||||
}
|
||||
|
||||
pub(crate) fn request_animation_frame(runner_ref: WebRunner) -> Result<(), JsValue> {
|
||||
let window = web_sys::window().unwrap();
|
||||
let closure = Closure::once(move || paint_and_schedule(&runner_ref));
|
||||
window.request_animation_frame(closure.as_ref().unchecked_ref())?;
|
||||
closure.forget(); // We must forget it, or else the callback is canceled on drop
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------------
|
||||
|
||||
pub(crate) fn install_document_events(runner_ref: &WebRunner) -> Result<(), JsValue> {
|
||||
|
|
@ -275,7 +267,7 @@ pub(crate) fn install_color_scheme_change_event(runner_ref: &WebRunner) -> Resul
|
|||
}
|
||||
|
||||
pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValue> {
|
||||
let canvas = canvas_element(runner_ref.try_lock().unwrap().canvas_id()).unwrap();
|
||||
let canvas = runner_ref.try_lock().unwrap().canvas().clone();
|
||||
|
||||
{
|
||||
let prevent_default_events = [
|
||||
|
|
@ -304,7 +296,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|
|||
let modifiers = modifiers_from_mouse_event(&event);
|
||||
runner.input.raw.modifiers = modifiers;
|
||||
if let Some(button) = button_from_mouse_event(&event) {
|
||||
let pos = pos_from_mouse_event(runner.canvas_id(), &event);
|
||||
let pos = pos_from_mouse_event(runner.canvas(), &event);
|
||||
let modifiers = runner.input.raw.modifiers;
|
||||
runner.input.raw.events.push(egui::Event::PointerButton {
|
||||
pos,
|
||||
|
|
@ -331,7 +323,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|
|||
|event: web_sys::MouseEvent, runner| {
|
||||
let modifiers = modifiers_from_mouse_event(&event);
|
||||
runner.input.raw.modifiers = modifiers;
|
||||
let pos = pos_from_mouse_event(runner.canvas_id(), &event);
|
||||
let pos = pos_from_mouse_event(runner.canvas(), &event);
|
||||
runner.input.raw.events.push(egui::Event::PointerMoved(pos));
|
||||
runner.needs_repaint.repaint_asap();
|
||||
event.stop_propagation();
|
||||
|
|
@ -343,7 +335,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|
|||
let modifiers = modifiers_from_mouse_event(&event);
|
||||
runner.input.raw.modifiers = modifiers;
|
||||
if let Some(button) = button_from_mouse_event(&event) {
|
||||
let pos = pos_from_mouse_event(runner.canvas_id(), &event);
|
||||
let pos = pos_from_mouse_event(runner.canvas(), &event);
|
||||
let modifiers = runner.input.raw.modifiers;
|
||||
runner.input.raw.events.push(egui::Event::PointerButton {
|
||||
pos,
|
||||
|
|
@ -381,7 +373,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|
|||
"touchstart",
|
||||
|event: web_sys::TouchEvent, runner| {
|
||||
let mut latest_touch_pos_id = runner.input.latest_touch_pos_id;
|
||||
let pos = pos_from_touch_event(runner.canvas_id(), &event, &mut latest_touch_pos_id);
|
||||
let pos = pos_from_touch_event(runner.canvas(), &event, &mut latest_touch_pos_id);
|
||||
runner.input.latest_touch_pos_id = latest_touch_pos_id;
|
||||
runner.input.latest_touch_pos = Some(pos);
|
||||
let modifiers = runner.input.raw.modifiers;
|
||||
|
|
@ -404,7 +396,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|
|||
"touchmove",
|
||||
|event: web_sys::TouchEvent, runner| {
|
||||
let mut latest_touch_pos_id = runner.input.latest_touch_pos_id;
|
||||
let pos = pos_from_touch_event(runner.canvas_id(), &event, &mut latest_touch_pos_id);
|
||||
let pos = pos_from_touch_event(runner.canvas(), &event, &mut latest_touch_pos_id);
|
||||
runner.input.latest_touch_pos_id = latest_touch_pos_id;
|
||||
runner.input.latest_touch_pos = Some(pos);
|
||||
runner.input.raw.events.push(egui::Event::PointerMoved(pos));
|
||||
|
|
@ -467,7 +459,7 @@ pub(crate) fn install_canvas_events(runner_ref: &WebRunner) -> Result<(), JsValu
|
|||
});
|
||||
|
||||
let scroll_multiplier = match unit {
|
||||
egui::MouseWheelUnit::Page => canvas_size_in_points(runner.canvas_id()).y,
|
||||
egui::MouseWheelUnit::Page => canvas_size_in_points(runner.canvas()).y,
|
||||
egui::MouseWheelUnit::Line => {
|
||||
#[allow(clippy::let_and_return)]
|
||||
let points_per_scroll_line = 8.0; // Note that this is intentionally different from what we use in winit.
|
||||
|
|
|
|||
|
|
@ -1,7 +1,9 @@
|
|||
use super::{canvas_element, canvas_origin, AppRunner};
|
||||
use super::{canvas_origin, AppRunner};
|
||||
|
||||
pub fn pos_from_mouse_event(canvas_id: &str, event: &web_sys::MouseEvent) -> egui::Pos2 {
|
||||
let canvas = canvas_element(canvas_id).unwrap();
|
||||
pub fn pos_from_mouse_event(
|
||||
canvas: &web_sys::HtmlCanvasElement,
|
||||
event: &web_sys::MouseEvent,
|
||||
) -> egui::Pos2 {
|
||||
let rect = canvas.get_bounding_client_rect();
|
||||
egui::Pos2 {
|
||||
x: event.client_x() as f32 - rect.left() as f32,
|
||||
|
|
@ -27,7 +29,7 @@ pub fn button_from_mouse_event(event: &web_sys::MouseEvent) -> Option<egui::Poin
|
|||
/// `touch_id_for_pos` is the [`TouchId`](egui::TouchId) of the [`Touch`](web_sys::Touch) we previously used to determine the
|
||||
/// pointer position.
|
||||
pub fn pos_from_touch_event(
|
||||
canvas_id: &str,
|
||||
canvas: &web_sys::HtmlCanvasElement,
|
||||
event: &web_sys::TouchEvent,
|
||||
touch_id_for_pos: &mut Option<egui::TouchId>,
|
||||
) -> egui::Pos2 {
|
||||
|
|
@ -47,7 +49,7 @@ pub fn pos_from_touch_event(
|
|||
.or_else(|| event.touches().get(0))
|
||||
.map_or(Default::default(), |touch| {
|
||||
*touch_id_for_pos = Some(egui::TouchId::from(touch.identifier()));
|
||||
pos_from_touch(canvas_origin(canvas_id), &touch)
|
||||
pos_from_touch(canvas_origin(canvas), &touch)
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -59,7 +61,7 @@ fn pos_from_touch(canvas_origin: egui::Pos2, touch: &web_sys::Touch) -> egui::Po
|
|||
}
|
||||
|
||||
pub fn push_touches(runner: &mut AppRunner, phase: egui::TouchPhase, event: &web_sys::TouchEvent) {
|
||||
let canvas_origin = canvas_origin(runner.canvas_id());
|
||||
let canvas_origin = canvas_origin(runner.canvas());
|
||||
for touch_idx in 0..event.changed_touches().length() {
|
||||
if let Some(touch) = event.changed_touches().item(touch_idx) {
|
||||
runner.input.raw.events.push(egui::Event::Touch {
|
||||
|
|
|
|||
|
|
@ -100,26 +100,23 @@ fn theme_from_dark_mode(dark_mode: bool) -> Theme {
|
|||
}
|
||||
}
|
||||
|
||||
fn canvas_element(canvas_id: &str) -> Option<web_sys::HtmlCanvasElement> {
|
||||
fn get_canvas_element_by_id(canvas_id: &str) -> Option<web_sys::HtmlCanvasElement> {
|
||||
let document = web_sys::window()?.document()?;
|
||||
let canvas = document.get_element_by_id(canvas_id)?;
|
||||
canvas.dyn_into::<web_sys::HtmlCanvasElement>().ok()
|
||||
}
|
||||
|
||||
fn canvas_element_or_die(canvas_id: &str) -> web_sys::HtmlCanvasElement {
|
||||
canvas_element(canvas_id)
|
||||
fn get_canvas_element_by_id_or_die(canvas_id: &str) -> web_sys::HtmlCanvasElement {
|
||||
get_canvas_element_by_id(canvas_id)
|
||||
.unwrap_or_else(|| panic!("Failed to find canvas with id {canvas_id:?}"))
|
||||
}
|
||||
|
||||
fn canvas_origin(canvas_id: &str) -> egui::Pos2 {
|
||||
let rect = canvas_element(canvas_id)
|
||||
.unwrap()
|
||||
.get_bounding_client_rect();
|
||||
fn canvas_origin(canvas: &web_sys::HtmlCanvasElement) -> egui::Pos2 {
|
||||
let rect = canvas.get_bounding_client_rect();
|
||||
egui::pos2(rect.left() as f32, rect.top() as f32)
|
||||
}
|
||||
|
||||
fn canvas_size_in_points(canvas_id: &str) -> egui::Vec2 {
|
||||
let canvas = canvas_element(canvas_id).unwrap();
|
||||
fn canvas_size_in_points(canvas: &web_sys::HtmlCanvasElement) -> egui::Vec2 {
|
||||
let pixels_per_point = native_pixels_per_point();
|
||||
egui::vec2(
|
||||
canvas.width() as f32 / pixels_per_point,
|
||||
|
|
@ -127,8 +124,10 @@ fn canvas_size_in_points(canvas_id: &str) -> egui::Vec2 {
|
|||
)
|
||||
}
|
||||
|
||||
fn resize_canvas_to_screen_size(canvas_id: &str, max_size_points: egui::Vec2) -> Option<()> {
|
||||
let canvas = canvas_element(canvas_id)?;
|
||||
fn resize_canvas_to_screen_size(
|
||||
canvas: &web_sys::HtmlCanvasElement,
|
||||
max_size_points: egui::Vec2,
|
||||
) -> Option<()> {
|
||||
let parent = canvas.parent_element()?;
|
||||
|
||||
// Prefer the client width and height so that if the parent
|
||||
|
|
|
|||
|
|
@ -5,7 +5,7 @@ use std::{cell::Cell, rc::Rc};
|
|||
|
||||
use wasm_bindgen::prelude::*;
|
||||
|
||||
use super::{canvas_element, AppRunner, WebRunner};
|
||||
use super::{AppRunner, WebRunner};
|
||||
|
||||
static AGENT_ID: &str = "egui_text_agent";
|
||||
|
||||
|
|
@ -121,7 +121,7 @@ pub fn update_text_agent(runner: &mut AppRunner) -> Option<()> {
|
|||
let window = web_sys::window()?;
|
||||
let document = window.document()?;
|
||||
let input: HtmlInputElement = document.get_element_by_id(AGENT_ID)?.dyn_into().unwrap();
|
||||
let canvas_style = canvas_element(runner.canvas_id())?.style();
|
||||
let canvas_style = runner.canvas().style();
|
||||
|
||||
if runner.mutable_text_under_cursor {
|
||||
let is_already_editing = input.hidden();
|
||||
|
|
@ -205,14 +205,16 @@ fn is_mobile() -> Option<bool> {
|
|||
// candidate window moves following text element (agent),
|
||||
// so it appears that the IME candidate window moves with text cursor.
|
||||
// On mobile devices, there is no need to do that.
|
||||
pub fn move_text_cursor(ime: Option<egui::output::IMEOutput>, canvas_id: &str) -> Option<()> {
|
||||
pub fn move_text_cursor(
|
||||
ime: Option<egui::output::IMEOutput>,
|
||||
canvas: &web_sys::HtmlCanvasElement,
|
||||
) -> Option<()> {
|
||||
let style = text_agent().style();
|
||||
// Note: moving agent on mobile devices will lead to unpredictable scroll.
|
||||
if is_mobile() == Some(false) {
|
||||
ime.as_ref().and_then(|ime| {
|
||||
let egui::Pos2 { x, y } = ime.cursor_rect.left_top();
|
||||
|
||||
let canvas = canvas_element(canvas_id)?;
|
||||
let bounding_rect = text_agent().get_bounding_client_rect();
|
||||
let y = (y + (canvas.scroll_top() + canvas.offset_top()) as f32)
|
||||
.min(canvas.client_height() as f32 - bounding_rect.height() as f32);
|
||||
|
|
|
|||
|
|
@ -9,8 +9,8 @@ pub(crate) trait WebPainter {
|
|||
// where
|
||||
// Self: Sized;
|
||||
|
||||
/// Id of the canvas in use.
|
||||
fn canvas_id(&self) -> &str;
|
||||
/// Reference to the canvas in use.
|
||||
fn canvas(&self) -> &web_sys::HtmlCanvasElement;
|
||||
|
||||
/// Maximum size of a texture in one direction.
|
||||
fn max_texture_side(&self) -> usize;
|
||||
|
|
|
|||
|
|
@ -10,7 +10,6 @@ use super::web_painter::WebPainter;
|
|||
|
||||
pub(crate) struct WebPainterGlow {
|
||||
canvas: HtmlCanvasElement,
|
||||
canvas_id: String,
|
||||
painter: egui_glow::Painter,
|
||||
}
|
||||
|
||||
|
|
@ -20,7 +19,7 @@ impl WebPainterGlow {
|
|||
}
|
||||
|
||||
pub async fn new(canvas_id: &str, options: &WebOptions) -> Result<Self, String> {
|
||||
let canvas = super::canvas_element_or_die(canvas_id);
|
||||
let canvas = super::get_canvas_element_by_id_or_die(canvas_id);
|
||||
|
||||
let (gl, shader_prefix) =
|
||||
init_glow_context_from_canvas(&canvas, options.webgl_context_option)?;
|
||||
|
|
@ -30,11 +29,7 @@ impl WebPainterGlow {
|
|||
let painter = egui_glow::Painter::new(gl, shader_prefix, None)
|
||||
.map_err(|err| format!("Error starting glow painter: {err}"))?;
|
||||
|
||||
Ok(Self {
|
||||
canvas,
|
||||
canvas_id: canvas_id.to_owned(),
|
||||
painter,
|
||||
})
|
||||
Ok(Self { canvas, painter })
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -43,8 +38,8 @@ impl WebPainter for WebPainterGlow {
|
|||
self.painter.max_texture_side()
|
||||
}
|
||||
|
||||
fn canvas_id(&self) -> &str {
|
||||
&self.canvas_id
|
||||
fn canvas(&self) -> &HtmlCanvasElement {
|
||||
&self.canvas
|
||||
}
|
||||
|
||||
fn paint_and_update_textures(
|
||||
|
|
|
|||
|
|
@ -41,7 +41,6 @@ impl HasDisplayHandle for EguiWebWindow {
|
|||
|
||||
pub(crate) struct WebPainterWgpu {
|
||||
canvas: HtmlCanvasElement,
|
||||
canvas_id: String,
|
||||
surface: wgpu::Surface<'static>,
|
||||
surface_configuration: wgpu::SurfaceConfiguration,
|
||||
render_state: Option<RenderState>,
|
||||
|
|
@ -163,7 +162,7 @@ impl WebPainterWgpu {
|
|||
}
|
||||
}
|
||||
|
||||
let canvas = super::canvas_element_or_die(canvas_id);
|
||||
let canvas = super::get_canvas_element_by_id_or_die(canvas_id);
|
||||
let surface = instance
|
||||
.create_surface(wgpu::SurfaceTarget::Canvas(canvas.clone()))
|
||||
.map_err(|err| format!("failed to create wgpu surface: {err}"))?;
|
||||
|
|
@ -188,7 +187,6 @@ impl WebPainterWgpu {
|
|||
|
||||
Ok(Self {
|
||||
canvas,
|
||||
canvas_id: canvas_id.to_owned(),
|
||||
render_state: Some(render_state),
|
||||
surface,
|
||||
surface_configuration,
|
||||
|
|
@ -200,8 +198,8 @@ impl WebPainterWgpu {
|
|||
}
|
||||
|
||||
impl WebPainter for WebPainterWgpu {
|
||||
fn canvas_id(&self) -> &str {
|
||||
&self.canvas_id
|
||||
fn canvas(&self) -> &HtmlCanvasElement {
|
||||
&self.canvas
|
||||
}
|
||||
|
||||
fn max_texture_side(&self) -> usize {
|
||||
|
|
|
|||
|
|
@ -1,4 +1,7 @@
|
|||
use std::{cell::RefCell, rc::Rc};
|
||||
use std::{
|
||||
cell::{Cell, RefCell},
|
||||
rc::Rc,
|
||||
};
|
||||
|
||||
use wasm_bindgen::prelude::*;
|
||||
|
||||
|
|
@ -24,6 +27,9 @@ pub struct WebRunner {
|
|||
/// They have to be in a separate `Rc` so that we don't need to pass them to
|
||||
/// the panic handler, since they aren't `Send`.
|
||||
events_to_unsubscribe: Rc<RefCell<Vec<EventToUnsubscribe>>>,
|
||||
|
||||
/// Used in `destroy` to cancel a pending frame.
|
||||
request_animation_frame_id: Cell<Option<i32>>,
|
||||
}
|
||||
|
||||
impl WebRunner {
|
||||
|
|
@ -41,6 +47,7 @@ impl WebRunner {
|
|||
panic_handler,
|
||||
runner: Rc::new(RefCell::new(None)),
|
||||
events_to_unsubscribe: Rc::new(RefCell::new(Default::default())),
|
||||
request_animation_frame_id: Cell::new(None),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -71,7 +78,7 @@ impl WebRunner {
|
|||
events::install_color_scheme_change_event(self)?;
|
||||
}
|
||||
|
||||
events::request_animation_frame(self.clone())?;
|
||||
self.request_animation_frame()?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
|
|
@ -108,6 +115,11 @@ impl WebRunner {
|
|||
pub fn destroy(&self) {
|
||||
self.unsubscribe_from_all_events();
|
||||
|
||||
if let Some(id) = self.request_animation_frame_id.get() {
|
||||
let window = web_sys::window().unwrap();
|
||||
window.cancel_animation_frame(id).ok();
|
||||
}
|
||||
|
||||
if let Some(runner) = self.runner.replace(None) {
|
||||
runner.destroy();
|
||||
}
|
||||
|
|
@ -179,6 +191,18 @@ impl WebRunner {
|
|||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) fn request_animation_frame(&self) -> Result<(), wasm_bindgen::JsValue> {
|
||||
let window = web_sys::window().unwrap();
|
||||
let closure = Closure::once({
|
||||
let runner_ref = self.clone();
|
||||
move || events::paint_and_schedule(&runner_ref)
|
||||
});
|
||||
let id = window.request_animation_frame(closure.as_ref().unchecked_ref())?;
|
||||
self.request_animation_frame_id.set(Some(id));
|
||||
closure.forget(); // We must forget it, or else the callback is canceled on drop
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue