This() deprecation branch

src/workerd/jsg/resource.h

@@ -639,12 +639,19 @@ struct GetterCallback;
       liftKj(info, [&]() {                                                                         \
         auto isolate = info.GetIsolate();                                                          \
         auto context = isolate->GetCurrentContext();                                               \
+        // For interceptors on the prototype, HolderV2() returns the prototype, but This()         \
+        // returns the actual instance. We need to validate This() for security, but use           \
+        // HolderV2() wouldn't work here since it's the prototype object.                          \
+        // Note: This still uses the deprecated This() API - see V8 bug 455600234.                 \
         auto obj = info.This();                                                                    \
         auto& js = Lock::from(isolate);                                                            \
         auto& wrapper = TypeWrapper::from(isolate);                                                \
         /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */   \
+        /* Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where */    \
+        /* a native object is used as a prototype and accessed through a derived object. */        \
         if (!isContext &&                                                                          \
-            !wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj)) {           \
+            (!wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj) ||          \
+             obj != info.This())) {                                                                \
           throwTypeError(isolate, kIllegalInvocation);                                             \
         }                                                                                          \
         auto& self = extractInternalPointer<T, isContext>(context, obj);                           \
@@ -689,8 +696,11 @@ struct GetterCallback;
         auto obj = info.This();                                                                    \
         auto& wrapper = TypeWrapper::from(isolate);                                                \
         /* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */   \
+        /* Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where */    \
+        /* a native object is used as a prototype and accessed through a derived object. */        \
         if (!isContext &&                                                                          \
-            !wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj)) {           \
+            (!wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj) ||          \
+             obj != info.This())) {                                                                \
           throwTypeError(isolate, kIllegalInvocation);                                             \
         }                                                                                          \
         auto& self = extractInternalPointer<T, isContext>(context, obj);                           \
@@ -889,7 +899,11 @@ struct SetterCallback<TypeWrapper, methodName, void (T::*)(Arg), method, isConte
       auto obj = info.This();
       auto& wrapper = TypeWrapper::from(isolate);
       // V8 no longer supports AccessorSignature, so we must manually verify `this`'s type.
-      if (!isContext && !wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj)) {
+      // Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where
+      // a native object is used as a prototype and accessed through a derived object.
+      if (!isContext &&
+          (!wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj) ||
+              obj != info.This())) {
         throwTypeError(isolate, kIllegalInvocation);
       }
       auto& self = extractInternalPointer<T, isContext>(context, obj);
@@ -915,7 +929,11 @@ struct SetterCallback<TypeWrapper, methodName, void (T::*)(Lock&, Arg), method,
       auto obj = info.This();
       auto& wrapper = TypeWrapper::from(isolate);
       // V8 no longer supports AccessorSignature, so we must manually verify `this`'s type.
-      if (!isContext && !wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj)) {
+      // Also check that HolderV2() == This() to prevent "prototype-as-this" attacks where
+      // a native object is used as a prototype and accessed through a derived object.
+      if (!isContext &&
+          (!wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj) ||
+              obj != info.This())) {
         throwTypeError(isolate, kIllegalInvocation);
       }
       auto& self = extractInternalPointer<T, isContext>(context, obj);
