From 12d28dffc6c8d08e6620751e55d8a382c66443a2 Mon Sep 17 00:00:00 2001 From: Andreu Botella Date: Tue, 22 Mar 2022 11:33:29 +0100 Subject: fix(fetch): Fix uncaught rejection panic with `WebAssembly.instantiateStreaming` (#13925) When an exception is thrown during the processing of streaming WebAssembly, `op_wasm_streaming_abort` is called. This op calls into V8, which synchronously rejects the promise and calls into the promise rejection handler, if applicable. But calling an op borrows the isolate's `JsRuntimeState` for the duration of the op, which means it is borrowed when V8 calls into `promise_reject_callback`, which tries to borrow it again, panicking. This change changes `op_wasm_streaming_abort` from an op to a binding (`Deno.core.abortWasmStreaming`). Although that binding must borrow the `JsRuntimeState` in order to access the `WasmStreamingResource` stored in the `OpTable`, it also takes ownership of that `WasmStreamingResource` instance, which means it can drop any borrows of the `JsRuntimeState` before calling into V8. --- core/bindings.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) (limited to 'core/bindings.rs') diff --git a/core/bindings.rs b/core/bindings.rs index 6e85a57d4..91087799a 100644 --- a/core/bindings.rs +++ b/core/bindings.rs @@ -7,10 +7,12 @@ use crate::modules::parse_import_assertions; use crate::modules::validate_import_assertions; use crate::modules::ImportAssertionsKind; use crate::modules::ModuleMap; +use crate::ops_builtin::WasmStreamingResource; use crate::resolve_url_or_path; use crate::JsRuntime; use crate::OpState; use crate::PromiseId; +use crate::ResourceId; use crate::ZeroCopyBuf; use anyhow::Error; use log::debug; @@ -99,6 +101,9 @@ pub static EXTERNAL_REFERENCES: Lazy = v8::ExternalReference { function: set_wasm_streaming_callback.map_fn_to(), }, + v8::ExternalReference { + function: abort_wasm_streaming.map_fn_to(), + }, ]) }); @@ -226,6 +231,7 @@ pub fn initialize_context<'s>( "setWasmStreamingCallback", set_wasm_streaming_callback, ); + set_func(scope, core_val, "abortWasmStreaming", abort_wasm_streaming); // Direct bindings on `window`. set_func(scope, global, "queueMicrotask", queue_microtask); @@ -762,8 +768,6 @@ fn set_wasm_streaming_callback( args: v8::FunctionCallbackArguments, _rv: v8::ReturnValue, ) { - use crate::ops_builtin::WasmStreamingResource; - let cb = match arg0_to_cb(scope, args) { Ok(cb) => cb, Err(()) => return, @@ -805,6 +809,48 @@ fn set_wasm_streaming_callback( }); } +fn abort_wasm_streaming( + scope: &mut v8::HandleScope, + args: v8::FunctionCallbackArguments, + _rv: v8::ReturnValue, +) { + let rid: ResourceId = match serde_v8::from_v8(scope, args.get(0)) { + Ok(rid) => rid, + Err(_) => return throw_type_error(scope, "Invalid argument"), + }; + let exception = args.get(1); + + let wasm_streaming = { + let state_rc = JsRuntime::state(scope); + let state = state_rc.borrow(); + let wsr = state + .op_state + .borrow_mut() + .resource_table + .take::(rid); + match wsr { + Ok(wasm_streaming) => wasm_streaming, + Err(err) => { + let message = v8::String::new(scope, &err.to_string()).unwrap(); + let v8_error = v8::Exception::error(scope, message); + scope.throw_exception(v8_error); + return; + } + } + }; + + // At this point there are no clones of Rc on the + // resource table, and no one should own a reference because we're never + // cloning them. So we can be sure `wasm_streaming` is the only reference. + if let Ok(wsr) = std::rc::Rc::try_unwrap(wasm_streaming) { + // NOTE: v8::WasmStreaming::abort can't be called while `state` is borrowed; + // see https://github.com/denoland/deno/issues/13917 + wsr.0.into_inner().abort(Some(exception)); + } else { + panic!("Couldn't consume WasmStreamingResource."); + } +} + fn encode( scope: &mut v8::HandleScope, args: v8::FunctionCallbackArguments, -- cgit v1.2.3