microsoft / Chakra-Samples

Repository for Chakra JavaScript engine related samples.
MIT License
216 stars 84 forks source link

Add Python sample #34

Closed obastemur closed 8 years ago

cosinusoidally commented 8 years ago

Thanks, this sample worked for me on Ubuntu 14.04.

Unfortunately, as I mentioned in (https://github.com/Microsoft/ChakraCore/issues/1353) I believe there is a serious flaw to this approach. Any JsValueRef will not be kept alive on the C stack, so if a GC occurs they will end up getting collected prematurely. For example, if you change:

chakraCore.JsRunScriptUtf8(script, 0, "", byref(jsResult));

to

chakraCore.JsRunScriptUtf8(script, 0, "", byref(jsResult));
chakraCore.JsCollectGarbage(runtime);

This will cause the sample to segfault as jsResult gets garbage collected since there are no references to it on the C stack.

I'm not sure if there is any way of avoiding this, other than doing something like wrapping every API function in a C function that calls JsAddRef on the returned JsValueRef (and then calling JsRelease from Python when you are done with the JsValueRef).

cosinusoidally commented 8 years ago

Or it could be potentially be done with some creative abuse of CFUNCTYPE from Python ctypes. I'll maybe give that a go when I get a chance.

obastemur commented 8 years ago

I'm not sure if I understood what you mean by C reference to a pointer and GC?

obastemur commented 8 years ago

I believe there is a serious flaw to this approach. Any JsValueRef will not be kept alive on the C stack, so if a GC occurs they will end up getting collected prematurely.

@cosinusoidally Are you mixing C variables to C++ class destruction / scope ?

If you keep a reference to (in the JS land not C) to that return value, it wouldn't be GC'ed.

liminzhu commented 8 years ago

@obastemur can you produce a readme stating the purpose and requirement for the sample similar to what we have for other hello-world samples?

cosinusoidally commented 8 years ago

I believe there is a serious flaw to this approach. Any JsValueRef will not be kept alive on the C stack, so if a GC occurs they will end up getting collected prematurely.

@cosinusoidally Are you mixing C variables to C++ class destruction / scope ?

If you keep a reference to (in the JS land not C) to that return value, it wouldn't be GC'ed.

Sorry I wasn't clear. I was referring to the CharkraCore GC doing conservative stack scanning to locate references to JavaScript objects from the native C stack (https://github.com/Microsoft/ChakraCore/wiki/JavaScript-Runtime-%28JSRT%29-Overview#memory-management). These references are just bare pointers to JsValueRefs. In the case of the Python sample, these pointers will live on the heap, and so will not be seen by the GC.

For example, if you apply the following patch to the C++ sample (https://github.com/Microsoft/Chakra-Samples/blob/master/ChakraCore%20Samples/Hello%20World/Linux_OSX/sample.cpp) you will note that it segfaults:

--- sample.cpp    2016-07-28 23:55:03.362788808 +0000
+++ sample.cpp    2016-08-03 10:23:53.035944763 +0000
@@ -11,6 +11,8 @@

 int main()
 {
+void ** foo;
+foo=(void **)malloc(sizeof(void *));
     JsRuntimeHandle runtime;
     JsContextRef context;
     JsValueRef result;
@@ -30,10 +32,13 @@

     // Run the script.
     JsRunScriptUtf8(script, currentSourceContext++, "", &result);
+foo[0]=result;
+result=nullptr;
+ JsCollectGarbage(runtime);

     // Convert your script result to String in JavaScript; redundant if your script returns a String
     JsValueRef resultJSString;
-    JsConvertValueToString(result, &resultJSString);
+    JsConvertValueToString(foo[0], &resultJSString);

     // Project script result back to C++.
     char *resultSTR;

This crashes because I have overwritten result with a nullptr. foo[0] holds a pointer to the result, but since it is heap allocated the GC does not see it when it scans the stack, and so the JavaScript value gets collected prematurely.

If you then run the sample under gdb, you will observe that the crash goes away if you write the contents of foo[0] back in to result (thereby placing the pointer back on the C stack, and so making it once again visible to the GC):

root@ljw-desktop:~/ChakraCore/Linux_OSX# gdb sample
GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.04) 7.11.1
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from sample...done.
(gdb) break 37
Breakpoint 1 at 0x434cf4: file sample.cpp, line 37.
(gdb) run
Starting program: /root/ChakraCore/Linux_OSX/sample
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, main () at sample.cpp:37
37     JsCollectGarbage(runtime);
(gdb) print result
$1 = (JsValueRef) 0x0
(gdb) continue
Continuing.

Program received signal SIGSEGV, Segmentation fault.
Js::Type::GetLibrary (this=0x0) at /root/ChakraCore/lib/Jsrt/../Runtime/Types/Type.h:48
48            JavascriptLibrary* GetLibrary() const { return javascriptLibrary; }
(gdb) backtrace
#0  Js::Type::GetLibrary (this=0x0) at /root/ChakraCore/lib/Jsrt/../Runtime/Types/Type.h:48
#1  0x000000000058562d in Js::RecyclableObject::GetLibrary (this=0x7ff7f2edcc00) at /root/ChakraCore/lib/Jsrt/../Runtime/Types/RecyclableObject.inl:42
#2  0x00000000005828b5 in Js::RecyclableObject::GetScriptContext (this=0x7ff7f2edcc00) at /root/ChakraCore/lib/Jsrt/../Runtime/Types/RecyclableObject.inl:47
#3  0x000000000057b06f in JsConvertValueToString::$_22::operator() (this=0x7fffffffe078, scriptContext=0x7ffff7046170) at /root/ChakraCore/lib/Jsrt/Jsrt.cpp:1237
#4  0x000000000055bafa in ContextAPIWrapper<true, JsConvertValueToString::$_22> (fn=...) at /root/ChakraCore/lib/Jsrt/JsrtInternal.h:139
#5  0x000000000055b9fd in JsConvertValueToString (value=0x7ff7f2edcc00, result=0x7fffffffe110) at /root/ChakraCore/lib/Jsrt/Jsrt.cpp:1236
#6  0x0000000000434d13 in main () at sample.cpp:41
(gdb) run
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /root/ChakraCore/Linux_OSX/sample
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

Breakpoint 1, main () at sample.cpp:37
37     JsCollectGarbage(runtime);
(gdb) print result
$2 = (JsValueRef) 0x0
(gdb) set result=foo[0]
(gdb) print result
$3 = (JsValueRef) 0x7ff7f2edcc00
(gdb) continue
Continuing.
Result -> Hello world!
[Inferior 1 (process 2658) exited normally]
(gdb)

As you mention, this can sometimes be worked around by making sure there is a reference to the Garbage Collectable thing from JavaScript land (eg in this case by changing the script string to something like "a=(()=>{return \'Hello world!\';})()"). Having said that, with the pervasive use of JsValueRefs throughout the API, I'm not sure if this approach could work in the general case.

obastemur commented 8 years ago

@cosinusoidally I agree and don't agree. Python's C interface has its own issues/flavor yet that doesn't defeat the purpose or how ChakraCore can be reached from Python's C or how shared library needs to be initialized.

I'm unable to see the difference with the sample you provided and the Python case you have pointed out earlier. On both you trigger the conservative stack powered GC hence You actually do the same thing (as result was being GCed due to not having a reference)

There are multiple ways to overcome those based on which platform you are embedding ChakraCore. i.e. JsAddRef -> https://msdn.microsoft.com/en-us/library/dn249659(v=vs.94).aspx

This has been a valuable conversation. Thanks for that.

obastemur commented 8 years ago

@liminzhu review please. @cosinusoidally Thanks for the remarks.

liminzhu commented 8 years ago

Added some nitpicks in README :smile:

obastemur commented 8 years ago

@liminzhu thanks for the review and comments. Updated both sample and readme.

obastemur commented 8 years ago

Added Linux/Windows requirements.