Skip to content

Commit

Permalink
web: Check for locked 'Player' mutex and reschedule with setTimeout
Browse files Browse the repository at this point in the history
We use 'wasm-bindgen-futures' as our futures executor on web, which
in turn uses 'queueMicroTask'. This can result in the browser executing
one of our futures while we're still inside our `requestAnimationFrame`
callback (in particular, while we still have the `Player` mutex locked).

We now detect this condition by attempting the lock the Player mutex
inside of our `spawn_local` future. If this fails, we `await`
a `setTimeout`-based promise, which ensures that our code runs in
a new top-level `setTimeout` javascript 'task' (outside of our
`requestAnimationFrame` callback).
  • Loading branch information
Aaron1011 authored and Dinnerbone committed Jul 15, 2024
1 parent c9fedb2 commit 6e93927
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 7 deletions.
4 changes: 3 additions & 1 deletion core/src/backend/navigator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::loader::Error;
use crate::socket::{ConnectionState, SocketAction, SocketHandle};
use crate::string::WStr;
use async_channel::{Receiver, Sender};
use downcast_rs::Downcast;
use encoding_rs::Encoding;
use indexmap::IndexMap;
use std::borrow::Cow;
Expand Down Expand Up @@ -244,7 +245,7 @@ pub struct ErrorResponse {
pub type OwnedFuture<T, E> = Pin<Box<dyn Future<Output = Result<T, E>> + 'static>>;

/// A backend interacting with a browser environment.
pub trait NavigatorBackend {
pub trait NavigatorBackend: Downcast {
/// Cause a browser navigation to a given URL.
///
/// The URL given may be any URL scheme a browser can support. This may not
Expand Down Expand Up @@ -319,6 +320,7 @@ pub trait NavigatorBackend {
sender: Sender<SocketAction>,
);
}
impl_downcast!(NavigatorBackend);

#[cfg(not(target_family = "wasm"))]
pub struct NullExecutor(futures::executor::LocalPool);
Expand Down
2 changes: 1 addition & 1 deletion core/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ pub struct UpdateContext<'a, 'gc> {
pub audio_manager: &'a mut AudioManager<'gc>,

/// The navigator backend, used by the AVM to make HTTP requests and visit webpages.
pub navigator: &'a mut (dyn NavigatorBackend + 'a),
pub navigator: &'a mut dyn NavigatorBackend,

/// The renderer, used by the display objects to draw themselves.
pub renderer: &'a mut dyn RenderBackend,
Expand Down
4 changes: 4 additions & 0 deletions core/src/player.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1941,6 +1941,10 @@ impl Player {
&self.navigator
}

pub fn navigator_mut(&mut self) -> &mut Navigator {
&mut self.navigator
}

// The frame rate of the current movie in FPS.
pub fn frame_rate(&self) -> f64 {
self.frame_rate
Expand Down
4 changes: 3 additions & 1 deletion frontend-utils/src/backends/navigator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ impl<F: FutureSpawner, I: NavigatorInterface> ExternalNavigatorBackend<F, I> {
}
}

impl<F: FutureSpawner, I: NavigatorInterface> NavigatorBackend for ExternalNavigatorBackend<F, I> {
impl<F: FutureSpawner + 'static, I: NavigatorInterface> NavigatorBackend
for ExternalNavigatorBackend<F, I>
{
fn navigate_to_url(
&self,
url: &str,
Expand Down
11 changes: 10 additions & 1 deletion web/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,16 @@ impl RuffleInstanceBuilder {
.with_page_url(window.location().href().ok())
.build();

if let Ok(mut core) = core.try_lock() {
let player_weak = Arc::downgrade(&core);

{
let mut core = core
.lock()
.expect("Failed to lock player after construction");
core.navigator_mut()
.downcast_mut::<WebNavigatorBackend>()
.expect("Expected WebNavigatorBackend")
.set_player(player_weak);
// Set config parameters.
core.set_volume(self.volume);
core.set_background_color(self.background_color);
Expand Down
40 changes: 37 additions & 3 deletions web/src/navigator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use async_channel::{Receiver, Sender};
use futures_util::future::Either;
use futures_util::{future, SinkExt, StreamExt};
use gloo_net::websocket::{futures::WebSocket, Message};
use js_sys::{Array, Uint8Array};
use js_sys::{Array, Promise, Uint8Array};
use ruffle_core::backend::navigator::{
async_return, create_fetch_error, create_specific_fetch_error, get_encoding, ErrorResponse,
NavigationMethod, NavigatorBackend, OpenURLMode, OwnedFuture, Request, SuccessResponse,
Expand All @@ -14,10 +14,11 @@ use ruffle_core::indexmap::IndexMap;
use ruffle_core::loader::Error;
use ruffle_core::socket::{ConnectionState, SocketAction, SocketHandle};
use ruffle_core::swf::Encoding;
use ruffle_core::Player;
use std::borrow::Cow;
use std::cell::RefCell;
use std::rc::Rc;
use std::sync::Arc;
use std::sync::{Arc, Mutex, Weak};
use std::time::Duration;
use tracing_subscriber::layer::Layered;
use tracing_subscriber::Registry;
Expand All @@ -40,6 +41,7 @@ pub struct WebNavigatorBackend {
open_url_mode: OpenURLMode,
socket_proxies: Vec<SocketProxy>,
credential_allow_list: Vec<String>,
player: Weak<Mutex<Player>>,
}

#[allow(clippy::too_many_arguments)]
Expand Down Expand Up @@ -96,8 +98,14 @@ impl WebNavigatorBackend {
open_url_mode,
socket_proxies,
credential_allow_list,
player: Weak::new(),
}
}

/// We need to set the player after construction because the player is created after the navigator.
pub fn set_player(&mut self, player: Weak<Mutex<Player>>) {
self.player = player;
}
}

impl NavigatorBackend for WebNavigatorBackend {
Expand Down Expand Up @@ -356,8 +364,34 @@ impl NavigatorBackend for WebNavigatorBackend {

fn spawn_future(&mut self, future: OwnedFuture<(), Error>) {
let subscriber = self.log_subscriber.clone();
let player = self.player.clone();

spawn_local(async move {
let _subscriber = tracing::subscriber::set_default(subscriber);
let _subscriber = tracing::subscriber::set_default(subscriber.clone());
if player
.upgrade()
.expect("Called spawn_future after player was dropped")
.try_lock()
.is_err()
{
// The player is locked - this can occur due to 'wasm-bindgen-futures' using
// 'queueMicroTask', which may result in one of our future's getting polled
// while we're still inside of our 'requestAnimationFrame' callback (e.g.
// when we call into javascript).
//
// When this happens, we 'reschedule' this future by waiting fot a 'setTimeout'
// callback to be resolved. This will cause our future to get woken up from
// inside the 'setTimeout' JavaScript task (which is a new top-level call stack),
// outside of the 'requestAnimationFrame' callback, which will allow us to lock
// the Player.
let promise = Promise::new(&mut |resolve, _reject| {
web_sys::window()
.expect("window")
.set_timeout_with_callback(&resolve)
.expect("Failed to call setTimeout with dummy promise");
});
let _ = JsFuture::from(promise).await;
}
if let Err(e) = future.await {
tracing::error!("Asynchronous error occurred: {}", e);
}
Expand Down

0 comments on commit 6e93927

Please sign in to comment.