Fix compilation with V8 version >= 11.9.

[MeirShpilraien]

On V8 version 11.9, the GetInternalField API was changed to return a Local<Data> instead of Local<Value>. Modify the code according to this change.

[iddm]

I checked the error last week, it is strange that it happens now because the v8::Data is a parent for v8::Value, and it should work. The static_assert there, IIRC, was checking for the v8::Local<T> (the inner type) to be a sub-class of v8::Data, which v8::Value is.

https://v8.github.io/api/head/classv8_1_1Data.html

[iddm]

Perhaps, this static_assert should be changed to check for the parent class properly:

  template <class S>
  V8_INLINE Local(Local<S> that) : LocalBase<T>(that) {
    /**
     * This check fails when trying to convert between incompatible
     * handles. For example, converting from a Local<String> to a
     * Local<Number>.
     */
    static_assert(std::is_base_of<T, S>::value, "type check");
  }

I will check more thoroughly later.
This check doesn't make sense for me cuz v8::Data may be v8::Value (cuz the latter is a sub-class of v8::Data), and this check checks for the v8::Data being a sub-class of v8::Value, which is invalid, due to their reverse relationship.

Is there any other way to get a local value from local data?

[MeirShpilraien]

The change is that GetInternalField now returns Local<Data> and before it returned Local<Value>. So now the static assert will not work and we need a runtime check.

[iddm]

I was finally able to look at the code properly:

commit 65d8fbecd82df64fe857a112d75ee7628a3d559c (HEAD, tag: 11.6.189.20-pgo, tag: 11.6.189.20, origin/chromium/5845_172, origin/chromium/5845, origin/11.6-lkgr, branch-heads/11.6)
Author: V8 Autoroll <v8-ci-autoroll-builder@chops-service-accounts.iam.gserviceaccount.com>
Date:   Thu Aug 31 11:45:46 2023 -0700

    Version 11.6.189.20

    Change-Id: I1831a15eb14699cb07553b8591ff2fcbd4a2c789
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4833275
    Bot-Commit: v8-ci-autoroll-builder <v8-ci-autoroll-builder@chops-service-accounts.iam.gserviceaccount.com>
    Cr-Commit-Position: refs/branch-heads/11.6@{#35}
    Cr-Branched-From: e29c028f391389a7a60ee37097e3ca9e396d6fa4-refs/heads/11.6.189@{#3}
    Cr-Branched-From: 95cbef20e2aa556a1ea75431a48b36c4de6b9934-refs/heads/main@{#88340}

It has v8::Local<v8::Value at this commit:

// --- Implementation ---

Local<Value> Object::GetInternalField(int index) {
#ifndef V8_ENABLE_CHECKS
  using A = internal::Address;
  using I = internal::Internals;
  A obj = internal::ValueHelper::ValueAsAddress(this);
  // Fast path: If the object is a plain JSObject, which is the common case, we
  // know where to find the internal fields and can return the value directly.
  int instance_type = I::GetInstanceType(obj);
  if (I::CanHaveInternalField(instance_type)) {
    int offset = I::kJSObjectHeaderSize + (I::kEmbedderDataSlotSize * index);
    A value = I::ReadRawField<A>(obj, offset);
#ifdef V8_COMPRESS_POINTERS
    // We read the full pointer value and then decompress it in order to avoid
    // dealing with potential endiannes issues.
    value = I::DecompressTaggedField(obj, static_cast<uint32_t>(value));
#endif

    auto isolate = reinterpret_cast<v8::Isolate*>(
        internal::IsolateFromNeverReadOnlySpaceObject(obj));
    return Local<Value>::New(isolate, value);
  }
#endif
  return SlowGetInternalField(index);
}

File: include/v8-object.h. Isn't this the one?
Also, their own tests:

        v8::Local<v8::Object> b =
            a->GetInternalField(0)->ToObject(context).ToLocalChecked();
        v8::Local<v8::Object> c =
            b->GetInternalField(0)->ToObject(context).ToLocalChecked();

        InternalFieldData* a1 = reinterpret_cast<InternalFieldData*>(
            a->GetAlignedPointerFromInternalField(1));
        v8::Local<v8::Value> a2 = a->GetInternalField(2);

[MeirShpilraien]

@vityafx please look at the main branch of V8. As I already wrote the API was change to return Local<Data> instead of Local<Value>. This is why we need to check if the returned Local<Data> is in fact Local<Value> and if it does we need to cast it.
This is exactly what the PR does if the version has this new API with the change. Otherwise it keeps the code as is.

[iddm]

But Value is always Data because the Data type is the parent for all these classes. We can't store anything that isn't Data; we only store Value there anyway. This is our code, which we control. In their own tests, they do it exactly the same way:

// The fatal error handler would call exit() so this should not be run in
// parallel.
TEST(InternalDataFields) {
  LocalContext env;
  v8::Isolate* isolate = env->GetIsolate();
  v8::HandleScope scope(isolate);

  Local<v8::FunctionTemplate> templ = v8::FunctionTemplate::New(isolate);
  Local<v8::ObjectTemplate> instance_templ = templ->InstanceTemplate();
  instance_templ->SetInternalFieldCount(1);
  Local<v8::Object> obj = templ->GetFunction(env.local())
                              .ToLocalChecked()
                              ->NewInstance(env.local())
                              .ToLocalChecked();
  CHECK_EQ(1, obj->InternalFieldCount());
  Local<v8::Data> data = obj->GetInternalField(0);
  CHECK(data->IsValue() && data.As<v8::Value>()->IsUndefined());
  Local<v8::Private> sym = v8::Private::New(isolate, v8_str("Foo"));
  obj->SetInternalField(0, sym);
  Local<v8::Data> field = obj->GetInternalField(0);
  CHECK(!field->IsValue());
  CHECK(field->IsPrivate());
  CHECK_EQ(sym, field);

As we always store v8::Values there, we are safe that we won't have anything else returned, or it would be a descendant of v8::Data.

[MeirShpilraien]

So first its a test and you do not mind it will crash.
Second when I came with this argument here: #51 (comment)
Where it is impossible to have an isolate without an ID you were against asserting it. Why here your suggestion is the other way around?
My general opinion is that we should assert things that can not happened (even if the V8 api suggest otherwise), but I believe it is more important to be consistent, this is why I chose to check it.

[iddm]

I thought their As and Cast were clueless because I saw them working with just pointers and static_cast. I checked further, and they indeed throw an exception there, so it will fail at runtime if the target type isn't desired. So your way will not throw an exception but will always return something. I find this not good because there may be other types used, and we still will be trying to return a Value. What if we return a v8::Local<v8::Data> from here instead and do the corresponding checks, if needed, in Rust? This way, we will have full control over the types and have the safety. If upstream allows us to store more types than just v8::Value, then we should allow it too; perhaps, we might even want to use one of those someday.

[MeirShpilraien]

@vityafx the goal of this PR is to fix the build issue. We can think about extending the API but this should be done on another PR. I see now that our rust code can not handle NULL return value so I suggest to just us the As<v8::Value>() anyway and it will assert if it fails. We can change the API later on..

[MeirShpilraien]

Issue opened: #59