Add a safe wrapper around ForOfIterator (#736)

`ForOfIterator` implements `for .. of` iteration in spidermonkey. It
holds stack roots and is a pain to use directly due to some weird
shenanigans where bindgen adds extra padding to the struct for some LLVM
versions. Its also hard to use in a crash-proof manner, because
`ForOfIterator` must not be moved. We'd have to `Pin<Box<>>` it to be
safe, which adds overhead.

This change instead adds `mozjs::rust::for_of`, a function that takes a
callback and uses `ForOfIterator` correctly so users don't have to worry
about it.

Testing: The iterator is used during conversion of IDL sequences, and
its covered by WPT. I'm also using it for web animations locally.
Part of servo/servo#36950
Servo PR: servo/servo#44356

---------

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>

Author: simonwuelker

mozjs-sys/Cargo.toml

@@ -2,7 +2,7 @@
 name = "mozjs_sys"
 description = "System crate for the Mozilla SpiderMonkey JavaScript engine."
 repository.workspace = true
-version = "0.140.10-2"
+version = "0.140.10-3"
 authors = ["Mozilla", "The Servo Project Developers"]
 links = "mozjs"
 license.workspace = true

mozjs-sys/src/jsapi.cpp

@@ -172,6 +172,10 @@ bool JS_ForOfIteratorNext(JS::ForOfIterator* iterator,
   return iterator->next(val, done);
 }
 
+bool JS_ForOfIteratorValueIsIterable(const JS::ForOfIterator* iterator) {
+  return iterator->valueIsIterable();
+}
+
 // These functions are only intended for use in testing,
 // to make sure that the Rust implementation of JS::Value
 // agrees with the C++ implementation.

mozjs-sys/src/jsimpls.rs

@@ -2,8 +2,9 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use crate::jsapi::glue::JS_ForOfIteratorInit;
-use crate::jsapi::glue::JS_ForOfIteratorNext;
+use crate::jsapi::glue::{
+    JS_ForOfIteratorInit, JS_ForOfIteratorNext, JS_ForOfIteratorValueIsIterable,
+};
 use crate::jsapi::jsid;
 use crate::jsapi::mozilla;
 use crate::jsapi::JSAutoRealm;
@@ -577,6 +578,10 @@ impl JS::ForOfIterator {
     pub unsafe fn next(&mut self, val: JS::MutableHandleValue, done: *mut bool) -> bool {
         JS_ForOfIteratorNext(self, val, done)
     }
+
+    pub fn is_iterable(&self) -> bool {
+        unsafe { JS_ForOfIteratorValueIsIterable(self) }
+    }
 }
 
 impl<T> mozilla::Range<T> {

mozjs/Cargo.toml

@@ -27,7 +27,7 @@ encoding_rs = "0.8.35"
 libc.workspace = true
 log = "0.4"
 # When doing non-version changes also update ../mozjs-sys/etc/sm-security-bump.py
-mozjs_sys = { version = "=0.140.10-2", path = "../mozjs-sys" }
+mozjs_sys = { version = "=0.140.10-3", path = "../mozjs-sys" }
 num-traits = "0.2"
 
 [target.'cfg(target_arch = "wasm32")'.dev-dependencies]

mozjs/src/conversions.rs

@@ -31,25 +31,25 @@
 use crate::error::throw_type_error;
 use crate::jsapi::AssertSameCompartment;
 use crate::jsapi::JS;
-use crate::jsapi::{ForOfIterator, ForOfIterator_NonIterableBehavior};
 use crate::jsapi::{Heap, JS_DefineElement, JS_GetLatin1StringCharsAndLength};
-use crate::jsapi::{JSContext, JSObject, JSString, RootedObject, RootedValue};
+use crate::jsapi::{JSContext, JSObject, JSString};
 use crate::jsapi::{JS_DeprecatedStringHasLatin1Chars, JS_NewStringCopyUTF8N, JSPROP_ENUMERATE};
 use crate::jsapi::{JS_GetTwoByteStringCharsAndLength, NewArrayObject1};
 use crate::jsval::{BooleanValue, DoubleValue, Int32Value, NullValue, UInt32Value, UndefinedValue};
 use crate::jsval::{JSVal, ObjectOrNullValue, ObjectValue, StringValue, SymbolValue};
 use crate::rooted;
+use crate::rust::for_of;
 use crate::rust::maybe_wrap_value;
+use crate::rust::ForOfIterationFailure;
 use crate::rust::{maybe_wrap_object_or_null_value, maybe_wrap_object_value, ToString};
 use crate::rust::{HandleValue, MutableHandleValue};
 use crate::rust::{ToBoolean, ToInt32, ToInt64, ToNumber, ToUint16, ToUint32, ToUint64};
 use libc;
 use log::debug;
-use mozjs_sys::jsgc::Rooted;
 use num_traits::PrimInt;
 use std::borrow::Cow;
 use std::ffi::CStr;
-use std::mem;
+use std::ops::ControlFlow;
 use std::ptr::NonNull;
 use std::rc::Rc;
 use std::{ptr, slice};
@@ -757,31 +757,6 @@ impl<T: ToJSValConvertible> ToJSValConvertible for Vec<T> {
     }
 }
 
-/// Rooting guard for the iterator field of ForOfIterator.
-/// Behaves like RootedGuard (roots on creation, unroots on drop),
-/// but borrows and allows access to the whole ForOfIterator, so
-/// that methods on ForOfIterator can still be used through it.
-struct ForOfIteratorGuard<'a> {
-    root: &'a mut ForOfIterator,
-}
-
-impl<'a> ForOfIteratorGuard<'a> {
-    fn new(cx: *mut JSContext, root: &'a mut ForOfIterator) -> Self {
-        unsafe {
-            Rooted::add_to_root_stack(&raw mut root.iterator, cx);
-        }
-        ForOfIteratorGuard { root }
-    }
-}
-
-impl<'a> Drop for ForOfIteratorGuard<'a> {
-    fn drop(&mut self) {
-        unsafe {
-            self.root.iterator.remove_from_root_stack();
-        }
-    }
-}
-
 impl<C: Clone, T: FromJSValConvertible<Config = C>> FromJSValConvertible for Vec<T> {
     type Config = C;
 
@@ -794,57 +769,31 @@ impl<C: Clone, T: FromJSValConvertible<Config = C>> FromJSValConvertible for Vec
             return Ok(ConversionResult::Failure(c"Value is not an object".into()));
         }
 
