script: make Error::to_jsval safe (#35411)

Signed-off-by: Stephen Muss <stephenmuss@gmail.com>

components/script/dom/bindings/error.rs

@@ -11,9 +11,7 @@ use backtrace::Backtrace;
 use js::error::{throw_range_error, throw_type_error};
 #[cfg(feature = "js_backtrace")]
 use js::jsapi::StackFormat as JSStackFormat;
-use js::jsapi::{
+use js::jsapi::{ExceptionStackBehavior, JS_ClearPendingException, JS_IsExceptionPending};
-    ExceptionStackBehavior, JSContext, JS_ClearPendingException, JS_IsExceptionPending,
-};
 use js::jsval::UndefinedValue;
 use js::rust::wrappers::{JS_ErrorFromException, JS_GetPendingException, JS_SetPendingException};
 use js::rust::{HandleObject, HandleValue, MutableHandleValue};
@@ -341,19 +339,21 @@ pub(crate) fn throw_constructor_without_new(cx: SafeJSContext, name: &str) {
 impl Error {
     /// Convert this error value to a JS value, consuming it in the process.
     #[allow(clippy::wrong_self_convention)]
-    pub(crate) unsafe fn to_jsval(
+    pub(crate) fn to_jsval(
         self,
-        cx: *mut JSContext,
+        cx: SafeJSContext,
         global: &GlobalScope,
         rval: MutableHandleValue,
     ) {
         match self {
             Error::JSFailed => (),
-            _ => assert!(!JS_IsExceptionPending(cx)),
+            _ => unsafe { assert!(!JS_IsExceptionPending(*cx)) },
+        }
+        throw_dom_exception(cx, global, self);
+        unsafe {
+            assert!(JS_IsExceptionPending(*cx));
+            assert!(JS_GetPendingException(*cx, rval));
+            JS_ClearPendingException(*cx);
         }
-        throw_dom_exception(SafeJSContext::from_ptr(cx), global, self);
-        assert!(JS_IsExceptionPending(cx));
-        assert!(JS_GetPendingException(cx, rval));
-        JS_ClearPendingException(cx);
     }
 }

components/script/dom/promise.rs

@@ -219,14 +219,11 @@ impl Promise {
         self.reject(cx, v.handle());
     }
 
-    #[allow(unsafe_code)]
     pub(crate) fn reject_error(&self, error: Error) {
         let cx = GlobalScope::get_cx();
         let _ac = enter_realm(self);
         rooted!(in(*cx) let mut v = UndefinedValue());
-        unsafe {
+        error.to_jsval(cx, &self.global(), v.handle_mut());
-            error.to_jsval(*cx, &self.global(), v.handle_mut());
-        }
         self.reject(cx, v.handle());
     }
 

components/script/dom/readablestream.rs

@@ -417,11 +417,10 @@ impl ReadableStream {
 
     /// <https://streams.spec.whatwg.org/#readable-stream-error>
     /// Note: in other use cases this call happens via the controller.
-    #[allow(unsafe_code)]
     pub(crate) fn error_native(&self, error: Error) {
         let cx = GlobalScope::get_cx();
         rooted!(in(*cx) let mut error_val = UndefinedValue());
-        unsafe { error.to_jsval(*cx, &self.global(), error_val.handle_mut()) };
+        error.to_jsval(cx, &self.global(), error_val.handle_mut());
         self.error(error_val.handle());
     }
 

components/script/dom/readablestreambyobreader.rs

@@ -153,20 +153,17 @@ impl ReadableStreamBYOBReader {
     }
 
     /// <https://streams.spec.whatwg.org/#abstract-opdef-readablestreambyobreaderrelease>
-    #[allow(unsafe_code)]
     pub(crate) fn release(&self) -> Fallible<()> {
         // Perform ! ReadableStreamReaderGenericRelease(reader).
         self.generic_release()?;
         // Let e be a new TypeError exception.
         let cx = GlobalScope::get_cx();
         rooted!(in(*cx) let mut error = UndefinedValue());
-        unsafe {
         Error::Type("Reader is released".to_owned()).to_jsval(
-                *cx,
+            cx,
             &self.global(),
             error.handle_mut(),
-            )
+        );
-        };
 
         // Perform ! ReadableStreamBYOBReaderErrorReadIntoRequests(reader, e).
         self.error_read_into_requests(error.handle());

components/script/dom/readablestreamdefaultcontroller.rs

@@ -450,7 +450,6 @@ impl ReadableStreamDefaultController {
     }
 
     /// <https://streams.spec.whatwg.org/#readable-stream-default-controller-call-pull-if-needed>
-    #[allow(unsafe_code)]
     fn call_pull_if_needed(&self, can_gc: CanGc) {
         if !self.should_call_pull() {
             return;
@@ -500,11 +499,9 @@ impl ReadableStreamDefaultController {
             let cx = GlobalScope::get_cx();
             rooted!(in(*cx) let mut rval = UndefinedValue());
             // TODO: check if `self.global()` is the right globalscope.
-            unsafe {
             error
                 .clone()
-                    .to_jsval(*cx, &self.global(), rval.handle_mut())
+                .to_jsval(cx, &self.global(), rval.handle_mut());
-            };
             let promise = Promise::new(&global, can_gc);
             promise.reject_native(&rval.handle());
             promise
@@ -513,7 +510,6 @@ impl ReadableStreamDefaultController {
     }
 
     /// <https://streams.spec.whatwg.org/#rs-default-controller-private-cancel>
-    #[allow(unsafe_code)]
     pub(crate) fn perform_cancel_steps(
         &self,
         reason: SafeHandleValue,
@@ -540,11 +536,9 @@ impl ReadableStreamDefaultController {
             let cx = GlobalScope::get_cx();
             rooted!(in(*cx) let mut rval = UndefinedValue());
             // TODO: check if `self.global()` is the right globalscope.
-            unsafe {
             error
                 .clone()
-                    .to_jsval(*cx, &self.global(), rval.handle_mut())
+                .to_jsval(cx, &self.global(), rval.handle_mut());
-            };
             let promise = Promise::new(&global, can_gc);
             promise.reject_native(&rval.handle());
             promise

components/script/dom/readablestreamdefaultreader.rs

@@ -245,20 +245,17 @@ impl ReadableStreamDefaultReader {
     }
 
     /// <https://streams.spec.whatwg.org/#abstract-opdef-readablestreamdefaultreaderrelease>
-    #[allow(unsafe_code)]
     pub(crate) fn release(&self) -> Fallible<()> {
         // Perform ! ReadableStreamReaderGenericRelease(reader).
         self.generic_release()?;
         // Let e be a new TypeError exception.
         let cx = GlobalScope::get_cx();
         rooted!(in(*cx) let mut error = UndefinedValue());
-        unsafe {
         Error::Type("Reader is released".to_owned()).to_jsval(
-                *cx,
+            cx,
             &self.global(),
             error.handle_mut(),
-            )
+        );
-        };
 
         // Perform ! ReadableStreamDefaultReaderErrorReadRequests(reader, e).
         self.error_read_requests(error.handle());
@@ -363,19 +360,16 @@ impl ReadableStreamDefaultReaderMethods<crate::DomTypeHolder> for ReadableStream
     }
 
     /// <https://streams.spec.whatwg.org/#default-reader-read>
-    #[allow(unsafe_code)]
     fn Read(&self, can_gc: CanGc) -> Rc<Promise> {
         // If this.[[stream]] is undefined, return a promise rejected with a TypeError exception.
         if self.stream.get().is_none() {
             let cx = GlobalScope::get_cx();
             rooted!(in(*cx) let mut error = UndefinedValue());
-            unsafe {
             Error::Type("stream is undefined".to_owned()).to_jsval(
-                    *cx,
+                cx,
                 &self.global(),
                 error.handle_mut(),
-                )
+            );
-            };
             return Promise::new_rejected(&self.global(), cx, error.handle()).unwrap();
         }
         // Let promise be a new promise.

components/script/dom/readablestreamgenericreader.rs

@@ -101,13 +101,11 @@ pub(crate) trait ReadableStreamGenericReader {
                 // Otherwise, set reader.[[closedPromise]] to a promise rejected with a TypeError exception.
                 let cx = GlobalScope::get_cx();
                 rooted!(in(*cx) let mut error = UndefinedValue());
-                unsafe {
                 Error::Type("Cannot release lock due to stream state.".to_owned()).to_jsval(
-                        *cx,
+                    cx,
                     &stream.global(),
                     error.handle_mut(),
-                    )
+                );
-                };
 
                 self.set_closed_promise(
                     Promise::new_rejected(&stream.global(), cx, error.handle()).unwrap(),

components/script/script_module.rs

@@ -76,10 +76,9 @@ use crate::realms::{enter_realm, AlreadyInRealm, InRealm};
 use crate::script_runtime::{CanGc, JSContext as SafeJSContext};
 use crate::task::TaskBox;
 
-#[allow(unsafe_code)]
+fn gen_type_error(global: &GlobalScope, string: String) -> RethrowError {
-unsafe fn gen_type_error(global: &GlobalScope, string: String) -> RethrowError {
     rooted!(in(*GlobalScope::get_cx()) let mut thrown = UndefinedValue());
-    Error::Type(string).to_jsval(*GlobalScope::get_cx(), global, thrown.handle_mut());
+    Error::Type(string).to_jsval(GlobalScope::get_cx(), global, thrown.handle_mut());
 
     RethrowError(RootedTraceableBox::from_box(Heap::boxed(thrown.get())))
 }
@@ -1428,8 +1427,7 @@ fn fetch_an_import_module_script_graph(
 
     // Step 2.
     if url.is_err() {
-        let specifier_error =
+        let specifier_error = gen_type_error(global, "Wrong module specifier".to_owned());
-            unsafe { gen_type_error(global, "Wrong module specifier".to_owned()) };
         return Err(specifier_error);
     }