Skip to content

remove uses of deprecated v8::PropertyCallbackInfo::This() #6003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dcarney-cf wants to merge 7 commits into
base: main
Choose a base branch
from
+26 −24
Open

remove uses of deprecated v8::PropertyCallbackInfo::This() #6003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7235c95
move rpc wildcard interceptors to instance objects
dcarney-cf Feb 2, 2026
8a63008
skip interceptor
dcarney-cf Feb 2, 2026
7b605ec
Add query
dcarney-cf Feb 3, 2026
90620d2
use query to mask interceptors
dcarney-cf Feb 3, 2026
2311aed
remove remaining use cases
dcarney-cf Feb 4, 2026
69a6dd7
stick with the prototype inspect property
dcarney-cf Feb 4, 2026
a20439e
Merge branch 'main' into dcarney/this-deprecation
dcarney-cf Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/workerd/api/actor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ class DurableObject final: public Fetcher {

JSG_READONLY_INSTANCE_PROPERTY(id, getId);
JSG_READONLY_INSTANCE_PROPERTY(name, getName);
// Need to inherit getRpcMethod interceptor since we need it on the holder.
JSG_WILDCARD_PROPERTY(getRpcMethod);

JSG_TS_DEFINE(interface DurableObject {
fetch(request: Request): Response | Promise<Response>;
Expand Down
2 changes: 2 additions & 0 deletions src/workerd/api/export-loopback.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class LoopbackServiceStub: public Fetcher {
JSG_RESOURCE_TYPE(LoopbackServiceStub) {
JSG_INHERIT(Fetcher);
JSG_CALLABLE(call);
// Need to inherit getRpcMethod interceptor since we need it on the holder.
JSG_WILDCARD_PROPERTY(getRpcMethod);

JSG_TS_ROOT();
JSG_TS_OVERRIDE(
Expand Down
5 changes: 2 additions & 3 deletions src/workerd/api/worker-rpc.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,9 +1382,8 @@ class JsRpcTargetBase: public rpc::JsRpcTarget::Server {
} else if (object.isInstanceOf<JsRpcStub>(js) || object.isInstanceOf<Fetcher>(js) ||
(inStub && object.isInstanceOf<JsRpcProperty>(js))) {
// Yes. It's a JsRpcStub or Fetcher. We should allow descending into the stub.
// Note that the wildcard property of a stub is a prototype property, not an instance
// property, so setting allowInstanceProperties = false here gets the behavior we
// want.
// The wildcard property interceptor is on the instance template, so properties
// accessed via the interceptor appear as instance properties.
// TODO(someday): We'll need to support JsRpcPromise here if someday we allow it to
// be serialized.
allowInstanceProperties = false;
Expand Down
6 changes: 3 additions & 3 deletions src/workerd/jsg/jsg-test.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,11 @@ KJ_TEST("can't use builtin as prototype") {
e.expectEval("function JsType() {}\n"
"JsType.prototype = new NumberBox(123);\n"
"new JsType().value",
"throws", kIllegalInvocation);
"number", "123");
Copy link
Contributor Author

@dcarney-cf dcarney-cf Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kentonv there's another observable change here. in theory it shouldn't matter? certainly moving from throw an exception to not is unlikely to break much but just checking

Copy link
Member

@kentonv kentonv Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not too worried about this.

We might also want to add a test that uses Reflect.get() with an explicit receiver (third argument) to verify that the property still ends up being read from the holder and not the receiver somehow.

e.expectEval("function JsType() {}\n"
"JsType.prototype = new ExtendedNumberBox(123, 'foo');\n"
"new JsType().value",
"throws", kIllegalInvocation);
"number", "123");
e.expectEval("function JsType() {}\n"
"JsType.prototype = this;\n"
"new JsType().getContextProperty()",
Expand Down Expand Up @@ -395,7 +395,7 @@ JSG_DECLARE_ISOLATE_TYPE(InterceptIsolate, InterceptContext, InterceptContext::P
KJ_TEST("Named interceptor") {
Evaluator<InterceptContext, InterceptIsolate> e(v8System);
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, 'foo')", "boolean", "false");
Copy link
Collaborator

@jasnell jasnell Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be my only hesitation here as an observable change. I highly doubt it'll actually break anything so consider it non-blocking but it at least warrants pointing out.

Copy link
Contributor Author

@dcarney-cf dcarney-cf Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what i understand, this should have been the behaviour all along. if anyone relies on this, it's a bit crazy, but yes, it's observable and i'm not sure there's much we can do about it

Copy link
Member

@kentonv kentonv Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually this worries me. If someone is checking for the existence of properties in an RPC stub, today they'd get true for everything, but after this change they get false?

I could imagine someone being broken.

Could we solve this by placing an interceptor on the prototype as well, and having that one return true for these queries? (I think we discussed such a thing at the meeting last week.)

Copy link
Contributor Author

@dcarney-cf dcarney-cf Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I believe only one interceptor is supported and a second interceptor up the chain will just be ignored. We were talking about having different portions of the interception on different portions of the chain (like the query in on spot and the getter in another), but I don't think that works here. I'll experiment.

Copy link
Member

@kentonv kentonv Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we distinguish somehow whether the caller called Reflect.has() vs Object.hasOwn()? We really want the former to return true but the latter to return false.

If we can't distinguish that, maybe it wouldn't be too hard to change V8 so that we can?

Copy link
Collaborator

@jasnell jasnell Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Relect.has(...) will hit the has proxy trap, while Object.hasOwn(...) will hit the getOwnPropertyDescriptor() proxy trap... not sure if that helps us here.. but that's one way we can differentiate if we are using Proxy

Copy link
Member

@kentonv kentonv Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if we switch to Proxy then we don't have any problems... but that's a big change, hoping to avoid it. :)

e.expectEval("p = new ProxyImpl; Reflect.has(p, 'bar')", "boolean", "true");
e.expectEval("p = new ProxyImpl; Reflect.has(p, 'baz')", "boolean", "false");
e.expectEval("p = new ProxyImpl; p.abc", "throws", "TypeError: boom");
Expand Down
35 changes: 17 additions & 18 deletions src/workerd/jsg/resource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@
#include <type_traits>
#include <typeindex>

// TODO(soon): Resolve .This() -> .HolderV2() deprecation warnings, then remove this pragma.
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"

namespace std {
inline auto KJ_HASHCODE(const std::type_index& idx) {
// Make std::type_index (which points to std::type_info) usable as a kj::HashMap key.
Expand Down Expand Up @@ -639,7 +635,7 @@ struct GetterCallback;
liftKj(info, [&]() { \
auto isolate = info.GetIsolate(); \
auto context = isolate->GetCurrentContext(); \
auto obj = info.This(); \
auto obj = info.HolderV2(); \
auto& js = Lock::from(isolate); \
auto& wrapper = TypeWrapper::from(isolate); \
/* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \
Expand Down Expand Up @@ -686,7 +682,7 @@ struct GetterCallback;
auto isolate = info.GetIsolate(); \
auto context = isolate->GetCurrentContext(); \
auto& js = Lock::from(isolate); \
auto obj = info.This(); \
auto obj = info.HolderV2(); \
auto& wrapper = TypeWrapper::from(isolate); \
/* V8 no longer supports AccessorSignature, so we must manually verify `this`'s type. */ \
if (!isContext && \
Expand Down Expand Up @@ -884,9 +880,7 @@ struct SetterCallback<TypeWrapper, methodName, void (T::*)(Arg), method, isConte
auto isolate = info.GetIsolate();
auto context = isolate->GetCurrentContext();
auto& js = Lock::from(isolate);
// TODO(soon): resolve .This() -> .HolderV2() deprecation message. When doing so, please
// also remove the "#pragma clang diagnostic ignored "-Wdeprecated-declarations"" above.
auto obj = info.This();
auto obj = info.HolderV2();
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)) {
Expand All @@ -912,7 +906,7 @@ struct SetterCallback<TypeWrapper, methodName, void (T::*)(Lock&, Arg), method,
liftKj(info, [&]() {
auto isolate = info.GetIsolate();
auto context = isolate->GetCurrentContext();
auto obj = info.This();
auto obj = info.HolderV2();
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)) {
Expand Down Expand Up @@ -1182,7 +1176,7 @@ struct WildcardPropertyCallbacks<TypeWrapper,
WildcardPropertyCallbacks()
: v8::NamedPropertyHandlerConfiguration(getter,
nullptr,
nullptr,
query,
nullptr,
nullptr,
nullptr,
Expand All @@ -1193,13 +1187,20 @@ struct WildcardPropertyCallbacks<TypeWrapper,
static_cast<int>(v8::PropertyHandlerFlags::kHasNoSideEffect) |
static_cast<int>(v8::PropertyHandlerFlags::kOnlyInterceptStrings))) {}

// Query callback is needed for V8 to properly handle property creation with correct
// enumerable attributes when the interceptor is on the instance template. Additionally, we want to ensure wildcard properties basically act as phantom properties om the instance. They should not be visible from javascript, only gettable.
static v8::Intercepted query(
v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Integer>& info) {
return v8::Intercepted::kNo;
}

static v8::Intercepted getter(
v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
v8::Intercepted result = v8::Intercepted::kNo;
liftKj(info, [&]() -> v8::Local<v8::Value> {
auto isolate = info.GetIsolate();
auto context = isolate->GetCurrentContext();
auto obj = info.This();
auto obj = info.HolderV2();
auto& wrapper = TypeWrapper::from(isolate);
if (!wrapper.getTemplate(isolate, static_cast<T*>(nullptr))->HasInstance(obj)) {
throwTypeError(isolate, kIllegalInvocation);
Expand Down Expand Up @@ -1249,7 +1250,7 @@ struct ResourceTypeBuilder {

template <typename Type, typename GetNamedMethod, GetNamedMethod getNamedMethod>
inline void registerWildcardProperty() {
prototype->SetHandler(
instance->SetHandler(
WildcardPropertyCallbacks<TypeWrapper, Type, GetNamedMethod, getNamedMethod>{});
}

Expand Down Expand Up @@ -1449,15 +1450,16 @@ struct ResourceTypeBuilder {

template <const char* name, typename Getter, Getter getter>
inline void registerInspectProperty() {
using Gcb = GetterCallback<TypeWrapper, name, Getter, getter, isContext>;
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, {},
static_cast<v8::PropertyAttribute>(
v8::PropertyAttribute::ReadOnly | v8::PropertyAttribute::DontEnum));
}
Expand Down Expand Up @@ -2004,7 +2006,4 @@ class ObjectWrapper {
kj::Maybe<v8::Local<v8::Object>> parentObject) = delete;
};

// TODO(soon): Resolve .This() -> .HolderV2() deprecation warnings, then remove this pragma.
#pragma clang diagnostic pop

} // namespace workerd::jsg
Loading