-        // Depending on the version of LLVM in use, bindgen can end up including
-        // a padding field in the ForOfIterator. To support multiple versions of
-        // LLVM that may not have the same fields as a result, we create an empty
-        // iterator instance and initialize a non-empty instance using the empty
-        // instance as a base value.
-        #[allow(unused_variables)]
-        let zero = mem::zeroed();
-        let mut iterator = ForOfIterator {
-            cx_: cx,
-            iterator: RootedObject::new_unrooted(ptr::null_mut()),
-            nextMethod: RootedValue::new_unrooted(JSVal { asBits_: 0 }),
-            index: ::std::u32::MAX, // NOT_ARRAY
-            ..zero
-        };
-        let iterator = ForOfIteratorGuard::new(cx, &mut iterator);
-        let iterator: &mut ForOfIterator = &mut *iterator.root;
-
-        if !iterator.init(
-            value.into(),
-            ForOfIterator_NonIterableBehavior::AllowNonIterable,
-        ) {
-            return Err(());
-        }
-
-        if iterator.iterator.data.is_null() {
-            return Ok(ConversionResult::Failure(c"Value is not iterable".into()));
-        }
+        let mut return_value = vec![];
+        let result = for_of(cx, value, |iterator_element| {
+            let conversion_result = T::from_jsval(cx, iterator_element, option.clone())
+                .map_err(|_| ForOfIterationFailure::JSFailed)?;
+            return_value.push(match conversion_result {
+                ConversionResult::Success(value) => value,
+                ConversionResult::Failure(error) => {
+                    return Err(ForOfIterationFailure::Other(error));
+                }
+            });
 
-        let mut ret = vec![];
+            Ok(ControlFlow::Continue(()))
+        });
 
