1
Vote

v8 function wrapper.

description

Hi.
While working with the library I've noticed that there is no support for js callbacks in .NET code. Function objects supplied to .net code (e.g. as method arguments) are converted to empty dictionaries which obviously are not very useful.
So I've created my own wrapper for functions and would like to share with others. May be someone will find it useful too.
Attached are 2 files (.cpp/.h) that describe managed class JavascriptFunction. It holds v8 handle to js function which can be called from .NET code. To use this new class small patch to JavascriptInterop class should be applied (also attached).

file attachments

comments

oliverbock wrote Apr 1, 2013 at 11:00 PM

Your change looks good, but the source code has moved to github. I suggest you add an Issue/pull there.

Ariman wrote Apr 2, 2013 at 1:43 PM

Unfortunately I'm not a Github user.

chillitom wrote Apr 24, 2013 at 10:29 AM

I'll take a look at adding this in shortly, thanks for this.

chillitom wrote May 7, 2013 at 3:07 PM

I've just been reviewing this with a view to adding it to the github code base and I've found a couple of issues which will need to be addressed.
  • JavascriptFunction has no finalizer leading to memory leaks if JavascriptFunction is not disposed
  • Equals and == are implemented but fail to override the implementations in Object
  • JavascriptFunction leaks memory as mFuncHandle is newed but never deleted
  • JavascriptFunction::Call fails to enter the Isolate and Lock maintained by JavascriptContext::Enter
The first three are trivial to fix, the last issue will require a bit of work.

chillitom wrote May 7, 2013 at 4:12 PM

I've opened a pull request on Github with the JavascriptFunction changes and fixes for the first three issues outlined above.

https://github.com/chillitom/Javascript.Net/pull/1

Ariman wrote May 8, 2013 at 7:17 AM

Thanks for the fixes. I've totally forgot about finalizers in cli (not really much of a user).

Would adding line
JavascriptScope scope(JavascriptContext::GetCurrent());
at the beginning of the JavascriptFunction::Call solve the last issue?

chillitom wrote May 8, 2013 at 8:30 PM

That's a good idea, I've given it a try but I'm hitting problems. V8 is complaining that handles cannot be created outside of handle scopes when I try and access the Global object. I've tried opening a HandleScope and even getting the Global object off the JavascriptContext's v8::Context but no joy.

Do you have this code working in the form you initially submitted? I've been unable to get it to behave.

Ariman wrote May 9, 2013 at 7:44 AM

Yes. It was working. Although example code was very simple. I didn't test this class in any complex scenarios (project didn't reach that phase yet). Also I didn't check this in "standalone mode", only as part of existing workflow so may be issue with handle scope wasn't obvious.
Could you share your test code that don't work? I'll try it here.

chillitom wrote May 9, 2013 at 8:23 AM

Cool, the test code is in the file RegressionTests_JavascriptFunction.cs on the Github pull request

AlbertYang wrote May 10, 2013 at 2:27 AM

It's quite interesting! I'm waiting for new progress.

Ariman wrote May 13, 2013 at 10:03 AM

You got extra parentheses in handle scope definition inside Call. It should be
HandleScope handleScope;
not
HandleScope handleScope();
Without them there is no scope error when accessing Global.

However test sample still does not work for me. I have memory access error when accessing mFuncHandle.

Ariman wrote May 13, 2013 at 2:04 PM

OK. I've figured it out. In my initial code usage of Persistent is wrong. In JavascriptFunction constructor code
mFuncHandle = new Persistent<Function>(Handle<Function>::Cast(iFunction));
must be replaced with
mFuncHandle = new Persistent<Function>();
*mFuncHandle = Persistent<Function>::New(Handle<Function>::Cast(iFunction));
Along with removed braces from handleScope as mentioned earlier test works perfectly.
PS. Also you can return JavascriptContext::mContent to protected section (revert JavascriptContext.h changes in pull request).

Ariman wrote May 13, 2013 at 2:17 PM

One more thing. Further testing of the code revealed that finalizer requires JavascriptScope usage inside. Or Dispose fails with message about isolate is null. Fixed method (at least this version worked for me)
JavascriptFunction::!JavascriptFunction()
{
    JavascriptScope scope(mContext);
    
    mFuncHandle->Dispose();
    delete mFuncHandle;
}

chillitom wrote May 13, 2013 at 3:02 PM

nice, I can confirm it's working for me too. I've updated the pull request with these changes, I want to give this another thorough review and then I'll merge it.

Ariman wrote May 16, 2013 at 10:10 AM

Are you sure that SuppressFinalize call is needed in destructor? Theoretically destructor should not be called for objects created with gcnew (which makes them tracked by GC).

chillitom wrote May 16, 2013 at 12:49 PM

My understanding is that destructors on ref classes just implement IDisposable.Dispose() (if you try explicitly adding a Dispose() method to a ref class the compiler will throw an error). Whenever you use stack semantics for ref class object creation you are still just newing up an object on the heap, it's just that the compiler changes the syntax to look like traditional C++ stack objects. When an ref class object using stack semantics goes out of scope its destructor will be called if it implements IDisposable.

Ariman wrote May 17, 2013 at 2:09 PM

Still objects created without gcnew/^ syntax should be in unmanaged heap (not operated by GC). Consequently finalizer should not be called.

Ariman wrote May 17, 2013 at 2:11 PM

My point is that destructor and finalizer are already mutually exclusive. At least how I understand c++/cli.

chillitom wrote May 17, 2013 at 3:28 PM

That's the thing, all ref class objects are always created on the heap. We should also remember these objects are exposed to other CLI languages like C# where the Disposable and Dispose method will be very visible and callable at any time, hence the need to dispose and finalizer. Could actually remove the dispose method and rely on the finalizer alone but I think it might be nice to offer users the opportunity to dispose a little more deterministically, though I don't know whether that makes any sense given that the javascript object won't get free'd until the V8 gc decides to clean up.

Ariman wrote May 17, 2013 at 4:19 PM

No. I don't suggest to remove destructor. I just saying that you don't really need to call SuppressFinalize in it (last line)
System::GC::SuppressFinalize(this);
since if destructor is called then finalizer will not be used anyway and we don't need additional explicit call to GC.