@@ -1199,7 +1217,9 @@ struct WildcardPropertyCallbacks<TypeWrapper,
     liftKj(info, [&]() -> v8::Local<v8::Value> {
       auto isolate = info.GetIsolate();
       auto context = isolate->GetCurrentContext();
-      auto obj = info.This();
+      // With the interceptor on the instance template, HolderV2() returns the instance
+      // (same as This()), so we can use the non-deprecated HolderV2() API.
+      auto obj = info.HolderV2();
       auto& wrapper = TypeWrapper::from(isolate);
       if (!wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj)) {
         throwTypeError(isolate, kIllegalInvocation);
@@ -1249,7 +1269,10 @@ struct ResourceTypeBuilder {
 
   template <typename Type, typename GetNamedMethod, GetNamedMethod getNamedMethod>
   inline void registerWildcardProperty() {
-    prototype->SetHandler(
+    // Install on instance template (not prototype) so that HolderV2() == This().
+    // This matches Chromium's approach and avoids needing the deprecated
+    // PropertyCallbackInfo::This().
+    instance->SetHandler(
         WildcardPropertyCallbacks<TypeWrapper, Type, GetNamedMethod, getNamedMethod>{});
   }
 
@@ -1449,15 +1472,20 @@ struct ResourceTypeBuilder {
 
   template <const char* name, typename Getter, Getter getter>
   inline void registerInspectProperty() {
-    using Gcb = GetterCallback<TypeWrapper, name, Getter, getter, isContext>;
+    // Use PropertyGetterCallback (FunctionCallbackInfo) instead of GetterCallback
+    // (PropertyCallbackInfo) because this property is on the prototype but needs access
+    // to instance state. FunctionCallbackInfo::This() is not deprecated, while
+    // PropertyCallbackInfo::This() is.
+    using Gcb = PropertyGetterCallback<TypeWrapper, name, Getter, getter, isContext>;
 
     auto v8Name = v8StrIntern(isolate, name);
 
     // Create a new unique symbol so this property can only be accessed through `util.inspect()`
     auto symbol = v8::Symbol::New(isolate, v8Name);
     inspectProperties->Set(v8Name, symbol, v8::PropertyAttribute::ReadOnly);
 
-    prototype->SetNativeDataProperty(symbol, &Gcb::callback, nullptr, v8::Local<v8::Value>(),
+    auto getterFn = v8::FunctionTemplate::New(isolate, &Gcb::callback);
+    prototype->SetAccessorProperty(symbol, getterFn, v8::Local<v8::FunctionTemplate>(),
         static_cast<v8::PropertyAttribute>(
             v8::PropertyAttribute::ReadOnly | v8::PropertyAttribute::DontEnum));
   }

v8-this-deprecation-case.md

@@ -0,0 +1,84 @@
+# Case for PropertyCallbackInfo::This()
+
+## Summary
+
+Unfortunately, Cloudflare Workers (workerd) genuinely needs `PropertyCallbackInfo::This()` and cannot follow Chromium's pattern. We tested moving interceptors from prototype to instance template (like Chromium does) and it breaks RPC functionality.
+
+## What We Tested
+
+### Attempt: Move Interceptors to Instance Template
+
+Chromium puts interceptors on `instance_template`, while workerd puts them on `prototype`:
+
+```python
+# Chromium (interface.py line 5815)
+interceptor_template = "${instance_template}"
+```
+
+```cpp
+// Workerd (resource.h)
+prototype->SetHandler(...)  // On prototype, not instance
+```
+
+We tried changing workerd to match Chromium:
+
+```cpp
+instance->SetHandler(...)  // Changed to instance
+```
+
+Result: Our RPC tests fail:
+```
+TypeError: stub.foo is not a function
+TypeError: stub.increment is not a function
+```
+
+### Why It Fails
+
+Workerd uses prototype-level interceptors for RPC stubs. These are wrapper objects that intercept any property access and forward it over RPC:
+
+```javascript
+const stub = env.MY_DURABLE_OBJECT.get(id);
+stub.anyMethodName(args);  // Intercepted and sent over RPC
+```
+
+With interceptor on prototype:
+- Works for any object inheriting from `JsRpcStub.prototype`
+- Enables flexible RPC proxying
+
+With interceptor on instance:
+- Only works for objects created directly from the template
+- Breaks RPC stub pattern
+
+This is fundamentally different from Chromium's use of interceptors (HTMLCollection, Storage, etc.) where the interceptor is on the actual object being accessed.
+
+## Backwards compatibility issue
+
+In addition to the RPC issue we also have a backwards compatibility issue. Workers take
+backwards compatibility very seriously - we can't break already-deployed scripts when
+we run them with a newer version of V8.
+
+In 2022 we moved to setting accessors on the prototype instead of on the instance.
+This solved some compatibility issues, eg. people could not subclass and get the
+expected behaviour. It also aligns better with IDL. However we still have a
+compatibility flag for this since it is trivially observable from JS.
+
+## What Workerd Needs
+
+| Use Case | API Needed | Can Migrate? |
+|----------|-----------|--------------|
+| Prototype properties (new Workers) | `FunctionCallbackInfo::This()` | Already done (2022) |
+| Named interceptors (RPC stubs) | `PropertyCallbackInfo::This()` | **No** - breaks RPC |
+| Instance properties (legacy Workers) | `PropertyCallbackInfo::This()` | **No** - backwards compat |
+
+## Conclusion
+
+`PropertyCallbackInfo::This()` is needed for workerd's legitimate use case of prototype-level interceptors. This is not a case of something that can be fixed by following Chromium's pattern - we tested that and it breaks functionality.
+
+Options:
+1. Keep `This()` available for prototype-level interceptors
+2. Provide alternative API to get the receiver in `PropertyCallbackInfo`
+3. Clarify the migration path for embedders who need prototype-level interceptors
+
+---
+
+*Erik Corry, Cloudflare*