SetAccessor -> SetNativeDataProperty

[James Snell]

In Cloudflare Workers we just updated to v8 12.6 and spotted a new pending deprecation notice saying to move from using SetAccessor to SetNativeDataProperty. Unfortunately, there appears to possibly to be an issue with that when using a `v8::FunctionTemplate` and setting the accessor property on the `PrototypeTemplate()`

Take for example the following snippet (there are a few utility methods in here from the workerd codebase like check() and `js.str(...)` etc that are just helpers...

      auto tmpl = v8::FunctionTemplate::New(js.v8Isolate, nullptr);
      v8::Local<v8::String> foo = js.str("foo"_kj);

      tmpl->PrototypeTemplate()->SetNativeDataProperty(foo,
        [](v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Value>& info) {
          auto& js = Lock::from(info.GetIsolate());
          info.GetReturnValue().Set(v8::Local<v8::Value>(js.str("foo"_kj)));
        },
        [](v8::Local<v8::Name> name, v8::Local<v8::Value> value,
           const v8::PropertyCallbackInfo<void>& info) {
          KJ_DBG("Setter is called!");
          info.GetReturnValue().Set(value);
        }, v8::Local<v8::Value>(), v8::PropertyAttribute::None);

      v8::Local<v8::Object> obj = check(tmpl->InstanceTemplate()->NewInstance(js.v8Context()));

      // The setter is not called in this case!
      check(obj->Set(js.v8Context(), foo, js.num(1)));

      // The setter IS called in this one
      check(obj->GetPrototype().As<v8::Object>()->Set(js.v8Context(), foo, js.num(2)));

In this case, the Setter configured on the obj prototype is not called when set via `obj->Set(...)`. 

The question is: with the intended move to `SetNativeDataProperty()` as communicated via the pending deprecation notice, is this a bug or is the behavior here intentional? Should setters provided `PrototypeInstance()->SetNativeDataProperty(...)` Just Work? Or am I just doing something wrong here?

[Igor Sheludko]

Hello James,

TL;DR;: the SetAccessor() Api creates data properties that behave differently than expected by JavaScript spec, see https://crbug.com/336325111 for details.

Indeed, if your code is relying on triggering a setter callback installed on the prototype object then the SetNativeDataProperty() does not fit your needs. The right way to go would be to use SetAccessorProperty() Api that'll define a proper accessor property in JavaScript sense and for such a property an attempt to store via a child object would trigger the setter callback.

While we are on the V8 Api changing topic, here's another cleanup project we are working on which might theoretically affect your code: https://crbug.com/333672197.

Regards,

-- Igor

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/da4aa5ef-d4ae-4b35-ae5c-4946cf8f4d49n%40googlegroups.com.

--

Igor Sheludko

Software Engineer

ish...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

    

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[James Snell]

Excellent, thank you Igor. Probably the one suggestion I would make then is to expand either the doc comments around the deprecation notice or update the deprecation notice to reflect that `SetAccessorProperty()` might be more appropriate a replacement than `SetNativeDataProperty()`

Will take a look at how the other change may affect us! Thank you very much

- James