Access Violation in CreateWrapperForManagedObject

[dpwhittaker]

We have another issue showing up. It manifests as an access violation in JsContext::CreateWrapperForManagedObject. objTemplate->NewInstance on line 103 returns null, but this value is not checked for. I have not been able to create a simple repro to cause the issue yet, but wanted to go ahead and log the issue in case it sparked any ideas.

[dpwhittaker]

I am running this in a thread on the threadpool, and it only happens "later" after the V8 has been initialized, So I may be running it on a different thread than it was initialized on. Are there any thread local variables or anything that would prevent the same context from picking up on another thread? If so, I'll try writing a single thread with a producer-consumer queue to feed Execute calls to in our app and see if that resolves the issue.

[prepare]

@dpwhittaker ,

may be running it on a different thread than it was initialized on

1. Yes, V8 engine should run on a single thread. (as nodejs)

I'll try writing a single thread with a producer-consumer queue to feed Execute calls

2. Yes, C# async-await pattern should fit that.

3. for multithread-v8, I need a time to test/implement it.
   Could you post your (fake) code for that ? -> I will test it.

[dpwhittaker]

This is what we came up with for multi-threading and debugging. Basically, node gets its own thread, and the node event loop becomes the message pump for Javascript code. We needed to keep the node event loop running to handle the debugger protocol. In this class, the Execute function can be called with any thread, and it will put the work on the queue, wait for completion, then return the results. We still need to prove out the performance of this approach... we may end up tweaking the MainLoop to work on a 1 or 10 millisecond interval rather than SetImmediate.

    public class ScriptRuntime
    {
        private JsEngine _engine;
        private JsContext _context;
        private readonly Thread _jsThread;
        private readonly ConcurrentQueue<System.Action> workQueue = new ConcurrentQueue<System.Action>();
        public ScriptRuntime()
        {
            _jsThread = new Thread(ScriptThread);
            _jsThread.Start();
        }

        private void ScriptThread(object obj)
        {
            workQueue.Enqueue(InitializeJsGlobals);
            JsEngine.RunJsEngine(Debugger.IsAttached ? new string[] { "--inspect", "hello.espr" } : new string[] { "hello.espr" },
            (IntPtr nativeEngine, IntPtr nativeContext) =>
            {
                _engine = new JsEngine(nativeEngine);
                _context = _engine.CreateContext(nativeContext);
                JsTypeDefinition jstypedef = new JsTypeDefinition("LibEspressoClass");
                jstypedef.AddMember(new JsMethodDefinition("LoadMainSrcFile", args =>
                {
                    args.SetResult(@"
function MainLoop() {
    LibEspresso.Next();
    setImmediate(MainLoop);
}
MainLoop();");
                }));
                jstypedef.AddMember(new JsMethodDefinition("Next", args =>
                {
                    System.Action work;
                    if (workQueue.TryDequeue(out work)) work();
                }));
                _context.RegisterTypeDefinition(jstypedef);
                _context.SetVariableFromAny("LibEspresso", _context.CreateWrapper(new object(), jstypedef));
            });
        }

        private void DoWorkAndWait(System.Action work)
        {
            SemaphoreSlim wait = new SemaphoreSlim(0, 1);
            workQueue.Enqueue(() =>
            {
                work();
                wait.Release();
            });
            wait.Wait();
        }

        public object Execute(string script, Dictionary<string, object> ProcessData)
        {
            object result = null;
            JsException exception = null;
            DoWorkAndWait(() =>
            {
                foreach (string parameter in ProcessData.Keys)
                    _context.SetVariableFromAny(parameter, ProcessData[parameter]);
                // Execute the function itself
                try
                {
                    result = _context.Execute(script);
                }
                catch (JsException ex)
                {
                    exception = ex;
                }
            });
            if (exception != null)
                throw exception;
            return result;
        }
    }

[prepare]

@dpwhittaker

👍

Let me try that code
:)

[dpwhittaker]

We are still running into some weird issues. When node is built in debug mode we get the error mentioned above. When it is not built in debug, the error shows up later. I presume these DCHECK calls only happen in debug mode, so it's probably just catching the same problem earlier.

[prepare]

@dpwhittaker

---

All these are right ?

1. you want only 1 node engine in its own worker thread
2. then send to script to that thread
3. worker thread receive the code, put it in the worker queue
4. when the script is executed and return => notify back

[prepare]

@dpwhittaker

