Merge pull request #1585 from cloudflare/kenton/simplify-named-intercept

Simplify "named intercept"

src/workerd/api/worker-rpc.c++

@@ -51,11 +51,14 @@ kj::Promise<capnp::Response<rpc::JsRpcTarget::CallResults>> WorkerRpc::sendWorke
   // If we have arguments, serialize them.
   // Note that we may fail to serialize some element, in which case this will throw back to JS.
   if (argv.size() > 0) {
+    // TODO(perf): It would be nice if we could serialize directly into the capnp message to avoid
+    // a redundant copy of the bytes here. Maybe we could even cancel serialization early if it
+    // goes over the size limit.
     auto ser = serializeV8(js, js.arr(argv.asPtr()));
     JSG_ASSERT(ser.size() <= MAX_JS_RPC_MESSAGE_SIZE, Error,
         "Serialized RPC requests are limited to 1MiB, but the size of this request was: ",
         ser.size(), " bytes.");
-    builder.initSerializedArgs().setV8Serialized(kj::mv(ser));
+    builder.initArgs().setV8Serialized(ser);
   }
 
   auto callResult = builder.send();
@@ -69,21 +72,19 @@ kj::Promise<capnp::Response<rpc::JsRpcTarget::CallResults>> WorkerRpc::sendWorke
       }));
 }
 
-kj::Maybe<jsg::JsValue> WorkerRpc::getNamed(jsg::Lock& js, kj::StringPtr name) {
+kj::Maybe<WorkerRpc::RpcFunction> WorkerRpc::getRpcMethod(jsg::Lock& js, kj::StringPtr name) {
   // Named intercept is enabled, this means we won't default to legacy behavior.
   // The return value of the function is a promise that resolves once the remote returns the result
   // of the RPC call.
-  return jsg::JsValue(js.wrapPromiseReturningFunction(js.v8Context(),
-      [this, methodName=kj::str(name)]
+  return RpcFunction([this, methodName=kj::str(name)]
       (jsg::Lock& js, const v8::FunctionCallbackInfo<v8::Value>& args) -> jsg::Promise<jsg::Value> {
-        auto& ioContext = IoContext::current();
-        // Wait for the RPC to resolve and then process the result.
-        return ioContext.awaitIo(js, sendWorkerRpc(js, methodName, args),
-            [](jsg::Lock& js, auto result) -> jsg::Value {
-          return jsg::Value(js.v8Isolate, deserializeV8(js, result.getResult().getV8Serialized()));
-        });
-      }
-  ));
+    auto& ioContext = IoContext::current();
+    // Wait for the RPC to resolve and then process the result.
+    return ioContext.awaitIo(js, sendWorkerRpc(js, methodName, args),
+        [](jsg::Lock& js, auto result) -> jsg::Value {
+      return jsg::Value(js.v8Isolate, deserializeV8(js, result.getResult().getV8Serialized()));
+    });
+  });
 }
 
 // The capability that lets us call remote methods over RPC.
