From d131b2b5801827f1324a7b2a331e66691acf5b29 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Proch=C3=A1zka?= <honza.spacir@gmail.com>
Date: Mon, 27 May 2024 21:55:23 +0200
Subject: [PATCH] Fix: Don't `.forget()` RAF closure (#4551)

The closure is now stored in `WebRunner` and dropped in the next
`request_animation_frame` instead of being "leaked" via `forget`

---------

Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
---
 crates/eframe/src/web/web_runner.rs | 46 ++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/crates/eframe/src/web/web_runner.rs b/crates/eframe/src/web/web_runner.rs
index 35bb4be334b..6a230a8e329 100644
--- a/crates/eframe/src/web/web_runner.rs
+++ b/crates/eframe/src/web/web_runner.rs
@@ -1,7 +1,4 @@
-use std::{
-    cell::{Cell, RefCell},
-    rc::Rc,
-};
+use std::{cell::RefCell, rc::Rc};
 
 use wasm_bindgen::prelude::*;
 
@@ -29,7 +26,7 @@ pub struct WebRunner {
     events_to_unsubscribe: Rc<RefCell<Vec<EventToUnsubscribe>>>,
 
     /// Current animation frame in flight.
-    request_animation_frame_id: Cell<Option<i32>>,
+    frame: Rc<RefCell<Option<AnimationFrameRequest>>>,
 
     resize_observer: Rc<RefCell<Option<ResizeObserverContext>>>,
 }
@@ -49,7 +46,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),
+            frame: Default::default(),
             resize_observer: Default::default(),
         }
     }
@@ -125,9 +122,9 @@ impl WebRunner {
     pub fn destroy(&self) {
         self.unsubscribe_from_all_events();
 
-        if let Some(id) = self.request_animation_frame_id.get() {
+        if let Some(frame) = self.frame.take() {
             let window = web_sys::window().unwrap();
-            window.cancel_animation_frame(id).ok();
+            window.cancel_animation_frame(frame.id).ok();
         }
 
         if let Some(runner) = self.runner.replace(None) {
@@ -202,16 +199,33 @@ impl WebRunner {
         Ok(())
     }
 
+    /// Request an animation frame from the browser in which we can perform a paint.
+    ///
+    /// It is safe to call `request_animation_frame` multiple times in quick succession,
+    /// this function guarantees that only one animation frame is scheduled at a time.
     pub(crate) fn request_animation_frame(&self) -> Result<(), wasm_bindgen::JsValue> {
+        if self.frame.borrow().is_some() {
+            // there is already an animation frame in flight
+            return Ok(());
+        }
+
         let window = web_sys::window().unwrap();
         let closure = Closure::once({
             let runner_ref = self.clone();
-            move || events::paint_and_schedule(&runner_ref)
+            move || {
+                // We can paint now, so clear the animation frame.
+                // This drops the `closure` and allows another
+                // animation frame to be scheduled
+                let _ = runner_ref.frame.take();
+                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
+        self.frame.borrow_mut().replace(AnimationFrameRequest {
+            id,
+            _closure: closure,
+        });
 
         Ok(())
     }
@@ -232,6 +246,16 @@ impl WebRunner {
 
 // ----------------------------------------------------------------------------
 
+// https://rustwasm.github.io/wasm-bindgen/api/wasm_bindgen/closure/struct.Closure.html#using-fnonce-and-closureonce-with-requestanimationframe
+struct AnimationFrameRequest {
+    /// Represents the ID of a frame in flight.
+    id: i32,
+
+    /// The callback given to `request_animation_frame`, stored here both to prevent it
+    /// from being canceled, and from having to `.forget()` it.
+    _closure: Closure<dyn FnMut() -> Result<(), JsValue>>,
+}
+
 struct ResizeObserverContext {
     resize_observer: web_sys::ResizeObserver,
     closure: Closure<dyn FnMut(js_sys::Array)>,