from your example code + with above requirements.

please consider this => 2d1281e

I provide 3 examples

---

test 1: exec on main thread

---

test 2: exec from thread pool, from pic, please note .. ok98->99->0->1

---

test 3: async-await example, (I remove original SemaphoreSlim wait)

---

ScriptRuntime3 code

public class ScriptRuntime3
    {
        private JsEngine _engine;
        private JsContext _context;
        private readonly Thread _jsThread;
        private readonly ConcurrentQueue<System.Action> workQueue = new ConcurrentQueue<System.Action>();
        public ScriptRuntime3()
        {
            _jsThread = new Thread(ScriptThread);
            _jsThread.Start();
        }
        private void ScriptThread(object obj)
        {
            workQueue.Enqueue(InitializeJsGlobals);
            JsEngine.RunJsEngine(Debugger.IsAttached ? new string[] { "--inspect", "hello.espr" } : new string[] { "hello.espr" },
            (IntPtr nativeEngine, IntPtr nativeContext) =>
            {
                _engine = new JsEngine(nativeEngine);
                _context = _engine.CreateContext(nativeContext);

                JsTypeDefinition jstypedef = new JsTypeDefinition("LibEspressoClass");
                jstypedef.AddMember(new JsMethodDefinition("LoadMainSrcFile", args =>
                {
                    args.SetResult(@"
function MainLoop() {
    LibEspresso.Next();
    setImmediate(MainLoop);
}
MainLoop();");
                }));

                jstypedef.AddMember(new JsMethodDefinition("Log", args =>
                {
                    Console.WriteLine(args.GetArgAsObject(0));
                }));

                jstypedef.AddMember(new JsMethodDefinition("Next", args =>
                {
                    //call from js server
                    System.Action work;
                    if (workQueue.TryDequeue(out work))
                    {
                        work();
                    }

                }));
                _context.RegisterTypeDefinition(jstypedef);
                _context.SetVariableFromAny("LibEspresso", _context.CreateWrapper(new object(), jstypedef));
            });
        }
        public void Execute(string script, Dictionary<string, object> processData, Action<object> doneWithResult)
        {
            workQueue.Enqueue(() =>
            {
                foreach (var kp in processData)
                    _context.SetVariableFromAny(kp.Key, kp.Value);
                //----------------          
                object result = null;
                try
                {
                    result = _context.Execute(script);
                }
                catch (JsException ex)
                {
                    //set result as exception
                    result = ex;
                }
                //--------
                //notify result back
                doneWithResult(result);
            });
        }
       public Task<object> ExecuteAsync(string script, Dictionary<string, object> processData)
       {
             var tcs = new TaskCompletionSource<object>(); 
           Execute(script, processData, result =>
              {
                tcs.SetResult(result);
             }); 
             return tcs.Task;
          }

        void InitializeJsGlobals()
        { 
            IntPtr intptr = LoadLibrary(libEspr);
            int errCode = GetLastError();
            int libesprVer = JsBridge.LibVersion;
#if DEBUG
            JsBridge.dbugTestCallbacks();
#endif
        }

        [System.Runtime.InteropServices.DllImport("Kernel32.dll")]
        static extern IntPtr LoadLibrary(string dllname);
        [System.Runtime.InteropServices.DllImport("Kernel32.dll")]
        static extern int GetLastError();
    }

[prepare]

Still has some errors?

[prepare]

(clean up)
see=>
2d1281e

[dpwhittaker]

Here is the stack trace from the error we're seeing:

#
# Fatal error in C:\Projects\node-v8.4.0\deps\v8\src/api.h, line 349
# Check failed: allow_empty_handle || that != 0.
#

==== C stack trace ===============================

        v8::base::debug::StackTrace::StackTrace [0x000007FEB0013EC6+54] (c:\projects\node-v8.4.0\deps\v8\src\base\debug\stack_trace_win.cc:177)
        v8::platform::`anonymous namespace'::PrintStackTrace [0x000007FEAEDC70F2+34] (c:\projects\node-v8.4.0\deps\v8\src\libplatform\default-platform.cc:25)
        V8_Fatal [0x000007FEB0010EE9+169] (c:\projects\node-v8.4.0\deps\v8\src\base\logging.cc:73)
        v8::Utils::OpenHandle [0x000007FEAF248938+88] (c:\projects\node-v8.4.0\deps\v8\src\api.h:349)
        v8::Object::SetInternalField [0x000007FEAF411DCF+63] (c:\projects\node-v8.4.0\deps\v8\src\api.cc:6215)
        JsContext::CreateWrapperForManagedObject [0x000007FEAEBF6381+577] (c:\projects\node-v8.4.0\src\libespresso\bridge2_impl.cpp:109)
        CreateWrapperForManagedObject [0x000007FEAEBF65BA+58] (c:\projects\node-v8.4.0\src\libespresso\bridge2_impl.cpp:116)
        (No symbol) [0x000007FE5ED1310A]

After tracing through v8 code for a while trying to figure out what I am missing, I finally found the error and made a repro:

        public class Base { string _id { get; set; } = "id"; }
        public class A<T> where T : Base
        {
            public void Add(T a) { }
            public void Add(Base a) { }
            public void Add(object a) { }
        }
        public class B : Base { string data { get; set; } = "data"; }

        [Test("4", "TestOverload")]
        static void TestOverload()
        {


#if DEBUG
            JsBridge.dbugTestCallbacks();
#endif
            //create js engine and context

            using (JsEngine engine = new JsEngine())
            using (JsContext ctx = engine.CreateContext())
            {
                GC.Collect();
                System.Diagnostics.Stopwatch stwatch = new System.Diagnostics.Stopwatch();
                stwatch.Start();

                A<B> a = new A<B>();
                ctx.SetVariableFromAny("a", a);
                ctx.SetVariableFromAny("b", new B());
                object result = ctx.Execute("(function(){a.Add(b);})()");
                // if we get to here, we haven't thrown the exception

                stwatch.Stop();
                Console.WriteLine("result " + result);
                Console.WriteLine("met2 managed reflection:" + stwatch.ElapsedMilliseconds.ToString());

            }
        }

The problem is the overloaded methods. It tries to add the same name multiple times. V8 responds differently depending on whether it is built in Debug or Release mode. In Debug Mode, it catches the duplicate in line 106 of api-natives.cc. The error is swallowed upstream causing objTemplate->NewInstance() on line 103 of bridge2_impl.cpp to return a null instance, eventually throwing the stack trace above. In release mode it doesn't throw an error with this simple repro, but it does eventually cause errors in our production code - the javascript proxy object is not null, but the earlier versions of the Add method aren't there or something... it's hard to tell what's going on in the native side from a release build.

Either way, it looks like the right solution here is to have JsMethodDefinition represent all the potential overloads of a single method, so the JS side just sees a single Add method, but the JsMethodDefinition.InvokeMethod determines which overload to call. If you have a better idea, please share. If not, I'll start working on that solution soon (possibly tonight, my team is waiting for this fix) if you can't get to it sooner.

[prepare]

Thank you, I will test it :)

[prepare]

I define above problem is
"How to select the best method overload from js args"

[dpwhittaker]

I agree with that summary statement. For now I just turned the List into a Dictionary<string,JsMethodDefinition>. This ends up just using the last defined overload, which may or may not work, but it fixes our app for now. I still think it's worth finding a matching method and executing it. Type.GetMethod seems to do that for you.

One more issue I've identified. When there are more than 127 property accessors in the sytem, the native side starts looking for negative mIndexes in DoGetterProperty and DoSetterProperty. Turned out this was due to the BinaryStreamReader decoding bytes as chars instead of unsigned chars. I switched all the chars to unsigned chars and the problem went away.

[prepare]

I'm fixing 'How to select the best method overload from js args'
early preview will release soon

[prepare]

@dpwhittaker

see preview code and detail in => b27cfe4

---

pic 1: compare between pure .net (blue box) and Espresso (red box), correct method selection

THIS IS EARLY PREVIEW

If we have a group of overload method,
the TypeBuilder will create a new 'Wrapper' version of that method group.
and send the wrapper version to js side.

When js call back , It call to wrapper method.
and the wrapper method select best overload method
based on argument count=> then compare each arg type.

KNOWN LIMITATION in this version

1. performance ( runtime type check on overload method).
2. correctness of best method selection

PLAN:

1. review TODO: in the code

2. review Roslyn => How to select best method overload

3. use 'Hint' / 'Cache'

4. or+- auto generate unique js version of the each overload
   for user if they want to directly call to specific method( and bypass
   runtime overload method selection)
   ( eg Add$1(), Add$2(), Add$3())

Welcome all comments/ suggestions