Allow overriding the received (this) of a jsg::Function, update forEach impls (#1027)

Author: jasnell

src/workerd/api/form-data.c++

@@ -413,15 +413,18 @@ jsg::Ref<FormData::ValueIterator> FormData::values(jsg::Lock&) {
 
 void FormData::forEach(
     jsg::Lock& js,
-    jsg::V8Ref<v8::Function> callback,
-    jsg::Optional<jsg::Value> thisArg,
-    const jsg::TypeHandler<EntryType>& handler) {
-  auto localCallback = callback.getHandle(js);
-  auto localThisArg = thisArg.map([&js](jsg::Value& v) { return v.getHandle(js); })
-      .orDefault(js.v8Undefined());
-  // JSG_THIS.tryGetHandle() is guaranteed safe because `forEach()` is only called
-  // from JavaScript, which means a Headers JS wrapper object must already exist.
-  auto localParams = KJ_ASSERT_NONNULL(JSG_THIS.tryGetHandle(js.v8Isolate));
+    jsg::Function<void(EntryType, kj::StringPtr, jsg::Ref<FormData>)> callback,
+    jsg::Optional<jsg::Value> thisArg) {
+  // Here, if the thisArg is not passed, or is passed explicitly as a null or
+  // undefined, then undefined is used as the thisArg.
+  auto receiver = js.v8Undefined();
+  KJ_IF_MAYBE(arg, thisArg) {
+    auto handle = arg->getHandle(js);
+    if (!handle->IsNullOrUndefined()) {
+      receiver = handle;
+    }
+  }
+  callback.setReceiver(js.v8Ref(receiver));
 
   // On each iteration of the for loop, a JavaScript callback is invokved. If a new
   // item is appended to the URLSearchParams within that function, the loop must pick
@@ -430,16 +433,7 @@ void FormData::forEach(
   // are added to the search params unconditionally on each iteration.
   for (size_t i = 0; i < this->data.size(); i++) {
     auto& [key, value] = this->data[i];
-    static constexpr auto ARG_COUNT = 3;
-
-    v8::Local<v8::Value> args[ARG_COUNT] = {
-      handler.wrap(js, clone(value)),
-      jsg::v8Str(js.v8Isolate, key),
-      localParams,
-    };
-    // Call jsg::check() to propagate exceptions, but we don't expect any
-    // particular return value.
-    jsg::check(localCallback->Call(js.v8Context(), localThisArg, ARG_COUNT, args));
+    callback(js, clone(value), key, JSG_THIS);
   }
 }
 

src/workerd/api/form-data.h

@@ -98,9 +98,8 @@ class FormData: public jsg::Object {
 
   void forEach(
       jsg::Lock& js,
-      jsg::V8Ref<v8::Function> callback,
-      jsg::Optional<jsg::Value> thisArg,
-      const jsg::TypeHandler<EntryType>& handler);
+      jsg::Function<void(EntryType, kj::StringPtr, jsg::Ref<FormData>)> callback,
+      jsg::Optional<jsg::Value> thisArg);
 
   JSG_RESOURCE_TYPE(FormData, CompatibilityFlags::Reader flags) {
     JSG_METHOD(append);

src/workerd/api/http.c++

@@ -396,31 +396,19 @@ jsg::Ref<Headers::ValueIterator> Headers::values(jsg::Lock& js) {
 
 void Headers::forEach(
     jsg::Lock& js,
-    jsg::V8Ref<v8::Function> callback,
+    jsg::Function<void(kj::StringPtr, kj::StringPtr, jsg::Ref<Headers>)> callback,
     jsg::Optional<jsg::Value> thisArg) {
-  auto localCallback = callback.getHandle(js);
-  auto localThisArg = thisArg.map([&](jsg::Value& v) { return v.getHandle(js); })
-      .orDefault(js.v8Undefined());
-
-  auto isolate = js.v8Isolate;
-
-  // JSG_THIS.getHandle() is guaranteed safe because `forEach()` is only called
-  // from JavaScript, which means a Headers JS wrapper object must already exist.
-  auto localHeaders = KJ_ASSERT_NONNULL(JSG_THIS.tryGetHandle(isolate));
+  auto receiver = js.v8Undefined();
+  KJ_IF_MAYBE(arg, thisArg) {
+    auto handle = arg->getHandle(js);
+    if (!handle->IsNullOrUndefined()) {
+      receiver = handle;
+    }
+  }
+  callback.setReceiver(js.v8Ref(receiver));
 
-  auto context = js.v8Context();  // Needed later for Call().
   for (auto& entry: getDisplayedHeaders(js)) {
-    static constexpr auto ARG_COUNT = 3;
-    v8::Local<v8::Value> args[ARG_COUNT] = {
-      jsg::v8Str(isolate, entry.value),
-      // Intern the header name and not the value since the names are generally
-      // stable.
-      jsg::v8StrIntern(isolate, entry.key),
-      localHeaders,
-    };
-    // Call jsg::check() to propagate exceptions, but we don't expect any
-    // particular return value.
-    jsg::check(localCallback->Call(context, localThisArg, ARG_COUNT, args));
+    callback(js, entry.value, entry.key, JSG_THIS);
   }
 }
 

src/workerd/api/http.h

@@ -101,8 +101,8 @@ class Headers: public jsg::Object {
   void append(jsg::ByteString name, jsg::ByteString value);
   void delete_(jsg::ByteString name);
   void forEach(jsg::Lock& js,
-      jsg::V8Ref<v8::Function>,
-      jsg::Optional<jsg::Value>);
+               jsg::Function<void(kj::StringPtr, kj::StringPtr, jsg::Ref<Headers>)>,
+               jsg::Optional<jsg::Value>);
 
   JSG_ITERATOR(EntryIterator, entries,
                 kj::Array<jsg::ByteString>,

src/workerd/api/url-standard.c++

@@ -2079,25 +2079,24 @@ jsg::Ref<URLSearchParams::ValueIterator> URLSearchParams::values(jsg::Lock&) {
 
 void URLSearchParams::forEach(
     jsg::Lock& js,
-    jsg::V8Ref<v8::Function> callback,
+    jsg::Function<void(jsg::UsvStringPtr, jsg::UsvStringPtr, jsg::Ref<URLSearchParams>)> callback,
     jsg::Optional<jsg::Value> thisArg) {
-  auto cb = callback.getHandle(js);
-  auto this_ = thisArg.map([&js](jsg::Value& v) { return v.getHandle(js); })
-      .orDefault(js.v8Undefined());
-  auto query = KJ_ASSERT_NONNULL(JSG_THIS.tryGetHandle(js.v8Isolate));
-  // On each iteration of the for loop, a JavaScript callback is invokved. If a new
+  auto receiver = js.v8Undefined();
+  KJ_IF_MAYBE(arg, thisArg) {
+    auto handle = arg->getHandle(js);
+    if (!handle->IsNullOrUndefined()) {
+      receiver = handle;
+    }
+  }
+  callback.setReceiver(js.v8Ref(receiver));
+  // On each iteration of the for loop, a JavaScript callback is invoked. If a new
   // item is appended to the URLSearchParams within that function, the loop must pick
   // it up. Using the classic for (;;) syntax here allows for that. However, this does
   // mean that it's possible for a user to trigger an infinite loop here if new items
   // are added to the search params unconditionally on each iteration.
   for (size_t i = 0; i < list.size(); i++) {
     auto& entry = list[i];
-    v8::Local<v8::Value> args[3] = {
-      jsg::v8Str(js.v8Isolate, entry.value),
-      jsg::v8Str(js.v8Isolate, entry.name),
-      query,
-    };
-    jsg::check(cb->Call(js.v8Context(), this_, 3, args));
+    callback(js, entry.value, entry.name, JSG_THIS);
   }
 }
 

src/workerd/api/url-standard.h

@@ -116,10 +116,9 @@ class URLSearchParams: public jsg::Object {
                IteratorState,
                valueIteratorNext)
 
-  void forEach(
-      jsg::Lock& js,
-      jsg::V8Ref<v8::Function> callback,
-      jsg::Optional<jsg::Value> thisArg);
+  void forEach(jsg::Lock&,
+               jsg::Function<void(jsg::UsvStringPtr, jsg::UsvStringPtr, jsg::Ref<URLSearchParams>)>,
+               jsg::Optional<jsg::Value>);
 
   jsg::UsvString toString();
 

src/workerd/api/url.c++

@@ -587,31 +587,25 @@ void URLSearchParams::sort() {
 
 void URLSearchParams::forEach(
     jsg::Lock& js,
-    jsg::V8Ref<v8::Function> callback,
+    jsg::Function<void(kj::StringPtr, kj::StringPtr, jsg::Ref<URLSearchParams>)> callback,
     jsg::Optional<jsg::Value> thisArg) {
-  auto localCallback = callback.getHandle(js);
-  auto localThisArg = thisArg.map([&js](jsg::Value& v) { return v.getHandle(js); })
-      .orDefault(js.v8Undefined());
-  // JSG_THIS.getHandle() is guaranteed safe because `forEach()` is only called
-  // from JavaScript, which means a Headers JS wrapper object must already exist.
-  auto localParams = KJ_ASSERT_NONNULL(JSG_THIS.tryGetHandle(js.v8Isolate));
-
-  // On each iteration of the for loop, a JavaScript callback is invokved. If a new
+  auto receiver = js.v8Undefined();
+  KJ_IF_MAYBE(arg, thisArg) {
+    auto handle = arg->getHandle(js);
+    if (!handle->IsNullOrUndefined()) {
+      receiver = handle;
+    }
+  }
+  callback.setReceiver(js.v8Ref(receiver));
+
+  // On each iteration of the for loop, a JavaScript callback is invoked. If a new
   // item is appended to the this->url->query within that function, the loop must pick
   // it up. Using the classic for (;;) syntax here allows for that. However, this does
   // mean that it's possible for a user to trigger an infinite loop here if new items
   // are added to the search params unconditionally on each iteration.
   for (size_t i = 0; i < this->url->query.size(); i++) {
     auto& [key, value] = this->url->query[i];
-    static constexpr auto ARG_COUNT = 3;
-    v8::Local<v8::Value> args[ARG_COUNT] = {
-      jsg::v8Str(js.v8Isolate, value),
-      jsg::v8Str(js.v8Isolate, key),
-      localParams,
-    };
-    // Call jsg::check() to propagate exceptions, but we don't expect any
-    // particular return value.
-    jsg::check(localCallback->Call(js.v8Context(), localThisArg, ARG_COUNT, args));
+    callback(js, value, key, JSG_THIS);
   }
 }
 

src/workerd/api/url.h

@@ -161,10 +161,9 @@ class URLSearchParams: public jsg::Object {
                 IteratorState,
                 valueIteratorNext)
 
-  void forEach(
-      jsg::Lock& js,
-      jsg::V8Ref<v8::Function> callback,
-      jsg::Optional<jsg::Value> thisArg);
+  void forEach(jsg::Lock&,
+               jsg::Function<void(kj::StringPtr, kj::StringPtr, jsg::Ref<URLSearchParams>)>,
+               jsg::Optional<jsg::Value>);
 
   kj::String toString();
 

src/workerd/jsg/function.h

@@ -122,13 +122,18 @@ class Function<Ret(Args...)> {
 
   typedef Ret Wrapper(
       jsg::Lock& js,
-      v8::Local<v8::Object> receiver,  // the `this` value in the function
+      v8::Local<v8::Value> receiver,  // the `this` value in the function
       v8::Local<v8::Function> fn,
       Args...);
   // When holding a JavaScript function, `Wrapper` is a C++ function that will handle converting
   // C++ arguments into JavaScript values and then call the JS function.
 
   Function(Wrapper* wrapper, V8Ref<v8::Object> receiver, V8Ref<v8::Function> function)
+      : Function(wrapper,
+                 receiver.cast<v8::Value>(Lock::from(v8::Isolate::GetCurrent())),
+                 kj::mv(function)) {}
+
+  Function(Wrapper* wrapper, Value receiver, V8Ref<v8::Function> function)
       : impl(JsImpl {
           .wrapper = kj::mv(wrapper),
           .receiver = kj::mv(receiver),
@@ -231,12 +236,18 @@ class Function<Ret(Args...)> {
     __builtin_unreachable();
   }
 
+  inline void setReceiver(Value receiver) {
+    KJ_IF_MAYBE(i, impl.template tryGet<JsImpl>()) {
+      i->receiver = kj::mv(receiver);
+    }
+  }
+
 private:
   Function(Ref<NativeFunction>&& func) : impl(kj::mv(func)) {}
 
   struct JsImpl {
     Wrapper* wrapper;
-    V8Ref<v8::Object> receiver;
+    Value receiver;
     V8Ref<v8::Function> handle;
   };
 
@@ -335,7 +346,7 @@ class FunctionWrapper {
     auto isolate = context->GetIsolate();
 
     auto wrapperFn = [](Lock& js,
-                        v8::Local<v8::Object> receiver,
+                        v8::Local<v8::Value> receiver,
                         v8::Local<v8::Function> func,
                         Args... args) -> Ret {
       auto isolate = js.v8Isolate;
@@ -372,7 +383,7 @@ class FunctionWrapper {
     auto isolate = context->GetIsolate();
 
     auto wrapperFn = [](Lock& js,
-                        v8::Local<v8::Object> receiver,
+                        v8::Local<v8::Value> receiver,
                         v8::Local<v8::Function> func,
                         Args... args) -> Ret {
       auto isolate = js.v8Isolate;