From 7ea7a4c66fc49488b4b030eb320cd5b127cb3a6c Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 05:42:36 +0000 Subject: [PATCH 01/16] fix: `Py_XDECREF` requires the GIL to be held, but it's unavailable after Python finalization --- src/modules/pythonmonkey/pythonmonkey.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc index 597f937b..992a0439 100644 --- a/src/modules/pythonmonkey/pythonmonkey.cc +++ b/src/modules/pythonmonkey/pythonmonkey.cc @@ -303,8 +303,6 @@ PyTypeObject JSObjectItemsProxyType = { }; static void cleanup() { - Py_XDECREF(PythonMonkey_Null); - Py_XDECREF(PythonMonkey_BigInt); delete autoRealm; delete global; if (GLOBAL_CX) JS_DestroyContext(GLOBAL_CX); From 345369637dca9c9e0768c7a669647f7d781a8350 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 07:10:32 +0000 Subject: [PATCH 02/16] fix: accessing a non-existent property on a Python `bytes` in JS land should return `undefined` instead of throwing a Python error --- src/PyBytesProxyHandler.cc | 3 +++ src/PyDictProxyHandler.cc | 2 +- src/PyIterableProxyHandler.cc | 13 ++++++++----- src/PyObjectProxyHandler.cc | 4 ++-- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/PyBytesProxyHandler.cc b/src/PyBytesProxyHandler.cc index 525b0068..304c37c9 100644 --- a/src/PyBytesProxyHandler.cc +++ b/src/PyBytesProxyHandler.cc @@ -415,6 +415,9 @@ bool PyBytesProxyHandler::getOwnPropertyDescriptor( PyObject *attrName = idToKey(cx, id); PyObject *self = JS::GetMaybePtrFromReservedSlot(proxy, PyObjectSlot); PyObject *item = PyObject_GetAttr(self, attrName); + if (!item && PyErr_ExceptionMatches(PyExc_AttributeError)) { + PyErr_Clear(); // clear error, we will be returning undefined in this case + } return handleGetOwnPropertyDescriptor(cx, id, desc, item); } diff --git a/src/PyDictProxyHandler.cc b/src/PyDictProxyHandler.cc index 72d2decf..4295d2c4 100644 --- a/src/PyDictProxyHandler.cc +++ b/src/PyDictProxyHandler.cc @@ -56,7 +56,7 @@ bool PyDictProxyHandler::getOwnPropertyDescriptor( ) const { PyObject *attrName = idToKey(cx, id); PyObject *self = JS::GetMaybePtrFromReservedSlot(proxy, PyObjectSlot); - PyObject *item = PyDict_GetItemWithError(self, attrName); + PyObject *item = PyDict_GetItemWithError(self, attrName); // returns NULL without an exception set if the key wasn’t present. return handleGetOwnPropertyDescriptor(cx, id, desc, item); } diff --git a/src/PyIterableProxyHandler.cc b/src/PyIterableProxyHandler.cc index 5de9ec3a..0a95603f 100644 --- a/src/PyIterableProxyHandler.cc +++ b/src/PyIterableProxyHandler.cc @@ -79,7 +79,7 @@ static bool toPrimitive(JSContext *cx, unsigned argc, JS::Value *vp) { _PyUnicodeWriter writer; _PyUnicodeWriter_Init(&writer); - + PyObject *s = PyObject_Repr(self); if (s == nullptr) { @@ -95,8 +95,8 @@ static bool toPrimitive(JSContext *cx, unsigned argc, JS::Value *vp) { return true; } - PyObject* repr = _PyUnicodeWriter_Finish(&writer); - + PyObject *repr = _PyUnicodeWriter_Finish(&writer); + args.rval().set(jsTypeFactory(cx, repr)); return true; } @@ -262,7 +262,7 @@ bool PyIterableProxyHandler::getOwnPropertyDescriptor( // symbol property if (id.isSymbol()) { JS::RootedSymbol rootedSymbol(cx, id.toSymbol()); - JS::SymbolCode symbolCode = JS::GetSymbolCode(rootedSymbol); + JS::SymbolCode symbolCode = JS::GetSymbolCode(rootedSymbol); if (symbolCode == JS::SymbolCode::iterator) { JSFunction *newFunction = JS_NewFunction(cx, iterable_values, 0, 0, NULL); @@ -275,7 +275,7 @@ bool PyIterableProxyHandler::getOwnPropertyDescriptor( ) )); return true; - } + } else if (symbolCode == JS::SymbolCode::toPrimitive) { JSFunction *newFunction = JS_NewFunction(cx, toPrimitive, 0, 0, nullptr); if (!newFunction) return false; @@ -293,6 +293,9 @@ bool PyIterableProxyHandler::getOwnPropertyDescriptor( PyObject *attrName = idToKey(cx, id); PyObject *self = JS::GetMaybePtrFromReservedSlot(proxy, PyObjectSlot); PyObject *item = PyObject_GetAttr(self, attrName); + if (!item && PyErr_ExceptionMatches(PyExc_AttributeError)) { + PyErr_Clear(); // clear error, we will be returning undefined in this case + } return handleGetOwnPropertyDescriptor(cx, id, desc, item); } \ No newline at end of file diff --git a/src/PyObjectProxyHandler.cc b/src/PyObjectProxyHandler.cc index 7f5e4b70..0671774c 100644 --- a/src/PyObjectProxyHandler.cc +++ b/src/PyObjectProxyHandler.cc @@ -143,8 +143,8 @@ bool PyObjectProxyHandler::getOwnPropertyDescriptor( PyObject *attrName = idToKey(cx, id); PyObject *self = JS::GetMaybePtrFromReservedSlot(proxy, PyObjectSlot); PyObject *item = PyObject_GetAttr(self, attrName); - if (!item) { // clear error, we will be returning undefined in this case - PyErr_Clear(); + if (!item && PyErr_ExceptionMatches(PyExc_AttributeError)) { + PyErr_Clear(); // clear error, we will be returning undefined in this case } return handleGetOwnPropertyDescriptor(cx, id, desc, item); From dd0aaed14450ddae7044a1d29c393b8b11b32d70 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 07:55:02 +0000 Subject: [PATCH 03/16] fix: clean up SpiderMonkey when the PythonMonkey module gets destroyed We cannot use `Py_AtExit(cleanup);` because the GIL is unavailable after Python finalization, no more Python APIs can be called. --- src/modules/pythonmonkey/pythonmonkey.cc | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc index 992a0439..a9645a27 100644 --- a/src/modules/pythonmonkey/pythonmonkey.cc +++ b/src/modules/pythonmonkey/pythonmonkey.cc @@ -303,12 +303,20 @@ PyTypeObject JSObjectItemsProxyType = { }; static void cleanup() { + // Clean up the PythonMonkey module + Py_XDECREF(PythonMonkey_Null); + Py_XDECREF(PythonMonkey_BigInt); + + // Clean up SpiderMonkey delete autoRealm; delete global; if (GLOBAL_CX) JS_DestroyContext(GLOBAL_CX); delete JOB_QUEUE; JS_ShutDown(); } +static void cleanup(PyObject *) { + cleanup(); +} static PyObject *collect(PyObject *self, PyObject *args) { JS_GC(GLOBAL_CX); @@ -550,7 +558,6 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void) PyErr_SetString(SpiderMonkeyError, "Spidermonkey could not be initialized."); return NULL; } - Py_AtExit(cleanup); GLOBAL_CX = JS_NewContext(JS::DefaultHeapMaxBytes); if (!GLOBAL_CX) { @@ -643,6 +650,16 @@ PyMODINIT_FUNC PyInit_pythonmonkey(void) if (pyModule == NULL) return NULL; + // Clean up SpiderMonkey when the PythonMonkey module gets destroyed (module.___cleanup is GCed) + // The `cleanup` function will be called automatically when this PyCapsule gets GCed + // We cannot use `Py_AtExit(cleanup);` because the GIL is unavailable after Python finalization, no more Python APIs can be called. + PyObject *autoDestructor = PyCapsule_New(&pythonmonkey, NULL, cleanup); + if (PyModule_AddObject(pyModule, "___cleanup", autoDestructor) < 0) { + Py_DECREF(autoDestructor); + Py_DECREF(pyModule); + return NULL; + } + Py_INCREF(&NullType); if (PyModule_AddObject(pyModule, "null", (PyObject *)&NullType) < 0) { Py_DECREF(&NullType); From a0e3b0599496b5173145fbb868c7d1a2b7628803 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 09:24:18 +0000 Subject: [PATCH 04/16] fix: to fix memory corruption, use `PyUnicode_AsUTF8AndSize` with the string size when possible --- src/JSObjectProxy.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/JSObjectProxy.cc b/src/JSObjectProxy.cc index 4ae14af0..de2f933c 100644 --- a/src/JSObjectProxy.cc +++ b/src/JSObjectProxy.cc @@ -36,9 +36,10 @@ JSContext *GLOBAL_CX; /**< pointer to PythonMonkey's JSContext */ bool keyToId(PyObject *key, JS::MutableHandleId idp) { if (PyUnicode_Check(key)) { // key is str type JS::RootedString idString(GLOBAL_CX); - const char *keyStr = PyUnicode_AsUTF8(key); - JS::ConstUTF8CharsZ utf8Chars(keyStr, strlen(keyStr)); - idString.set(JS_NewStringCopyUTF8Z(GLOBAL_CX, utf8Chars)); + Py_ssize_t length; + const char *keyStr = PyUnicode_AsUTF8AndSize(key, &length); + JS::UTF8Chars utf8Chars(keyStr, length); + idString.set(JS_NewStringCopyUTF8N(GLOBAL_CX, utf8Chars)); return JS_StringToId(GLOBAL_CX, idString, idp); } else if (PyLong_Check(key)) { // key is int type uint32_t keyAsInt = PyLong_AsUnsignedLong(key); // TODO raise OverflowError if the value of pylong is out of range for a unsigned long From 667abc301db9f7c5e28e65511cea2f5cf812134e Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 10:15:12 +0000 Subject: [PATCH 05/16] fix: `PyUnicode_AsUTF8` needs a strict Python str object (not a subtype) --- src/modules/pythonmonkey/pythonmonkey.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc index a9645a27..5d8c424b 100644 --- a/src/modules/pythonmonkey/pythonmonkey.cc +++ b/src/modules/pythonmonkey/pythonmonkey.cc @@ -331,7 +331,7 @@ static bool getEvalOption(PyObject *evalOptions, const char *optionName, const c value = PyDict_GetItemString(evalOptions, optionName); } if (value && value != Py_None) { - *s_p = PyUnicode_AsUTF8(value); + *s_p = PyUnicode_AsUTF8(PyUnicode_FromObject(value)); } return value != NULL && value != Py_None; } @@ -449,7 +449,8 @@ static PyObject *eval(PyObject *self, PyObject *args) { #endif if (!getEvalOption(evalOptions, "filename", &s)) { if (filename && PyUnicode_Check(filename)) { - options.setFile(PyUnicode_AsUTF8(filename)); + PyObject *filenameStr = PyUnicode_FromObject(filename); // needs a strict Python str object (not a subtype) + options.setFile(PyUnicode_AsUTF8(filenameStr)); } } /* filename */ } /* fromPythonFrame */ From c804ca920512a9f9f5a7d6c3efc36d26a91df47e Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 10:17:12 +0000 Subject: [PATCH 06/16] fix: to fix memory corruption, use `PyUnicode_AsUTF8AndSize` with the string size when possible --- src/modules/pythonmonkey/pythonmonkey.cc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc index 5d8c424b..b0cfb09a 100644 --- a/src/modules/pythonmonkey/pythonmonkey.cc +++ b/src/modules/pythonmonkey/pythonmonkey.cc @@ -461,8 +461,9 @@ static PyObject *eval(PyObject *self, PyObject *args) { JS::Rooted rval(GLOBAL_CX); if (code) { JS::SourceText source; - const char *codeChars = PyUnicode_AsUTF8(code); - if (!source.init(GLOBAL_CX, codeChars, strlen(codeChars), JS::SourceOwnership::Borrowed)) { + Py_ssize_t codeLength; + const char *codeChars = PyUnicode_AsUTF8AndSize(code, &codeLength); + if (!source.init(GLOBAL_CX, codeChars, codeLength, JS::SourceOwnership::Borrowed)) { setSpiderMonkeyException(GLOBAL_CX); return NULL; } @@ -521,9 +522,10 @@ static PyObject *isCompilableUnit(PyObject *self, PyObject *args) { return NULL; } - const char *bufferUtf8 = PyUnicode_AsUTF8(item); + Py_ssize_t bufferLength; + const char *bufferUtf8 = PyUnicode_AsUTF8AndSize(item, &bufferLength); - if (JS_Utf8BufferIsCompilableUnit(GLOBAL_CX, *global, bufferUtf8, strlen(bufferUtf8))) { + if (JS_Utf8BufferIsCompilableUnit(GLOBAL_CX, *global, bufferUtf8, bufferLength)) { Py_RETURN_TRUE; } else { Py_RETURN_FALSE; From 7c7427410280d6124a965e9dd4fc12b807e1e70f Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 10:30:19 +0000 Subject: [PATCH 07/16] fix: `PyEventLoop`'s destructor should not use any Python API, after the GIL is already handed over to another thread --- src/JobQueue.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/JobQueue.cc b/src/JobQueue.cc index 0ee33b39..6dcb548f 100644 --- a/src/JobQueue.cc +++ b/src/JobQueue.cc @@ -121,6 +121,7 @@ bool sendJobToMainLoop(PyObject *pyFunc) { } loop.enqueue(pyFunc); + loop._loop = nullptr; // the `Py_XDECREF` Python API call in `PyEventLoop`'s destructor will not be accessible once we hand over the GIL by `PyGILState_Release` PyGILState_Release(gstate); return true; } From 73957c02ef459ca28739059c3847bbc698998283 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 10:43:12 +0000 Subject: [PATCH 08/16] fix: `PyUnicodeObject` needs to be well-formed in a debug build of CPython Otherwise a `_PyObject_AssertFailed` error will be raised See: `_PyUnicode_CheckConsistency` https://github.com/python/cpython/blob/v3.11.3/Objects/unicodeobject.c#L594-L600, #L552-L553 --- src/StrType.cc | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/StrType.cc b/src/StrType.cc index 714bed7d..c849072e 100644 --- a/src/StrType.cc +++ b/src/StrType.cc @@ -53,6 +53,16 @@ static bool containsSurrogatePair(const char16_t *chars, size_t length) { return false; } +/** + * @brief check if the Latin-1 encoded `chars` only contain ascii characters + */ +static bool containsOnlyAscii(const JS::Latin1Char *chars, size_t length) { + for (size_t i = 0; i < length; i++) { + if (chars[i] >= 128) return false; + } + return true; +} + /** * @brief creates new UCS4-encoded pyObject string. This must be called by the user if the original JSString contains any surrogate pairs * @@ -134,6 +144,16 @@ PyObject *StrType::proxifyString(JSContext *cx, JS::HandleValue strVal) { PY_UNICODE_OBJECT_WSTR_LENGTH(pyString) = 0; PY_UNICODE_OBJECT_READY(pyString) = 1; #endif + + #ifdef Py_DEBUG + // In a debug build of CPython, it needs to be a well-formed PyUnicodeObject, otherwise a `_PyObject_AssertFailed` error will be raised. + // See: `_PyUnicode_CheckConsistency` https://github.com/python/cpython/blob/v3.11.3/Objects/unicodeobject.c#L594-L600, #L552-L553 + if (containsOnlyAscii(chars, length)) { + PY_UNICODE_OBJECT_STATE(pyString).ascii = 1; + PY_UNICODE_OBJECT_UTF8(pyString) = (char *)chars; // XXX: most APIs (e.g. PyUnicode_AsUTF8) assume this is a \0 terminated string + PY_UNICODE_OBJECT_UTF8_LENGTH(pyString) = length; + } + #endif } else { // utf16 spidermonkey, ucs2 python const char16_t *chars = JS::GetTwoByteLinearStringChars(nogc, lstr); @@ -195,7 +215,7 @@ PyObject *StrType::getPyObject(JSContext *cx, JS::HandleValue str) { const char *StrType::getValue(JSContext *cx, JS::HandleValue str) { PyObject *pyString = proxifyString(cx, str); - const char *value = PyUnicode_AsUTF8(pyString); + const char *value = PyUnicode_AsUTF8(PyUnicode_FromObject(pyString)); Py_DECREF(pyString); return value; } \ No newline at end of file From 5a1390147ceeec00f1052dcd7377318fd4b35c95 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 10:45:25 +0000 Subject: [PATCH 09/16] fix: the code argument to `python.exec`/`eval` needs to be a well-formed string --- python/pythonmonkey/require.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/pythonmonkey/require.py b/python/pythonmonkey/require.py index 09025c49..0326669a 100644 --- a/python/pythonmonkey/require.py +++ b/python/pythonmonkey/require.py @@ -85,8 +85,8 @@ # Python 3.13 dramatically changed how the namespace in `exec`/`eval` works # See https://docs.python.org/3.13/whatsnew/3.13.html#defined-mutation-semantics-for-locals _locals = {} # keep the local variables inside `eval`/`exec` to a dict -globalThis.python.eval = lambda x: eval(x, None, _locals) -globalThis.python.exec = lambda x: exec(x, None, _locals) +globalThis.python.eval = lambda x: eval(str(x)[:], None, _locals) +globalThis.python.exec = lambda x: exec(str(x)[:], None, _locals) globalThis.python.getenv = os.getenv globalThis.python.paths = sys.path From 8e365a7f503977845a9e020e5f7ac922139f0d6e Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 10:58:00 +0000 Subject: [PATCH 10/16] refactor: replace the use of our own roundtrip `StrType::getValue` method with the simpler `JS_EncodeStringToUTF8` SpiderMonkey API --- include/StrType.hh | 2 -- src/ExceptionType.cc | 4 ++-- src/StrType.cc | 7 ------- src/setSpiderMonkeyException.cc | 4 ++-- 4 files changed, 4 insertions(+), 13 deletions(-) diff --git a/include/StrType.hh b/include/StrType.hh index 5e1954c1..d8199bc4 100644 --- a/include/StrType.hh +++ b/include/StrType.hh @@ -36,8 +36,6 @@ public: */ static PyObject *getPyObject(JSContext *cx, JS::HandleValue str); - static const char *getValue(JSContext *cx, JS::HandleValue str); - static PyObject *proxifyString(JSContext *cx, JS::HandleValue str); }; diff --git a/src/ExceptionType.cc b/src/ExceptionType.cc index 46b3743c..e2d69691 100644 --- a/src/ExceptionType.cc +++ b/src/ExceptionType.cc @@ -107,8 +107,8 @@ JSObject *ExceptionType::toJsError(JSContext *cx, PyObject *exceptionValue, PyOb if (stackObj.get()) { JS::RootedString stackStr(cx); JS::BuildStackString(cx, nullptr, stackObj, &stackStr, 2, js::StackFormat::SpiderMonkey); - JS::RootedValue stackStrVal(cx, JS::StringValue(stackStr)); - stackStream << "\nJS Stack Trace:\n" << StrType::getValue(cx, stackStrVal); + JS::UniqueChars stackStrUtf8 = JS_EncodeStringToUTF8(cx, stackStr); + stackStream << "\nJS Stack Trace:\n" << stackStrUtf8.get(); } diff --git a/src/StrType.cc b/src/StrType.cc index c849072e..7df301bd 100644 --- a/src/StrType.cc +++ b/src/StrType.cc @@ -212,10 +212,3 @@ PyObject *StrType::getPyObject(JSContext *cx, JS::HandleValue str) { return proxifyString(cx, str); } - -const char *StrType::getValue(JSContext *cx, JS::HandleValue str) { - PyObject *pyString = proxifyString(cx, str); - const char *value = PyUnicode_AsUTF8(PyUnicode_FromObject(pyString)); - Py_DECREF(pyString); - return value; -} \ No newline at end of file diff --git a/src/setSpiderMonkeyException.cc b/src/setSpiderMonkeyException.cc index d380d87c..6d9d10fa 100644 --- a/src/setSpiderMonkeyException.cc +++ b/src/setSpiderMonkeyException.cc @@ -66,8 +66,8 @@ PyObject *getExceptionString(JSContext *cx, const JS::ExceptionStack &exceptionS if (stackObj.get()) { JS::RootedString stackStr(cx); BuildStackString(cx, nullptr, stackObj, &stackStr, 2, js::StackFormat::SpiderMonkey); - JS::RootedValue stackStrVal(cx, JS::StringValue(stackStr)); - outStrStream << "Stack Trace:\n" << StrType::getValue(cx, stackStrVal); + JS::UniqueChars stackStrUtf8 = JS_EncodeStringToUTF8(cx, stackStr); + outStrStream << "Stack Trace:\n" << stackStrUtf8.get(); } } From 0eeda3535ad96a38e60191be0bbf3c0d9a4ca790 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 11:20:13 +0000 Subject: [PATCH 11/16] fix: properly handle the reference count when doing `list.concat()` `PyList_SetItem` steals the reference, so we must increase the reference count by 1 --- src/PyListProxyHandler.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/PyListProxyHandler.cc b/src/PyListProxyHandler.cc index f03fb67d..e6f17f7f 100644 --- a/src/PyListProxyHandler.cc +++ b/src/PyListProxyHandler.cc @@ -553,8 +553,11 @@ static bool array_concat(JSContext *cx, unsigned argc, JS::Value *vp) { PyObject *result = PyList_New(selfSize); + // Copy items to the new list for (Py_ssize_t index = 0; index < selfSize; index++) { - PyList_SetItem(result, index, PyList_GetItem(self, index)); + PyObject *item = PyList_GetItem(self, index); + Py_INCREF(item); // `PyList_SetItem` steals the reference, so we must increase the reference count by 1 + PyList_SetItem(result, index, item); } unsigned numArgs = args.length(); From c63b60902d18c59232292ab2c49910ce2dbb7ce2 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 11:28:36 +0000 Subject: [PATCH 12/16] perf: simply do a pythonic `result = list[:]` to get a copy of all items to the new list --- src/PyListProxyHandler.cc | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/PyListProxyHandler.cc b/src/PyListProxyHandler.cc index e6f17f7f..4a5cd9dc 100644 --- a/src/PyListProxyHandler.cc +++ b/src/PyListProxyHandler.cc @@ -550,15 +550,7 @@ static bool array_concat(JSContext *cx, unsigned argc, JS::Value *vp) { PyObject *self = JS::GetMaybePtrFromReservedSlot(proxy, PyObjectSlot); Py_ssize_t selfSize = PyList_GET_SIZE(self); - - PyObject *result = PyList_New(selfSize); - - // Copy items to the new list - for (Py_ssize_t index = 0; index < selfSize; index++) { - PyObject *item = PyList_GetItem(self, index); - Py_INCREF(item); // `PyList_SetItem` steals the reference, so we must increase the reference count by 1 - PyList_SetItem(result, index, item); - } + PyObject *result = PyList_GetSlice(self, 0, selfSize); unsigned numArgs = args.length(); JS::RootedValue elementVal(cx); From 9474990cf8c993d804647a69dde230802c453628 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 24 Sep 2024 11:37:00 +0000 Subject: [PATCH 13/16] fix the reference count WIP: I don't know exactly why... --- src/PyListProxyHandler.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/PyListProxyHandler.cc b/src/PyListProxyHandler.cc index 4a5cd9dc..33cb619b 100644 --- a/src/PyListProxyHandler.cc +++ b/src/PyListProxyHandler.cc @@ -436,8 +436,9 @@ static bool array_fill(JSContext *cx, unsigned argc, JS::Value *vp) { } } - if (!setItemCalled) { - Py_DECREF(fillValueItem); + Py_INCREF(fillValueItem); + if (setItemCalled) { + Py_INCREF(fillValueItem); } // return ref to self From dda291633d059d846114376db64eecfc2e2e0f1d Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 1 Oct 2024 19:13:11 +0000 Subject: [PATCH 14/16] fix: reference count for `array_fill` Since each call of `PyList_SetItem` steals a reference (even if its to the same object), we need multiple references to it for it to steal Co-authored-by: Caleb Aikens --- src/PyListProxyHandler.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/PyListProxyHandler.cc b/src/PyListProxyHandler.cc index 33cb619b..72da8d4e 100644 --- a/src/PyListProxyHandler.cc +++ b/src/PyListProxyHandler.cc @@ -428,18 +428,16 @@ static bool array_fill(JSContext *cx, unsigned argc, JS::Value *vp) { JS::RootedValue fillValue(cx, args[0].get()); PyObject *fillValueItem = pyTypeFactory(cx, fillValue); - bool setItemCalled = false; for (int index = actualStart; index < actualEnd; index++) { - setItemCalled = true; + // Since each call of `PyList_SetItem` steals a reference (even if its to the same object), + // We need multiple references to it for it to steal. + Py_INCREF(fillValueItem); if (PyList_SetItem(self, index, fillValueItem) < 0) { return false; } } - Py_INCREF(fillValueItem); - if (setItemCalled) { - Py_INCREF(fillValueItem); - } + Py_DECREF(fillValueItem); // return ref to self args.rval().set(jsTypeFactory(cx, self)); From 5b9deec8ec071d5c86749b99cb413fa6ea5d8ab1 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 1 Oct 2024 19:28:33 +0000 Subject: [PATCH 15/16] set `GLOBAL_CX = null` in the final cleanup function since it's no longer usable/useful after the context is destroyed --- src/modules/pythonmonkey/pythonmonkey.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/modules/pythonmonkey/pythonmonkey.cc b/src/modules/pythonmonkey/pythonmonkey.cc index b0cfb09a..8408b594 100644 --- a/src/modules/pythonmonkey/pythonmonkey.cc +++ b/src/modules/pythonmonkey/pythonmonkey.cc @@ -310,7 +310,10 @@ static void cleanup() { // Clean up SpiderMonkey delete autoRealm; delete global; - if (GLOBAL_CX) JS_DestroyContext(GLOBAL_CX); + if (GLOBAL_CX) { + JS_DestroyContext(GLOBAL_CX); + GLOBAL_CX = nullptr; + } delete JOB_QUEUE; JS_ShutDown(); } From a4762ae50096a777495a40d2228e82e315844654 Mon Sep 17 00:00:00 2001 From: Tom Tang Date: Tue, 1 Oct 2024 21:00:20 +0000 Subject: [PATCH 16/16] fix the reference count for dicts `test_get_default_not_found` in Python 3.11 Something is double-free-ed during the final finalization: in a debug build of Python, `./Include/object.h:602: _Py_NegativeRefcount: Assertion failed: object has negative ref count` In non-debug build of Python, it simply segfaults at the end during finalization. --- src/JSObjectProxy.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/JSObjectProxy.cc b/src/JSObjectProxy.cc index 29512974..e983335b 100644 --- a/src/JSObjectProxy.cc +++ b/src/JSObjectProxy.cc @@ -638,6 +638,7 @@ PyObject *JSObjectProxyMethodDefinitions::JSObjectProxy_get_method(JSObjectProxy PyObject *value = JSObjectProxy_get(self, key); if (value == Py_None) { + Py_INCREF(default_value); value = default_value; }