Skip to content
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.

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

Native property accessor has no internal fields if evaluated under the Inspector #19879

Closed
ACondie opened this issue Apr 8, 2018 · 4 comments
Labels
addons Issues and PRs related to native addons. question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.

Comments

@ACondie
Copy link

ACondie commented Apr 8, 2018

  • Version: v8.x.x
  • Platform: Win10 64-bit DLL
  • Subsystem: inspector

I am working on a native Node addon that exposes a C++ library to JavaScript. The primary reason for choosing Node was support for the inspector protocol and ease of debugging. The native addon works correctly if the script is run without the inspector attached but will fail if the inspector reads from a natively backed Accessor.

When the inspector attempts to read the natively backed Accessor the object passed for this does not have any internal fields. This is a problem as an internal field is used to store the raw C++ pointer the Accessor should read from. This implies that the inspector is working with a clone of the object, though a cursory glance through Node and V8 didn't find any reason for why that might be the case.

I have tried using both Chrome DevTools and Visual Studio as debugging clients and the behaviour is the same.

Is this behaviour expected?

My use case can be demonstrated using the following (slightly simplified) code:

struct MyStruct { int Value; };

// Create a class template and include an internal field for storing the raw C++ pointer
Local<FunctionTemplate> class_function = FunctionTemplate::New(isolate);
class_function->SetClassName(...);
class_function->InstanceTemplate()->SetInternalFieldCount(1);

// Add a read-only accessor to the prototype template, making it available on all instances
Local<ObjectTemplate> prototype_template = class_function->PrototypeTemplate();
Local<String> property_name = ...;
Local<External> as_external = WrapExternal(&MyStruct::Value);
prototype_template->SetAccessor(
    property_name,
    GetPropertyValue,
    nullptr,
    as_external,
    v8::DEFAULT,
    static_cast<PropertyAttribute>(PropertyAttribute::ReadOnly | PropertyAttribute::DontDelete)
);

Where GetPropertyValue is implemented as:

void GetPropertyValue(
    Local<Name> /*name*/,
    const PropertyCallbackInfo<Value>& info
) {
    EscapableHandleScope scope(info.GetIsolate());

    // Get the raw C++ pointer from [this]
    // *** This fails under the inspector!  The InternalFieldCount is always zero!
    MyStruct* cpp_object = reinterpret_cast<MyStruct*>(info.This()->GetAlignedPointerFromInternalField(0));

    // Get the property value and return it
    auto member_pointer = UnwrapExternal<decltype(&MyStruct::Value)>(info.Data().As<External>());
    auto property_value = cpp_object->*member_pointer;
    info.ReturnValue().Set(scope.Escape(ConvertValue(property_value)));
}
@addaleax addaleax added question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency. addons Issues and PRs related to native addons. labels Apr 8, 2018
@addaleax
Copy link
Member

addaleax commented Apr 8, 2018

In this case, you could:

  • Check InternalFieldCount() and adjust behaviour accordingly. Since you’re the one who sets up both the prototype template and the instance template, you control that value for both ones, so you can distinguish between an instance and the prototype in the getter.
  • Set the accessor on the instance template, rather than the prototype template. That’s going to set up the accessor on each instance of your class, rather than the prototype itself – it consumes a little more space because it’s per-object now, but the inspector should be fine with this.
  • You could use v8::AccessorSignature. Node.js ran into this problem a while ago, and what we did to address it is in 1ee3244 – this will make the getter throw an error when called on the wrong kind of object.

@ACondie
Copy link
Author

ACondie commented Apr 8, 2018

Thank you for your fast response!

I've performed a quick test and moving the native Accessor to the instance template does fix the issue with the internal fields being missing. Am I right in understanding that, because the Accessor is on the prototype instance it is being evaluated by the Inspector as part of the prototype object? That would certainly explain the problem.

I think using a combination of your second and third suggestions would be best for my integration as it would prevent bad usage rather than tolerating it. I'll update this ticket once I've done that.

@TimothyGu
Copy link
Member

Am I right in understanding that, because the Accessor is on the prototype instance it is being evaluated by the Inspector as part of the prototype object?

Yes!

As far as I can tell, this is exactly the same as the issue we ran into in Node.js a while ago with process.stdin._handle.__proto__. You can check out a more in-detail analysis of the problem in #17636 (comment).

The problem with @addaleax’s approach of putting the method on the instance (aka Option 2 in the “How do we fix this?” section in the linked issue) is that it can cause nontrivial memory usage increase (see #17636 (comment)). I would recommend going with Option 1 in your own application, which is also what we ended up doing to fix process.stdin._handle.__proto__.

@ACondie
Copy link
Author

ACondie commented Apr 10, 2018

I've spent a little more time looking at this issue. In my use case I want the Inspector to display the value of the native Accessor as that very useful to the developer. This means I cannot use the first option as, if the InternalFieldCount() is zero there is no option other than to fail gracefully. It is also effectively the same as the third option, where supplying a AccessorSignature will cause an error automatically. If I had to choose between implementing the first or third options I would pick the third as it is supported by V8.

In order for the Inspector to display the native Accessor value I must move it to the InstanceTemplate. The increased memory usage is undesirable but there is no viable alternative given the behaviour of the Inspector / debugging client (I'm still not sure which is triggering the behaviour).

So for clarity I took the following steps:

  1. I moved the native Accessor to the InstanceTemplate
  2. I added an AccessorSignature to cause an error if the native Accessor is called on the wrong kind of object

Thank you both for your help!

@ACondie ACondie closed this as completed Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. question Issues that look for answers. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

3 participants