@@ -99,7 +100,7 @@ public:
   // Handles the delivery of JS RPC method calls.
   kj::Promise<void> call(CallContext callContext) override {
     auto methodName = kj::heapString(callContext.getParams().getMethodName());
-    auto serializedArgs = callContext.getParams().getSerializedArgs().getV8Serialized().asBytes();
+    auto serializedArgs = callContext.getParams().getArgs().getV8Serialized().asBytes();
 
     // We want to fulfill the callPromise so customEvent can continue executing
     // regardless of the outcome of `call()`.

src/workerd/api/worker-rpc.h

@@ -29,12 +29,12 @@ constexpr size_t MAX_JS_RPC_MESSAGE_SIZE = 1u << 20;
 
 // A WorkerRpc object forwards JS method calls to the remote Worker/Durable Object over RPC.
 // Since methods are not known until runtime, WorkerRpc doesn't define any JS methods.
-// Instead, we use JSG_NAMED_INTERCEPT to intercept property accesses of names that are not known at
-// compile time.
+// Instead, we use JSG_WILDCARD_PROPERTY to intercept property accesses of names that are not known
+// at compile time.
 //
 // WorkerRpc only supports method calls. You cannot, for instance, access a property of a
 // Durable Object over RPC.
-class WorkerRpc : public Fetcher, public jsg::NamedIntercept {
+class WorkerRpc : public Fetcher {
 public:
   WorkerRpc(
       IoOwn<OutgoingFactory> outgoingFactory,
@@ -50,7 +50,10 @@ class WorkerRpc : public Fetcher, public jsg::NamedIntercept {
       kj::StringPtr name,
       const v8::FunctionCallbackInfo<v8::Value>& args);
 
-  kj::Maybe<jsg::JsValue> getNamed(jsg::Lock& js, kj::StringPtr name) override;
+  using RpcFunction = jsg::Function<jsg::Promise<jsg::Value>(
+      const v8::FunctionCallbackInfo<v8::Value>& info)>;
+
+  kj::Maybe<RpcFunction> getRpcMethod(jsg::Lock& js, kj::StringPtr name);
 
   // WARNING: Adding a new JSG_METHOD to a class that extends WorkerRpc can conflict with RPC method
   // names defined on your remote target. For example, if you add a new method `bar()` to the
@@ -66,7 +69,7 @@ class WorkerRpc : public Fetcher, public jsg::NamedIntercept {
   // change log.
   JSG_RESOURCE_TYPE(WorkerRpc, CompatibilityFlags::Reader flags) {
     if (flags.getWorkerdExperimental()) {
-      JSG_NAMED_INTERCEPT();
+      JSG_WILDCARD_PROPERTY(getRpcMethod);
     }
     JSG_INHERIT(Fetcher);
   }

src/workerd/io/worker-interface.capnp

@@ -189,11 +189,11 @@ struct JsValue {
 }
 
 interface JsRpcTarget {
-  call @0 (methodName :Text, serializedArgs :JsValue) -> (result :JsValue);
+  call @0 (methodName :Text, args :JsValue) -> (result :JsValue);
   # Runs a Worker/DO's RPC method.
   #
-  # `methodName` is the name of the method to run, and
-  # `serializedArgs` is an array of arguments that have been serialized by the v8 serializer.
+  # `methodName` is the name of the method to run, and `args` is a JsValue that is always a
+  # JavaScript Array, containing the arguments to the call.
 }
 
 interface EventDispatcher @0xf20697475ec1752d {

src/workerd/jsg/jsg-test.c++

@@ -367,29 +367,24 @@ KJ_TEST("Test JSG_CALLABLE") {
 
 // ========================================================================================
 struct InterceptContext: public ContextGlobalObject {
-  struct ProxyImpl: public jsg::Object,
-                    public jsg::NamedIntercept {
+  struct ProxyImpl: public jsg::Object {
     static jsg::Ref<ProxyImpl> constructor() { return jsg::alloc<ProxyImpl>(); }
 
     int getBar() { return 123; }
 
-    // NamedIntercept implementation
-    kj::Maybe<jsg::JsValue> getNamed(jsg::Lock& js, kj::StringPtr name) override {
+    // JSG_WILDCARD_PROPERTY implementation
+    kj::Maybe<kj::StringPtr> testGetNamed(jsg::Lock& js, kj::StringPtr name) {
       if (name == "foo") {
-        return kj::Maybe(js.str("bar"_kj));
+        return "bar"_kj;
       } else if (name == "abc") {
         JSG_FAIL_REQUIRE(TypeError, "boom");
       }
       return kj::none;
     }
 
-    kj::Array<kj::String> listNamed(Lock& js) override {
-      return kj::arr(kj::str("foo"));
-    }
-
     JSG_RESOURCE_TYPE(ProxyImpl) {
       JSG_READONLY_PROTOTYPE_PROPERTY(bar, getBar);
-      JSG_NAMED_INTERCEPT();
+      JSG_WILDCARD_PROPERTY(testGetNamed);
     }
   };
 
@@ -401,10 +396,6 @@ JSG_DECLARE_ISOLATE_TYPE(InterceptIsolate, InterceptContext, InterceptContext::P
 
 KJ_TEST("Named interceptor") {
   Evaluator<InterceptContext, InterceptIsolate> e(v8System);
-  // Calling Object.keys(p) here just to verify that it does not throw.
-  // Also, the test tries modifying the known intercepted property foo but verifies
-  // that the value is readonly/unchanged.
-  e.expectEval("p = new ProxyImpl; Object.keys(p); p.foo = 123; p.foo", "string", "bar");
   e.expectEval("p = new ProxyImpl; p.bar", "number", "123");
   e.expectEval("p = new ProxyImpl; Reflect.has(p, 'foo')", "boolean", "true");
   e.expectEval("p = new ProxyImpl; Reflect.has(p, 'bar')", "boolean", "true");

src/workerd/jsg/jsg.h

@@ -475,11 +475,24 @@ using HasGetTemplateOverload = decltype(
     wrapper.initReflection(this, __VA_ARGS__); \
   }
 
-// Configures the resource type to implement named property interception.
-// @see the definition of jsg::NamedIntercept in resource.h for more information.
-#define JSG_NAMED_INTERCEPT() \
+// Declares a wildcart property getter. If a property is requested that isn't already present on
+// the object or its prototypes, the wildcard property getter will be given a chance to return the
+// property.
+//
+// Example:
+//
+//   struct MyType {
+//     // Get the value of the named dynamic property. Returns none if the property doesn't exist.
+//     // `SomeType` can be any type that JSG is able to convert to JavaScript.
+//     kj::Maybe<SomeType> getWildcard(jsg::Lock& js, kj::StringPtr name);
+//
+//     JSG_RESOURCE_TYPE(MyType) {
+//       JSG_WILDCARD_PROPERTY(getWildcard);
+//     }
+//   };
+#define JSG_WILDCARD_PROPERTY(method) \
   do { \
-   registry.template registerNamedIntercept<Self>(); \
+   registry.template registerWildcardProperty<Self, decltype(&Self::method), &Self::method>(); \
   } while (false)
 
 // Use inside a JSG_RESOURCE_TYPE block to declare that this type should be considered a "root" for

src/workerd/jsg/jsvalue.h

@@ -554,25 +554,4 @@ class JsMessage final {
   v8::Local<v8::Message> inner;
 };
 
-inline kj::Maybe<JsValue>
-NamedIntercept::getNamed(Lock&, kj::StringPtr) {
-  return kj::none;
-}
-
-inline kj::Maybe<NamedIntercept::Attribute>
-NamedIntercept::queryNamed(Lock& js, kj::StringPtr name) {
-  // By default, we currently only support read only properties.
-  auto list = listNamed(js);
-  for (auto& item : list) {
-    if (item == name) return jsg::NamedIntercept::READ_ONLY_ATTRIBUTE;
-  }
-  return kj::none;
-}
-
-inline kj::Array<kj::String>
-NamedIntercept::listNamed(Lock&) {
-  return kj::Array<kj::String>();
-}
-
-
 }  // namespace workerd::jsg

src/workerd/jsg/resource.c++

@@ -8,9 +8,6 @@ namespace workerd::jsg {
 
 // TODO(cleanup): Factor out toObject(), getInterned() into some sort of v8 tools module?
 
-const NamedIntercept::Attribute NamedIntercept::READ_ONLY_ATTRIBUTE =
-    NamedIntercept::Attribute::READ_ONLY | NamedIntercept::Attribute::DONT_DELETE;
-
 void exposeGlobalScopeType(v8::Isolate* isolate, v8::Local<v8::Context> context) {
   auto global = context->Global();