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

Instantiation of classes which inherit from V8NativeObject in JS not working #13

Open
Exxenoz opened this issue May 2, 2019 · 3 comments
Labels
enhancement inreview I'm reviewing the enhancement now. If accepted I will set the 'todo' or 'inprogress' labal next. worksasdesigned The feature is working as expected.

Comments

@Exxenoz
Copy link

Exxenoz commented May 2, 2019

The following sample throws a V8ExecutionErrorException in TypeBinder::_BindTypeMembers, because handle.Object in line 1561 of V8Engine_Binding.cs returns null:

[ScriptObject("Person", ScriptMemberSecurity.NoAcccess)]
public class Person : V8NativeObject
{
	[ScriptMember("name", ScriptMemberSecurity.ReadWrite)]
	public string Name { get; set; }

	public Person(string name)
	{
		Name = name;
	}

	[ScriptMember("toString", ScriptMemberSecurity.ReadWrite)]
	public override string ToString()
	{
		return Name;
	}
}
Engine.RegisterType<Person>();
Engine.GlobalObject.SetProperty(typeof(Person));

Engine.Execute
(@"
	function test() {
		var p = new Person('Bruce');
	}

	test();
", "V8.NET", true, 0);

As always, thank you. :-)

@Exxenoz
Copy link
Author

Exxenoz commented May 2, 2019

Another sample does not throw an exception, but does not work as well:

Engine.RegisterType<Person>();
Engine.GlobalObject.SetProperty(typeof(Person));

Person p2 = Engine.CreateObject<Person>();
p2.Name = "Jackie";

Engine.GlobalObject.SetProperty("p2", p2, null, null, ScriptMemberSecurity.Locked);

Engine.Execute
(@"
	function hello() {
		Browser.println(p2.name); // Output: undefined
		if (p2.name == undefined) // True
			Browser.println('undefined'); // Output: undefined
	}

	hello();
", "V8.NET", true, 0);

@rjamesnw
Copy link
Owner

rjamesnw commented May 2, 2019

Inheriting from V8NativeObject and registering it as a type to be used is not supported (yet). Just remove V8NativeObject and use the inject attribute (or better yet, it's faster to just include the engine in the constructor like this: Person(string name, V8Engine engine)).

@rjamesnw rjamesnw closed this as completed May 2, 2019
@rjamesnw rjamesnw added worksasdesigned The feature is working as expected. enhancement todo labels May 2, 2019
@rjamesnw rjamesnw reopened this May 2, 2019
@rjamesnw
Copy link
Owner

rjamesnw commented May 2, 2019

Let me show you what is happening currently (and why what you are doing is not supported yet):

  1. You call new Person('Bruce') to create a new JS object.

  2. The hooks call into the CLR side to create a new managed object binder for that type.

  3. Line 1560 of V8Engine_Binding.cs tries to:
    a) call Activator.CreateInstance(BoundType, convertedArguments) to create a new instance, and
    b) Calls Engine.CreateBinding() to get a binder (an ObjectBinder instance - just CLR bridging hooks) for the new instance.

  4. Engine.CreateBinding() (online 2258) checks if the object is a supported handle object (which it is, since you are inheriting from a handle-based object). That tells the system to use the HANDLE value, and NOT the object itself.

     if (objType == null || obj is IHandleBased)
         return CreateValue(obj, recursive, memberSecurity);
    
  5. That results in a call to this on line 930-931 of V8Engine.cs:

         if (value == null)
             return CreateNullValue();
         else if (value is IHandleBased)
             return ((IHandleBased)value).InternalHandle; // (already a V8.NET value!)
         else ...
    
  6. This scenario is not supported by the binder, which then dies on this line:

         handle = Engine.CreateBinding(Activator.CreateInstance(BoundType, convertedArguments), null, _Recursive, _DefaultMemberSecurity, false);
         handle.Object.Initialize(true, args); <<<<<<< ERROR, Object was never set yet in the
                                                       binding system because of the handle
                                                       detection on step 4.
    

Handles are treated like remote values (which they are), and inheriting from it assumes you want to work with the value on the V8 side, not the object on the managed side.

@rjamesnw rjamesnw added inreview I'm reviewing the enhancement now. If accepted I will set the 'todo' or 'inprogress' labal next. and removed todo labels May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement inreview I'm reviewing the enhancement now. If accepted I will set the 'todo' or 'inprogress' labal next. worksasdesigned The feature is working as expected.
Projects
None yet
Development

No branches or pull requests

2 participants