Fix desktop effect race condition (#2313)

* don't poll desktop before the render has been applied

* fix desktop headless tests

* move edit queued code into the edit channel and add more comments

packages/desktop/headless_tests/events.rs

@@ -20,8 +20,9 @@ fn app() -> Element {
     let received = RECEIVED_EVENTS();
     let expected = utils::EXPECTED_EVENTS();
 
-    use_effect(move || {
+    use_memo(move || {
         println!("expecting {} events", utils::EXPECTED_EVENTS());
+        println!("received {} events", RECEIVED_EVENTS());
     });
 
     if expected != 0 && received == expected {

packages/desktop/src/edits.rs

@@ -1,4 +1,5 @@
-use std::{cell::RefCell, collections::VecDeque, rc::Rc};
+use std::cell::Cell;
+use std::{cell::RefCell, collections::VecDeque, rc::Rc, task::Waker};
 
 /// This handles communication between the requests that the webview makes and the interpreter. The interpreter constantly makes long running requests to the webview to get any edits that should be made to the DOM almost like server side events.
 /// It will hold onto the requests until the interpreter is ready to handle them and hold onto any pending edits until a new request is made.
@@ -6,6 +7,11 @@ use std::{cell::RefCell, collections::VecDeque, rc::Rc};
 pub(crate) struct EditQueue {
     queue: Rc<RefCell<VecDeque<Vec<u8>>>>,
     responder: Rc<RefCell<Option<wry::RequestAsyncResponder>>>,
+    // Stores any futures waiting for edits to be applied to the webview
+    // NOTE: We don't use a Notify here because we need polling the notify to be cancel safe
+    waiting_for_edits_flushed: Rc<RefCell<Vec<Waker>>>,
+    // If this webview is currently waiting for an edit to be flushed. We don't run the virtual dom while this is true to avoid running effects before the dom has been updated
+    edits_in_progress: Rc<Cell<bool>>,
 }
 
 impl EditQueue {
@@ -14,16 +20,48 @@ impl EditQueue {
         if let Some(bytes) = queue.pop_back() {
             responder.respond(wry::http::Response::new(bytes));
         } else {
+            // There are now no edits that need to be applied to the webview
+            self.edits_finished();
             *self.responder.borrow_mut() = Some(responder);
         }
     }
 
     pub fn add_edits(&self, edits: Vec<u8>) {
         let mut responder = self.responder.borrow_mut();
+        // There are pending edits that need to be applied to the webview before we run futures
+        self.start_edits();
         if let Some(responder) = responder.take() {
             responder.respond(wry::http::Response::new(edits));
         } else {
             self.queue.borrow_mut().push_front(edits);
         }
     }
+
+    fn start_edits(&self) {
+        self.edits_in_progress.set(true);
+    }
+
+    fn edits_finished(&self) {
+        for waker in self.waiting_for_edits_flushed.borrow_mut().drain(..) {
+            waker.wake();
+        }
+        self.edits_in_progress.set(false);
+    }
+
+    fn edits_in_progress(&self) -> bool {
+        self.edits_in_progress.get()
+    }
+
+    /// Wait until all pending edits have been rendered in the webview
+    pub fn poll_edits_flushed(&self, cx: &mut std::task::Context<'_>) -> std::task::Poll<()> {
+        if self.edits_in_progress() {
+            let waker = cx.waker();
+            self.waiting_for_edits_flushed
+                .borrow_mut()
+                .push(waker.clone());
+            std::task::Poll::Pending
+        } else {
+            std::task::Poll::Ready(())
+        }
+    }
 }

packages/desktop/src/webview.rs

@@ -229,10 +229,16 @@ impl WebviewInstance {
     pub fn poll_vdom(&mut self) {
         let mut cx = std::task::Context::from_waker(&self.waker);
 
-        // Continously poll the virtualdom until it's pending
+        // Continuously poll the virtualdom until it's pending
         // Wait for work will return Ready when it has edits to be sent to the webview
         // It will return Pending when it needs to be polled again - nothing is ready
         loop {
+            // If we're waiting for a render, wait for it to finish before we continue
+            let edits_flushed_poll = self.desktop_context.edit_queue.poll_edits_flushed(&mut cx);
+            if edits_flushed_poll.is_pending() {
+                return;
+            }
+
             {
                 let fut = self.dom.wait_for_work();
                 pin_mut!(fut);

packages/interpreter/src/js/hash.txt

@@ -1 +1 @@
-2770005544568683192
+5713307201725207733

packages/interpreter/src/ts/native.ts

@@ -15,7 +15,7 @@ var JSChannel_: typeof BaseInterpreter;
 if (RawInterpreter !== undefined && RawInterpreter !== null) {
   // @ts-ignore - this is coming from the host
   JSChannel_ = RawInterpreter;
-};
+}
 
 export class NativeInterpreter extends JSChannel_ {
   intercept_link_redirects: boolean;
@@ -40,28 +40,39 @@ export class NativeInterpreter extends JSChannel_ {
 
     // attach an event listener on the body that prevents file drops from navigating
     // this is because the browser will try to navigate to the file if it's dropped on the window
-    window.addEventListener("dragover", function (e) {
-      // // check which element is our target
-      if (e.target instanceof Element && e.target.tagName != "INPUT") {
+    window.addEventListener(
+      "dragover",
+      function (e) {
+        // // check which element is our target
+        if (e.target instanceof Element && e.target.tagName != "INPUT") {
+          e.preventDefault();
+        }
+      },
+      false
+    );
+
+    window.addEventListener(
+      "drop",
+      function (e) {
+        let target = e.target;
+
+        if (!(target instanceof Element)) {
+          return;
+        }
+
+        // Dropping a file on the window will navigate to the file, which we don't want
         e.preventDefault();
-      }
-    }, false);
-
-    window.addEventListener("drop", function (e) {
-      let target = e.target;
-
-      if (!(target instanceof Element)) {
-        return;
-      }
-
-      // Dropping a file on the window will navigate to the file, which we don't want
-      e.preventDefault();
-    }, false);
+      },
+      false
+    );
 
     // attach a listener to the route that listens for clicks and prevents the default file dialog
     window.addEventListener("click", (event) => {
       const target = event.target;
-      if (target instanceof HTMLInputElement && target.getAttribute("type") === "file") {
+      if (
+        target instanceof HTMLInputElement &&
+        target.getAttribute("type") === "file"
+      ) {
         // Send a message to the host to open the file dialog if the target is a file input and has a dioxus id attached to it
         let target_id = getTargetId(target);
         if (target_id !== null) {
@@ -79,12 +90,12 @@ export class NativeInterpreter extends JSChannel_ {
       }
     });
 
-
     // @ts-ignore - wry gives us this
     this.ipc = window.ipc;
 
     // make sure we pass the handler to the base interpreter
-    const handler: EventListener = (event) => this.handleEvent(event, event.type, true);
+    const handler: EventListener = (event) =>
+      this.handleEvent(event, event.type, true);
     super.initialize(root, handler);
   }
 
@@ -99,7 +110,9 @@ export class NativeInterpreter extends JSChannel_ {
     }
   }
 
-  getClientRect(id: NodeId): { type: string; origin: number[]; size: number[]; } | undefined {
+  getClientRect(
+    id: NodeId
+  ): { type: string; origin: number[]; size: number[] } | undefined {
     const node = this.nodes[id];
     if (node instanceof HTMLElement) {
       const rect = node.getBoundingClientRect();
@@ -171,13 +184,15 @@ export class NativeInterpreter extends JSChannel_ {
     // liveview does not have syncronous event handling, so we need to send the event to the host
     if (this.liveview) {
       // Okay, so the user might've requested some files to be read
-      if (target instanceof HTMLInputElement && (event.type === "change" || event.type === "input")) {
+      if (
+        target instanceof HTMLInputElement &&
+        (event.type === "change" || event.type === "input")
+      ) {
         if (target.getAttribute("type") === "file") {
           this.readFiles(target, contents, bubbles, realId, name);
         }
       }
     } else {
-
       const message = this.serializeIpcMessage("user_event", body);
       this.ipc.postMessage(message);
 
@@ -195,8 +210,6 @@ export class NativeInterpreter extends JSChannel_ {
     }
   }
 
-
-
   // This should:
   // - prevent form submissions from navigating
   // - prevent anchor tags from navigating
@@ -210,7 +223,10 @@ export class NativeInterpreter extends JSChannel_ {
       preventDefaultRequests = target.getAttribute(`dioxus-prevent-default`);
     }
 
-    if (preventDefaultRequests && preventDefaultRequests.includes(`on${event.type}`)) {
+    if (
+      preventDefaultRequests &&
+      preventDefaultRequests.includes(`on${event.type}`)
+    ) {
       event.preventDefault();
     }
 
@@ -224,7 +240,11 @@ export class NativeInterpreter extends JSChannel_ {
     }
   }
 
-  handleClickNavigate(event: Event, target: Element, preventDefaultRequests: string) {
+  handleClickNavigate(
+    event: Event,
+    target: Element,
+    preventDefaultRequests: string
+  ) {
     // todo call prevent default if it's the right type of event
     if (!this.intercept_link_redirects) {
       return;
@@ -279,24 +299,30 @@ export class NativeInterpreter extends JSChannel_ {
     }
   }
 
-  waitForRequest(headless: boolean) {
-    fetch(new Request(this.editsPath))
-      .then(response => response.arrayBuffer())
-      .then(bytes => {
-        // In headless mode, the requestAnimationFrame callback is never called, so we need to run the bytes directly
-        if (headless) {
-          // @ts-ignore
-          this.run_from_bytes(bytes);
-        } else {
-          this.enqueueBytes(bytes);
-          requestAnimationFrame(() => {
-            this.flushQueuedBytes();
-          });
-        }
+  // Run the edits the next animation frame
+  rafEdits(headless: boolean, bytes: ArrayBuffer) {
+    // In headless mode, the requestAnimationFrame callback is never called, so we need to run the bytes directly
+    if (headless) {
+      // @ts-ignore
+      this.run_from_bytes(bytes);
+      this.waitForRequest(headless);
+    } else {
+      this.enqueueBytes(bytes);
+      requestAnimationFrame(() => {
+        this.flushQueuedBytes();
+        // With request animation frames, we use the next reqwest as a marker to know when the frame is done and it is safe to run effects
         this.waitForRequest(headless);
       });
+    }
   }
 
+  waitForRequest(headless: boolean) {
+    fetch(new Request(this.editsPath))
+      .then((response) => response.arrayBuffer())
+      .then((bytes) => {
+        this.rafEdits(headless, bytes);
+      });
+  }
 
   kickAllStylesheetsOnPage() {
     // If this function is being called and we have not explicitly set kickStylesheets to true, then we should
@@ -314,7 +340,13 @@ export class NativeInterpreter extends JSChannel_ {
 
   //  A liveview only function
   // Desktop will intercept the event before it hits this
-  async readFiles(target: HTMLInputElement, contents: SerializedEvent, bubbles: boolean, realId: NodeId, name: string) {
+  async readFiles(
+    target: HTMLInputElement,
+    contents: SerializedEvent,
+    bubbles: boolean,
+    realId: NodeId,
+    name: string
+  ) {
     let files = target.files!;
     let file_contents: { [name: string]: number[] } = {};
 
@@ -388,7 +420,6 @@ function getTargetId(target: EventTarget): NodeId | null {
   return parseInt(realId);
 }
 
-
 // function applyFileUpload() {
 //   let inputs = document.querySelectorAll("input");
 //   for (let input of inputs) {