Fix bug when function returns destroyed proxy (#1847)

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

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.");
     }

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.
  */

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;

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,

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");
     }

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