From 687aa095f3bea5bd0b8efb12d65d3b542b2d4dea Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Mon, 20 Sep 2021 14:51:31 -0700 Subject: [PATCH] Fix bug when function returns destroyed proxy (#1847) --- conftest.py | 19 +++++++++++++++++-- src/core/error_handling.c | 5 +++++ src/core/error_handling.h | 4 ++++ src/core/hiwire.c | 34 ++++++++++++++++++++-------------- src/core/jsproxy.c | 2 +- src/core/python2js.c | 2 ++ src/tests/test_pyodide.py | 18 ++++++++++++++++++ 7 files changed, 67 insertions(+), 17 deletions(-) diff --git a/conftest.py b/conftest.py index 46384f721..e0725fa58 100644 --- a/conftest.py +++ b/conftest.py @@ -226,6 +226,13 @@ class SeleniumWrapper: def get_num_hiwire_keys(self): return self.run_js("return pyodide._module.hiwire.num_keys();") + @property + def force_test_fail(self) -> bool: + return self.run_js("return !!pyodide._module.fail_test;") + + def clear_force_test_fail(self): + self.run_js("pyodide._module.fail_test = false;") + def save_state(self): self.run_js("self.__savedState = pyodide._module.saveState();") @@ -415,14 +422,20 @@ def pytest_runtest_call(item): trace_pyproxies and pytest.mark.skip_refcount_check.mark not in item.own_markers ) - yield from test_wrapper_check_for_memory_leaks( + yield from extra_checks_test_wrapper( selenium, trace_hiwire_refs, trace_pyproxies ) else: yield -def test_wrapper_check_for_memory_leaks(selenium, trace_hiwire_refs, trace_pyproxies): +def extra_checks_test_wrapper(selenium, trace_hiwire_refs, trace_pyproxies): + """Extra conditions for test to pass: + 1. No explicit request for test to fail + 2. No leaked JsRefs + 3. No leaked PyProxys + """ + selenium.clear_force_test_fail() init_num_keys = selenium.get_num_hiwire_keys() if trace_pyproxies: selenium.enable_pyproxy_tracing() @@ -439,6 +452,8 @@ def test_wrapper_check_for_memory_leaks(selenium, trace_hiwire_refs, trace_pypro # get_result (we don't want to override the error message by raising a # different error here.) a.get_result() + if selenium.force_test_fail: + raise Exception("Test failure explicitly requested but no error was raised.") if trace_pyproxies and trace_hiwire_refs: delta_proxies = selenium.get_num_proxies() - init_num_proxies delta_keys = selenium.get_num_hiwire_keys() - init_num_keys diff --git a/src/core/error_handling.c b/src/core/error_handling.c index d96309ec4..907a2911b 100644 --- a/src/core/error_handling.c +++ b/src/core/error_handling.c @@ -67,6 +67,7 @@ fetch_and_normalize_exception(PyObject** type, Py_CLEAR(*type); Py_CLEAR(*value); Py_CLEAR(*traceback); + fail_test(); PyErr_SetString(PyExc_TypeError, "Pyodide internal error: no exception type or value"); PyErr_Fetch(type, value, traceback); @@ -129,6 +130,8 @@ finally: return success; } +EM_JS(void, fail_test, (), { Module.fail_test = true; }) + /** * Calls traceback.format_exception(type, value, traceback) and joins the * resulting list of strings together. @@ -190,6 +193,7 @@ wrap_exception() success = true; finally: if (!success) { + fail_test(); PySys_WriteStderr( "Pyodide: Internal error occurred while formatting traceback:\n"); PyErr_Print(); @@ -279,6 +283,7 @@ EM_JS_NUM(errcode, error_handling_init_js, (), { { constructor() { + Module.fail_test = true; super("If you are seeing this message, an internal Pyodide error has " + "occurred. Please report it to the Pyodide maintainers."); } diff --git a/src/core/error_handling.h b/src/core/error_handling.h index 15b222036..0f52fc114 100644 --- a/src/core/error_handling.h +++ b/src/core/error_handling.h @@ -16,6 +16,10 @@ error_handling_init(); extern PyObject* internal_error; +// If we are in the test suite, ensure that the current test fails. +void +fail_test(); + /** * Raised when conversion between Javascript and Python fails. */ diff --git a/src/core/hiwire.c b/src/core/hiwire.c index 7c855da2b..47e5b963d 100644 --- a/src/core/hiwire.c +++ b/src/core/hiwire.c @@ -106,6 +106,7 @@ EM_JS_NUM(int, hiwire_init, (), { Module.hiwire.get_value = function(idval) { if (!idval) { + Module.fail_test = true; // clang-format off // This might have happened because the error indicator is set. Let's // check. @@ -160,11 +161,16 @@ EM_JS_NUM(int, hiwire_init, (), { return result; }; + // This is factored out primarily for testing purposes. Module.hiwire.isPromise = function(obj) { - // clang-format off - return (!!obj) && typeof obj.then === 'function'; - // clang-format on + try { + // clang-format off + return (!!obj) && typeof obj.then === 'function'; + // clang-format on + } catch (e) { + return false; + } }; /** @@ -323,7 +329,7 @@ EM_JS(void _Py_NO_RETURN, hiwire_throw_error, (JsRef iderr), { throw Module.hiwire.pop_value(iderr); }); -EM_JS_NUM(bool, JsArray_Check, (JsRef idobj), { +EM_JS(bool, JsArray_Check, (JsRef idobj), { let obj = Module.hiwire.get_value(idobj); if (Array.isArray(obj)) { return true; @@ -581,7 +587,7 @@ EM_JS_REF(JsRef, hiwire_construct, (JsRef idobj, JsRef idargs), { return Module.hiwire.new_value(Reflect.construct(jsobj, jsargs)); }); -EM_JS_NUM(bool, hiwire_has_length, (JsRef idobj), { +EM_JS(bool, hiwire_has_length, (JsRef idobj), { let val = Module.hiwire.get_value(idobj); // clang-format off return (typeof val.size === "number") || @@ -602,7 +608,7 @@ EM_JS_NUM(int, hiwire_get_length, (JsRef idobj), { return ERROR_NUM; }); -EM_JS_NUM(bool, hiwire_get_bool, (JsRef idobj), { +EM_JS(bool, hiwire_get_bool, (JsRef idobj), { let val = Module.hiwire.get_value(idobj); // clang-format off if (!val) { @@ -619,22 +625,22 @@ EM_JS_NUM(bool, hiwire_get_bool, (JsRef idobj), { // clang-format on }); -EM_JS_NUM(bool, hiwire_is_pyproxy, (JsRef idobj), { +EM_JS(bool, hiwire_is_pyproxy, (JsRef idobj), { return Module.isPyProxy(Module.hiwire.get_value(idobj)); }); -EM_JS_NUM(bool, hiwire_is_function, (JsRef idobj), { +EM_JS(bool, hiwire_is_function, (JsRef idobj), { // clang-format off return typeof Module.hiwire.get_value(idobj) === 'function'; // clang-format on }); -EM_JS_NUM(bool, hiwire_is_comlink_proxy, (JsRef idobj), { +EM_JS(bool, hiwire_is_comlink_proxy, (JsRef idobj), { let value = Module.hiwire.get_value(idobj); return !!(Module.Comlink && value[Module.Comlink.createEndpoint]); }); -EM_JS_NUM(bool, hiwire_is_error, (JsRef idobj), { +EM_JS(bool, hiwire_is_error, (JsRef idobj), { // From https://stackoverflow.com/a/45496068 let value = Module.hiwire.get_value(idobj); // clang-format off @@ -643,7 +649,7 @@ EM_JS_NUM(bool, hiwire_is_error, (JsRef idobj), { // clang-format on }); -EM_JS_NUM(bool, hiwire_is_promise, (JsRef idobj), { +EM_JS(bool, hiwire_is_promise, (JsRef idobj), { // clang-format off let obj = Module.hiwire.get_value(idobj); return Module.hiwire.isPromise(obj); @@ -671,7 +677,7 @@ EM_JS_REF(char*, hiwire_constructor_name, (JsRef idobj), { }); #define MAKE_OPERATOR(name, op) \ - EM_JS_NUM(bool, hiwire_##name, (JsRef ida, JsRef idb), { \ + EM_JS(bool, hiwire_##name, (JsRef ida, JsRef idb), { \ return !!(Module.hiwire.get_value(ida) op Module.hiwire.get_value(idb)); \ }) @@ -728,14 +734,14 @@ EM_JS_REF(JsRef, JsObject_Values, (JsRef idobj), { return Module.hiwire.new_value(Object.values(jsobj)); }); -EM_JS_NUM(bool, hiwire_is_typedarray, (JsRef idobj), { +EM_JS(bool, hiwire_is_typedarray, (JsRef idobj), { let jsobj = Module.hiwire.get_value(idobj); // clang-format off return ArrayBuffer.isView(jsobj) || jsobj.constructor.name === "ArrayBuffer"; // clang-format on }); -EM_JS_NUM(bool, hiwire_is_on_wasm_heap, (JsRef idobj), { +EM_JS(bool, hiwire_is_on_wasm_heap, (JsRef idobj), { let jsobj = Module.hiwire.get_value(idobj); // clang-format off return jsobj.buffer === Module.HEAPU8.buffer; diff --git a/src/core/jsproxy.c b/src/core/jsproxy.c index 939824ef8..9925136fb 100644 --- a/src/core/jsproxy.c +++ b/src/core/jsproxy.c @@ -1221,7 +1221,7 @@ finally: // If we succeeded and the result was a promise then we destroy the // arguments in async_done_callback instead of here. Otherwise, destroy the // arguments and return value now. - if (hiwire_is_pyproxy(idresult)) { + if (idresult != NULL && hiwire_is_pyproxy(idresult)) { JsArray_Push(proxies, idresult); } destroy_proxies(proxies, diff --git a/src/core/python2js.c b/src/core/python2js.c index e7d334678..31ebad463 100644 --- a/src/core/python2js.c +++ b/src/core/python2js.c @@ -432,6 +432,7 @@ finally: "Conversion from python to javascript failed"); } } else { + fail_test(); PyErr_SetString(internal_error, "Internal error occurred in python2js"); } return NULL; @@ -504,6 +505,7 @@ python2js_with_context(ConversionContext context, PyObject* x) "Conversion from python to javascript failed"); } } else { + fail_test(); PyErr_SetString(internal_error, "Internal error occurred in python2js_with_depth"); } diff --git a/src/tests/test_pyodide.py b/src/tests/test_pyodide.py index d9b2e0330..d9db997b9 100644 --- a/src/tests/test_pyodide.py +++ b/src/tests/test_pyodide.py @@ -487,6 +487,24 @@ def test_create_proxy(selenium): ) +def test_return_destroyed_value(selenium): + selenium.run_js( + """ + self.f = function(x){ return x }; + pyodide.runPython(` + from pyodide import create_proxy, JsException + from js import f + p = create_proxy([]) + p.destroy() + try: + f(p) + except JsException as e: + assert str(e) == "Error: Object has already been destroyed" + `); + """ + ) + + def test_docstrings_a(): from _pyodide.docstring import get_cmeth_docstring, dedent_docstring from pyodide import JsProxy