-        loop {
-            let mut done = false;
-            rooted!(in(cx) let mut val = UndefinedValue());
-            if !iterator.next(val.handle_mut().into(), &mut done) {
-                return Err(());
+        match result {
+            Ok(_) => Ok(ConversionResult::Success(return_value)),
+            Err(ForOfIterationFailure::ValueIsNotIterable) => {
+                Ok(ConversionResult::Failure(c"Value is not iterable".into()))
             }
-
-            if done {
-                break;
+            Err(ForOfIterationFailure::JSFailed) => Err(()),
+            Err(ForOfIterationFailure::Other(error)) => {
+                throw_type_error(cx, error.as_ref());
+                Err(())
             }
-
-            ret.push(match T::from_jsval(cx, val.handle(), option.clone())? {
-                ConversionResult::Success(v) => v,
-                ConversionResult::Failure(e) => {
-                    throw_type_error(cx, e.as_ref());
-                    return Err(());
-                }
-            });
         }
-
-        Ok(ret).map(ConversionResult::Success)
     }
 }
 

mozjs/src/rust.rs

@@ -9,8 +9,9 @@ use std::char;
 use std::default::Default;
 use std::ffi::{c_char, c_void, CStr, CString};
 use std::marker::PhantomData;
+use std::mem;
 use std::mem::MaybeUninit;
-use std::ops::{Deref, DerefMut};
+use std::ops::{ControlFlow, Deref, DerefMut};
 use std::ptr::{self, NonNull};
 use std::slice;
 use std::str;
@@ -63,10 +64,12 @@ use crate::jsapi::{JS_RequestInterruptCallback, JS_RequestInterruptCallbackCanWa
 use crate::jsapi::{JS_SetGCParameter, JS_SetNativeStackQuota, JS_WrapObject, JS_WrapValue};
 use crate::jsapi::{JS_StackCapture_AllFrames, JS_StackCapture_MaxFrames};
 use crate::jsapi::{PersistentRootedObjectVector, ReadOnlyCompileOptions, RootingContext};
+use crate::jsapi::{
+    RootedObject, RootedValue, ToUint32Slow, ToUint64Slow, ToWindowProxyIfWindowSlow,
+};
 use crate::jsapi::{SetWarningReporter, SourceText, ToBooleanSlow};
 use crate::jsapi::{ToInt32Slow, ToInt64Slow, ToNumberSlow, ToStringSlow, ToUint16Slow};
-use crate::jsapi::{ToUint32Slow, ToUint64Slow, ToWindowProxyIfWindowSlow};
-use crate::jsval::{JSVal, ObjectValue};
+use crate::jsval::{JSVal, ObjectValue, UndefinedValue};
 use crate::panic::maybe_resume_unwind;
 use crate::realm::AutoRealm;
 use log::{debug, warn};
@@ -1267,6 +1270,105 @@ impl<'a> Handle<'a, StackGCVector<*mut JSString, js::TempAllocPolicy>> {
     }
 }
 
+#[derive(Clone, Copy, Debug)]
+pub enum ForOfIterationFailure<OtherError> {
+    ValueIsNotIterable,
+    /// There is a pending exception
+    JSFailed,
+    Other(OtherError),
+}
+
+impl<OtherError> From<OtherError> for ForOfIterationFailure<OtherError> {
+    fn from(value: OtherError) -> Self {
+        Self::Other(value)
+    }
+}
+
+/// Helper for running `for .. of` iteration from rust.
+///
+/// If `Ok()` is returned then the iteration completed without unexpected failures.
+///
+/// The callback returns `Err()` to indicate a pending exception or `Ok()` containing a boolean
+/// value that is `true` if the iterator should continue iterating.
+pub fn for_of<Callback, OtherError>(
+    cx: *mut JSContext,
+    iterable: HandleValue<'_>,
+    mut callback: Callback,
+) -> Result<(), ForOfIterationFailure<OtherError>>
+where
+    Callback: FnMut(HandleValue<'_>) -> Result<ControlFlow<()>, ForOfIterationFailure<OtherError>>,
+{
+    // Depending on the version of LLVM in use, bindgen can end up including
+    // a padding field in the ForOfIterator. To support multiple versions of
+    // LLVM that may not have the same fields as a result, we create an empty
+    // iterator instance and initialize a non-empty instance using the empty
+    // instance as a base value.
+    #[allow(unused_variables)]
+    let zero = unsafe { mem::zeroed() };
+    let mut iterator = jsapi::ForOfIterator {
+        cx_: cx,
+        iterator: RootedObject::new_unrooted(ptr::null_mut()),
+        nextMethod: RootedValue::new_unrooted(JSVal { asBits_: 0 }),
+        index: ::std::u32::MAX, // NOT_ARRAY
+        ..zero
+    };
+
+    // This code would benefit from https://github.com/rust-lang/rust/issues/144426
+    struct IteratorRootGuard<'a> {
+        inner: &'a mut jsapi::ForOfIterator,
+    }
+
+    impl<'a> Drop for IteratorRootGuard<'a> {
+        fn drop(&mut self) {
+            // SAFETY: These values won't be used anymore
+            unsafe {
+                self.inner.iterator.remove_from_root_stack();
+                self.inner.nextMethod.remove_from_root_stack();
+            }
+        }
+    }
+    let guard = IteratorRootGuard {
+        inner: &mut iterator,
+    };
+    let iterator = &mut *guard.inner;
+
+    unsafe {
+        RootedObject::add_to_root_stack(&raw mut iterator.iterator, cx);
+        RootedValue::add_to_root_stack(&raw mut iterator.nextMethod, cx);
+    }
+
+    let success = unsafe {
+        iterator.init(
+            iterable.into_handle(),
+            jsapi::ForOfIterator_NonIterableBehavior::AllowNonIterable,
+        )
+    };
+    if !success {
+        return Err(ForOfIterationFailure::JSFailed);
+    }
+    if !iterator.is_iterable() {
+        return Err(ForOfIterationFailure::ValueIsNotIterable);
+    }
+
+    let mut done = false;
+    rooted!(in(cx) let mut value = UndefinedValue());
+    loop {
+        if !unsafe { iterator.next(value.handle_mut().into(), &mut done) } {
+            return Err(ForOfIterationFailure::JSFailed);
+        }
+
+        if done {
+            break;
+        }
+
+        if callback(value.handle())?.is_break() {
+            break;
+        }
+    }
+
+    Ok(())
+}
+
 /// Wrappers for JSAPI methods that accept lifetimed Handle and MutableHandle arguments
 pub mod wrappers {
     macro_rules! wrap {