Replace deprecated SetAccessor with SetAccessorProperty

V8 has deprecated the SetAccessor method. Per the deprecation
message, we're supposed to replace it with SetNativeDataProperty.
Unfortunately, SetNativeDataProperty does not work with setters
specified on the prototype. Doh! Per feedback from the v8 team,
we should use SetAccessorProperty for this case instead.

Unfortunately, SetAccessorProperty is not a drop in replacement
for the deprecated SetAccessor and requires more extensive changes
than simply calling a different method.

src/workerd/api/http-test.js

@@ -90,17 +90,17 @@ export const inspect = {
  assert.strictEqual(util.inspect(url),
 `URL {
  searchParams: URLSearchParams(3) { 'a' => '1', 'a' => '2', 'b' => '3' },
- hash: '',
- search: '?a=1&a=2&b=3',
- pathname: '/path',
- port: '8787',
- hostname: 'placeholder',
- host: 'placeholder:8787',
- password: 'pass',
- username: 'user',
- protocol: 'http:',
+ origin: 'http:https://placeholder:8787',
  href: 'http:https://user:pass@placeholder:8787/path?a=1&a=2&b=3',
- origin: 'http:https://placeholder:8787'
+ protocol: 'http:',
+ username: 'user',
+ password: 'pass',
+ host: 'placeholder:8787',
+ hostname: 'placeholder',
+ port: '8787',
+ pathname: '/path',
+ search: '?a=1&a=2&b=3',
+ hash: ''
 }`
  );
 
@@ -129,7 +129,7 @@ export const inspect = {
  keepalive: false,
  integrity: '',
  cf: undefined,
- signal: AbortSignal { onabort: null, reason: undefined, aborted: false },
+ signal: AbortSignal { reason: undefined, aborted: false, onabort: null },
  fetcher: null,
  redirect: 'follow',
  headers: Headers(1) { 'content-type' => 'text/plain', [immutable]: false },
@@ -178,7 +178,6 @@ export const inspect = {
  assert.strictEqual(event.data,
 `MessageEvent {
  data: 'data',
- cancelBubble: false,
  isTrusted: true,
  timeStamp: 0,
  srcElement: WebSocket { extensions: '', protocol: '', url: null, readyState: 1 },
@@ -190,6 +189,7 @@ export const inspect = {
  composed: false,
  eventPhase: 2,
  type: 'message',
+ cancelBubble: false,
  NONE: 0,
  CAPTURING_PHASE: 1,
  AT_TARGET: 2,

src/workerd/api/url.c++

@@ -186,12 +186,12 @@ URL::URL(kj::Url&& u): url(kj::refcounted<RefcountedUrl>(kj::mv(u))) {
 kj::String URL::getHref() {
  return toString();
 }
-void URL::setHref(const v8::PropertyCallbackInfo<void>& info, kj::String value) {
+void URL::setHref(jsg::Lock& js, kj::String value) {
  KJ_IF_SOME(u, kj::Url::tryParse(kj::mv(value))) {
  url->kj::Url::operator=(kj::mv(u));
  } else {
  auto context = jsg::TypeErrorContext::setterArgument(typeid(URL), "href");
- jsg::throwTypeError(info.GetIsolate(), context, "valid URL string");
+ jsg::throwTypeError(js.v8Isolate, context, "valid URL string");
  // href's is the only setter which is allowed to throw on invalid input, according to the spec.
  }
 }

src/workerd/api/url.h

@@ -25,7 +25,7 @@ class URL: public jsg::Object {
 
  // Href is the only setter that throws. All others ignore errors, leaving their values
  // unchanged.
- void setHref(const v8::PropertyCallbackInfo<void>& info, kj::String value);
+ void setHref(jsg::Lock& js, kj::String value);
 
  kj::String getOrigin();
 

src/workerd/jsg/resource.c++

@@ -77,9 +77,8 @@ void scheduleUnimplementedMethodError(
 }
 
 void scheduleUnimplementedPropertyError(
- const v8::PropertyCallbackInfo<v8::Value>& args,
+ v8::Isolate* isolate,
  const std::type_info& type, const char* propertyName) {
- auto isolate = args.GetIsolate();
  isolate->ThrowError(v8StrIntern(isolate,
  kj::str("Failed to get the '", propertyName, "' property on '", typeName(type),
  "': the property is not implemented.")));

src/workerd/jsg/resource.h

@@ -76,10 +76,9 @@ void scheduleUnimplementedMethodError(
 // Called to throw errors about calling unimplemented functionality. It's assumed these are called
 // directly from the V8 trampoline without liftKj, so they don't throw JsExceptionThrown.
 void scheduleUnimplementedPropertyError(
- const v8::PropertyCallbackInfo<v8::Value>& args,
+ v8::Isolate* isolate,
  const std::type_info& type, const char* propertyName);
 
-
 // Implements the V8 callback function for calling the static `constructor()` method of the C++
 // class.
 template <typename TypeWrapper, typename T,
@@ -449,7 +448,7 @@ struct GetterCallback;
  isContext> { \
  static constexpr bool enumerable = false; \
  static void callback(v8::Local<v8::Name>, const v8::PropertyCallbackInfo<v8::Value>& info) { \
- scheduleUnimplementedPropertyError(info, typeid(T), propertyName); \
+ scheduleUnimplementedPropertyError(info.GetIsolate(), typeid(T), propertyName); \
  } \
  };
 
@@ -458,6 +457,100 @@ JSG_DEFINE_GETTER_CALLBACK_STRUCTS(const)
 
 #undef JSG_DEFINE_GETTER_CALLBACK_STRUCTS
 
+template <typename TypeWrapper, const char* methodName,
+ typename Method, Method method, bool isContext>
+struct PropertyGetterCallback;
+
+#define JSG_DEFINE_PROPERTY_GETTER_CALLBACK_STRUCTS(...) \
+ template <typename TypeWrapper, const char* methodName, typename T, typename Ret, \
+ typename... Args, Ret (T::*method)(Args...) __VA_ARGS__, bool isContext> \
+ struct PropertyGetterCallback<TypeWrapper, methodName, Ret (T::*)(Args...) __VA_ARGS__, \
+ method, isContext> { \
+ static constexpr bool enumerable = true; \
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) { \
+ liftKj(info, [&]() { \
+ auto isolate = info.GetIsolate(); \
+ auto context = isolate->GetCurrentContext(); \
+ 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.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \
+ throwTypeError(isolate, "Illegal invocation"); \
+ } \
+ auto& self = extractInternalPointer<T, isContext>(context, obj); \
+ return wrapper.wrap(context, obj, (self.*method)( \
+ wrapper.unwrap(context, (kj::Decay<Args>*)nullptr)...)); \
+ }); \
+ } \
+ }; \
+ \
+ /* Specialization for methods that take `Lock&` as their first parameter. */ \
+ template <typename TypeWrapper, const char* methodName, typename T, typename Ret, \
+ typename... Args, \
+ Ret (T::*method)(Lock&, Args...) __VA_ARGS__, \
+ bool isContext> \
+ struct PropertyGetterCallback<TypeWrapper, methodName, \
+ Ret (T::*)(Lock&, Args...) __VA_ARGS__, \
+ method, isContext> { \
+ static constexpr bool enumerable = true; \
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) { \
+ liftKj(info, [&]() { \
+ auto isolate = info.GetIsolate(); \
+ auto context = isolate->GetCurrentContext(); \
+ 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.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \
+ throwTypeError(isolate, "Illegal invocation"); \
+ } \
+ auto& self = extractInternalPointer<T, isContext>(context, obj); \
+ return wrapper.wrap(context, obj, (self.*method)(Lock::from(isolate), \
+ wrapper.unwrap(context, (kj::Decay<Args>*)nullptr)...)); \
+ }); \
+ } \
+ }; \
+ \
+ /* Specialization for methods that take `const v8::PropertyCallbackInfo<v8::Value>&` as \
+ * their first parameter. */ \
+ template <typename TypeWrapper, const char* methodName, typename T, typename Ret, \
+ typename... Args, \
+ Ret (T::*method)(const v8::PropertyCallbackInfo<v8::Value>&, Args...) __VA_ARGS__, \
+ bool isContext> \
+ struct PropertyGetterCallback<TypeWrapper, methodName, \
+ Ret (T::*)(const v8::FunctionCallbackInfo<v8::Value>&, Args...) __VA_ARGS__, \
+ method, isContext> { \
+ static constexpr bool enumerable = true; \
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) { \
+ liftKj(info, [&]() { \
+ auto isolate = info.GetIsolate(); \
+ auto context = isolate->GetCurrentContext(); \
+ 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.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) { \
+ throwTypeError(isolate, "Illegal invocation"); \
+ } \
+ auto& self = extractInternalPointer<T, isContext>(context, obj); \
+ return wrapper.wrap(context, obj, (self.*method)(info, \
+ wrapper.unwrap(context, (kj::Decay<Args>*)nullptr)...)); \
+ }); \
+ } \
+ }; \
+ \
+ template <typename TypeWrapper, const char* propertyName, typename T, \
+ Unimplemented (T::*method)() __VA_ARGS__, bool isContext> \
+ struct PropertyGetterCallback<TypeWrapper, propertyName, \
+ Unimplemented (T::*)() __VA_ARGS__, method, isContext> { \
+ static constexpr bool enumerable = false; \
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) { \
+ scheduleUnimplementedPropertyError(info.GetIsolate(), typeid(T), propertyName); \
+ } \
+ };
+
+JSG_DEFINE_PROPERTY_GETTER_CALLBACK_STRUCTS()
+JSG_DEFINE_PROPERTY_GETTER_CALLBACK_STRUCTS(const)
+
+
 // Implements the V8 callback function for calling a property setter method of a C++ class.
 template <typename TypeWrapper, const char* methodName,
  typename Method, Method method, bool isContext>
@@ -531,6 +624,75 @@ struct SetterCallback<TypeWrapper, methodName,
  }
 };
 
+template <typename TypeWrapper, const char* methodName,
+ typename Method, Method method, bool isContext>
+struct PropertySetterCallback;
+
+template <typename TypeWrapper, const char* methodName, typename T, typename Arg,
+ void (T::*method)(Arg), bool isContext>
+struct PropertySetterCallback<TypeWrapper, methodName, void (T::*)(Arg), method, isContext> {
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) {
+ liftKj(info, [&]() {
+ auto isolate = info.GetIsolate();
+ auto context = isolate->GetCurrentContext();
+ 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.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) {
+ throwTypeError(isolate, "Illegal invocation");
+ }
+ auto& self = extractInternalPointer<T, isContext>(context, obj);
+ (self.*method)(wrapper.template unwrap<Arg>(context, info[0],
+ TypeErrorContext::setterArgument(typeid(T), methodName)));
+ });
+ }
+};
+
+// Specialization for methods that take `Lock&` as their first parameter.
+template <typename TypeWrapper, const char* methodName, typename T, typename Arg,
+ void (T::*method)(Lock&, Arg), bool isContext>
+struct PropertySetterCallback<TypeWrapper, methodName,
+ void (T::*)(Lock&, Arg), method, isContext> {
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) {
+ liftKj(info, [&]() {
+ auto isolate = info.GetIsolate();
+ auto context = isolate->GetCurrentContext();
+ 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.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) {
+ throwTypeError(isolate, "Illegal invocation");
+ }
+ auto& self = extractInternalPointer<T, isContext>(context, obj);
+ (self.*method)(Lock::from(isolate), wrapper.template unwrap<Arg>(context, info[0],
+ TypeErrorContext::setterArgument(typeid(T), methodName)));
+ });
+ }
+};
+
+// Specialization for methods that take `const v8::PropertyCallbackInfo<void>&` as their
+// first parameter.
+template <typename TypeWrapper, const char* methodName, typename T, typename Arg,
+ void (T::*method)(const v8::PropertyCallbackInfo<void>&, Arg), bool isContext>
+struct PropertySetterCallback<TypeWrapper, methodName,
+ void (T::*)(const v8::PropertyCallbackInfo<void>&, Arg), method, isContext> {
+ static void callback(const v8::FunctionCallbackInfo<v8::Value>& info) {
+ liftKj(info, [&]() {
+ auto isolate = info.GetIsolate();
+ auto context = isolate->GetCurrentContext();
+ 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.template getTemplate(isolate, (T*)nullptr)->HasInstance(obj)) {
+ throwTypeError(isolate, "Illegal invocation");
+ }
+ auto& self = extractInternalPointer<T, isContext>(context, obj);
+ (self.*method)(info, wrapper.template unwrap<Arg>(context, info[0],
+ TypeErrorContext::setterArgument(typeid(T), methodName)));
+ });
+ }
+};
+
 template <typename T>
 class TypeHandler;
 
@@ -819,18 +981,21 @@ struct ResourceTypeBuilder {
  inline void registerPrototypeProperty() {
  auto v8Name = v8StrIntern(isolate, name);
 
- using Gcb = GetterCallback<TypeWrapper, name, Getter, getter, isContext>;
+ using Gcb = PropertyGetterCallback<TypeWrapper, name, Getter, getter, isContext>;
  if (!Gcb::enumerable) {
  inspectProperties->Set(v8Name, v8::False(isolate), v8::PropertyAttribute::ReadOnly);
  }
 
- // TODO(soon): The SetAccessor method is deprecated. We should switch to SetNativeDataProperty
- // but doing so causes some tests to fail. Needs further investigation.
- prototype->SetAccessor(
- v8Name,
- Gcb::callback,
- &SetterCallback<TypeWrapper, name, Setter, setter, isContext>::callback,
- v8::Local<v8::Value>(),
+ // Note that we cannot use `SetNativeDataProperty(...)` here the way we do with other
+ // properties because it will not properly handle the prototype chain when it comes
+ // to using setters... which is annoying. It means we end up having to use FunctionTemplates
+ // instead of the more convenient property callbacks.
+ auto getterFn = v8::FunctionTemplate::New(isolate, Gcb::callback);
+ auto setterFn = v8::FunctionTemplate::New(isolate,
+ &PropertySetterCallback<TypeWrapper, name, Setter, setter, isContext>::callback);
+
+ prototype->SetAccessorProperty(
+ v8Name, getterFn, setterFn,
  Gcb::enumerable ? v8::PropertyAttribute::None : v8::PropertyAttribute::DontEnum);
  }