Caleb/fix/this #227
Caleb/fix/this #227
Conversation
…fix usage. This commit introduces some GC segfault issues which must be addressed before merging
…izePyTypeAndGCThing with a FinalizationRegistry
|
Can we refactor to take into account the common code and concept between PyObjectProxyHandler and PyDictProxyHandler? |
|
This pr reopens #58 I didn't put tests in for those as I thought you would keep them fixed...let's make sure to have tests for those. I just added a test for #58, feel free to improve it of course Please also add a test for the #172 initial issue |
|
I can't get the finalization callbacks called even with explicit gc calls Indeed there is no guarantee that it gets called at all: Seems to put into question the FinalizationRegistry solution BUT: it seems we are registering stack-allocated objects instead of Heap ones |
|
JSMethodProxyType is currently not used, never instantiated except in a test Can you document explicitly somehow a good example of where and how to do this? |
src/jsTypeFactory.cc
Outdated
| registerArgs[0].setObject(*jsFuncObject); | ||
| registerArgs[1].setPrivate(object); | ||
| JS::RootedValue ignoredOutVal(GLOBAL_CX); | ||
| JS_CallFunctionName(GLOBAL_CX, *jsFunctionRegistry, "register", registerArgs, &ignoredOutVal); |
There was a problem hiding this comment.
I think it will never be called back because the target to be reclaimed is stack-allocated: jsFuncObject
There was a problem hiding this comment.
done, had to call JS::SetHostCleanupFinalizationRegistryCallback to set the callback that calls the callbacks ... 😵💫
There was a problem hiding this comment.
It still looks like we are registering a stack-allocated object, jsFuncObject
|
Let's have the PR description document in good detail the following: "doing above revealed GC bugs" |
| @@ -210,57 +246,14 @@ PyTypeObject JSObjectItemsProxyType = { | |||
| }; | |||
|
|
|||
| static void cleanup() { | |||
| delete jsFunctionRegistry; | |||
There was a problem hiding this comment.
this block below at line 368 seems outdated or needing update:
// TODO: Find a better way to destroy the root when necessary (when the returned Python object is GCed).
js::ESClass cls = js::ESClass::Other; // placeholder if rval is not a JSObject
if (rval.isObject()) {
JS::GetBuiltinClass(GLOBAL_CX, JS::RootedObject(GLOBAL_CX, &rval.toObject()), &cls);
if (JS_ObjectIsBoundFunction(&rval.toObject())) {
cls = js::ESClass::Function; // In SpiderMonkey 115 ESR, bound function is no longer a JSFunction but a js::BoundFunctionObject.
}
}
bool rvalIsFunction = cls == js::ESClass::Function; // function object
bool rvalIsString = rval->isString() || cls == js::ESClass::String; // string primitive or boxed String object
if (!(rvalIsFunction || rvalIsString)) { // rval may be a JS function or string which must be kept alive.
delete rval;
}
There was a problem hiding this comment.
done for functions, for strings still not handled but thats not within the scope of this MR
There was a problem hiding this comment.
let's please do it now that we are in-context
|
Did you want to uncomment and add several tests from test_arrays.py that I had put in commented-out? |
|
When it comes to: How about we always assume the this can be used and make sure it's there? |
…ive-Network/PythonMonkey into philippe/pyTypeFactory-cleanup
This reverts commit c69ea53.
…e destroying the context
…ory-cleanup Several fixes including: allocate RootedValue for pyTypeFactory call on the stack instead of the heap
This PR:
fixes the
thisbug by:implementing JSFunctionProxy to proxy JSFunctions
implementing JSMethodProxy to act as a method wrapper for JSFunctions (the python user must create a JSMethodProxy manually and add it to an object since it is not possible to programatically determine if the user intended for a given JSFunction call to act as a function or method on the object)
treating JSMethodProxys passed to JavaScript as bound functions
doing above revealed GC bugs related to storing JSObjects on the heap incorrectly, causing the program to crash, so decided to fix those as well in this PR by:
removing memoizePyTypeAndGCThing, PyTypeToGCThing, and handleSharedPythonMonkeyMemory, replaced by:
implementing JSStringProxy, JSFunctionProxy, JSMethodProxy
rooting JSStrings, JSFunctions, and JSObjects directly in the proxying PyObject
PyString refcounts handled with a map referenced by JSExternalStringCallbacks
PyFunction refcounts handled by a FinalizationRegistry
PyDict/List/other Object refcounts handled by Py(Dict/List/Object)ProxyHandler's finalize method (merged-in prior work)
other:
implemented PyObjectProxyHandler to proxy any type of python object not already handled, including user-defined custom types
addresses (but does not fix) #151
closes #124
closes #172
closes #252
closes #253