gjs r77 - in trunk: . gjs test/js test/js/modules/mutualImport

[otaylor svn gnome org]

Author: otaylor
Date: Sun Nov  9 17:57:47 2008
New Revision: 77
URL: http://svn.gnome.org/viewvc/gjs?rev=77&view=rev

Log:
Bug 558741 â Mutual imports cause great confusion

gjs/importer.c: Store the module object into the parent namspace
  object *before* we import the object, so that if the module imports
  another module that references it, things work properly.
  If the import fails, delete the module object property.

tests/js/testImporter.js test/js/modules/mutualImport/a.js:
  Add test for mutual imports; also check that a failed import
  is properly removed from the parent namespace object.

Added:
   trunk/test/js/modules/mutualImport/
   trunk/test/js/modules/mutualImport/a.js
   trunk/test/js/modules/mutualImport/b.js
Modified:
   trunk/Makefile-test.am
   trunk/gjs/importer.c
   trunk/test/js/testImporter.js

Modified: trunk/Makefile-test.am
==============================================================================
--- trunk/Makefile-test.am	(original)
+++ trunk/Makefile-test.am	Sun Nov  9 17:57:47 2008
@@ -96,6 +96,8 @@
 	test/js/modules/jsUnit.js		\
 	test/js/modules/alwaysThrows.js         \
 	test/js/modules/foobar.js               \
+ 	test/js/modules/mutualImport/a.js       \
+ 	test/js/modules/mutualImport/b.js       \
 	test/js/modules/subA/.secret.js         \
 	test/js/modules/subA/.hidden/hidden.js  \
 	test/js/modules/subA/foo                \

Modified: trunk/gjs/importer.c
==============================================================================
--- trunk/gjs/importer.c	(original)
+++ trunk/gjs/importer.c	Sun Nov  9 17:57:47 2008
@@ -131,18 +131,15 @@
 }
 
 static JSBool
-finish_import_and_define(JSContext  *context,
-                         JSObject   *obj,
-                         JSObject   *module_obj,
-                         const char *name)
+define_import(JSContext  *context,
+              JSObject   *obj,
+              JSObject   *module_obj,
+              const char *name)
 {
-    if (!finish_import(context, name))
-        return JS_FALSE;
-
     if (!JS_DefineProperty(context, obj,
                            name, OBJECT_TO_JSVAL(module_obj),
                            NULL, NULL,
-                           GJS_MODULE_PROP_FLAGS)) {
+                           GJS_MODULE_PROP_FLAGS & ~JSPROP_PERMANENT)) {
         gjs_debug(GJS_DEBUG_IMPORTER,
                   "Failed to define '%s' in importer",
                   name);
@@ -152,6 +149,59 @@
     return JS_TRUE;
 }
 
+/* Just like define_import, but makes the setting permament.
+ * we do this after the import succesfully completes.
+ */
+static void
+seal_import(JSContext    *context,
+            JSObject   *obj,
+            JSObject   *module_obj,
+            const char *name)
+{
+    if (!JS_DefineProperty(context, obj,
+                           name, OBJECT_TO_JSVAL(module_obj),
+                           NULL, NULL,
+                           GJS_MODULE_PROP_FLAGS)) {
+        gjs_debug(GJS_DEBUG_IMPORTER,
+                  "Failed to permanently define '%s' in importer",
+                  name);
+    }
+}
+
+/* An import failed. Delete the property pointing to the import
+ * from the parent namespace. In complicated situations this might
+ * not be sufficient to get us fully back to a sane state. If:
+ *
+ *  - We import module A
+ *  - module A imports module B
+ *  - module B imports module A, storing a reference to the current
+ *    module A module object
+ *  - module A subsequently throws an exception
+ *
+ * Then module B is left imported, but the imported module B has
+ * a reference to the failed module A module object. To handle this
+ * we could could try to track the entire "import operation" and
+ * roll back *all* modifications made to the namespace objects.
+ * It's not clear that the complexity would be worth the small gain
+ * in robustness. (You can still come up with ways of defeating
+ * the attempt to clean up.)
+ */
+static void
+cancel_import(JSContext  *context,
+              JSObject   *obj,
+              const char *name)
+{
+    gjs_debug(GJS_DEBUG_IMPORTER,
+              "Cleaning up from failed import of '%s'",
+              name);
+
+    if (!JS_DeleteProperty(context, obj, name)) {
+        gjs_debug(GJS_DEBUG_IMPORTER,
+                  "Failed to delete '%s' in importer",
+                  name);
+    }
+}
+
 static JSBool
 import_native_file(JSContext  *context,
                    JSObject   *obj,
@@ -170,7 +220,13 @@
         return JS_FALSE;
     }
 
-    JS_AddRoot(context, &module_obj);
+    /* We store the module object into the parent module before
+     * initializing the module. If the module has the
+     * GJS_NATIVE_SUPPLIES_MODULE_OBJ flag, it will just overwrite
+     * the reference we stored when it initializes.
+     */
+    if (!define_import(context, obj, module_obj, name))
+        return JS_FALSE;
 
     if (!define_meta_properties(context, module_obj, name, obj))
         goto out;
@@ -178,21 +234,16 @@
     if (!gjs_import_native_module(context, module_obj, full_path, &flags))
         goto out;
 
-    if (flags & GJS_NATIVE_SUPPLIES_MODULE_OBJ) {
-        /* In this case module_obj just gets garbage collected,
-         * we don't end up using it.
-         */
-        if (!finish_import(context, name))
-            goto out;
-    } else {
-        if (!finish_import_and_define(context, obj, module_obj, name))
-            goto out;
-    }
+    if (!finish_import(context, name))
+        goto out;
 
     retval = JS_TRUE;
 
  out:
-    JS_RemoveRoot(context, &module_obj);
+    if (!retval)
+        cancel_import(context, obj, name);
+    else if (!(flags & GJS_NATIVE_SUPPLIES_MODULE_OBJ))
+        seal_import(context, obj, module_obj, name);
 
     return retval;
 }
@@ -207,7 +258,8 @@
     gsize script_len;
     JSObject *module_obj;
     GError *error;
-    jsval retval;
+    jsval script_retval;
+    JSBool retval = JS_FALSE;
 
     gjs_debug(GJS_DEBUG_IMPORTER,
               "Importing '%s'", full_path);
@@ -217,9 +269,12 @@
         return JS_FALSE;
     }
 
-    if (!define_meta_properties(context, module_obj, name, obj))
+    if (!define_import(context, obj, module_obj, name))
         return JS_FALSE;
 
+    if (!define_meta_properties(context, module_obj, name, obj))
+        goto out;
+
     script = NULL;
     script_len = 0;
 
@@ -227,7 +282,7 @@
     if (!g_file_get_contents(full_path, &script, &script_len, &error)) {
         gjs_throw(context, "Could not open %s: %s", full_path, error->message);
         g_error_free(error);
-        return JS_FALSE;
+        goto out;
     }
 
     g_assert(script != NULL);
@@ -238,7 +293,7 @@
                            script_len,
                            full_path,
                            1, /* line number */
-                           &retval)) {
+                           &script_retval)) {
         g_free(script);
 
         /* If JSOPTION_DONT_REPORT_UNCAUGHT is set then the exception
@@ -255,15 +310,23 @@
                          "JS_EvaluateScript() returned FALSE but did not set exception");
         }
 
-        return JS_FALSE;
+        goto out;
     }
 
     g_free(script);
 
-    if (!finish_import_and_define(context, obj, module_obj, name))
-        return JS_FALSE;
+    if (!finish_import(context, obj))
+        goto out;
 
-    return JS_TRUE;
+    retval = JS_TRUE;
+
+ out:
+    if (!retval)
+        cancel_import(context, obj, name);
+    else
+        seal_import(context, obj, module_obj, name);
+
+    return retval;
 }
 
 static JSBool

Added: trunk/test/js/modules/mutualImport/a.js
==============================================================================
--- (empty file)
+++ trunk/test/js/modules/mutualImport/a.js	Sun Nov  9 17:57:47 2008
@@ -0,0 +1,16 @@
+const B = imports.mutualImport.b;
+
+let count = 0;
+
+function incrementCount() {
+    count++;
+}
+
+function getCount() {
+    return count;
+}
+
+function getCountViaB() {
+    return B.getCount();
+}
+

Added: trunk/test/js/modules/mutualImport/b.js
==============================================================================
--- (empty file)
+++ trunk/test/js/modules/mutualImport/b.js	Sun Nov  9 17:57:47 2008
@@ -0,0 +1,6 @@
+const A = imports.mutualImport.a;
+
+function getCount() {
+    return A.getCount();
+}
+

Modified: trunk/test/js/testImporter.js
==============================================================================
--- trunk/test/js/testImporter.js	(original)
+++ trunk/test/js/testImporter.js	Sun Nov  9 17:57:47 2008
@@ -5,6 +5,9 @@
     assertRaises(function() { const m = imports.nonexistentModuleName; });
     assertRaises(function() { const m = imports.alwaysThrows; });
 
+    // Try again to make sure that we properly discarded the module object
+    assertRaises(function() { const m = imports.alwaysThrows; });
+
     // Import a non-broken module
     const foobar = imports.foobar;
     assertNotUndefined(foobar);
@@ -13,6 +16,11 @@
     assertEquals(foobar.foo, "This is foo");
     assertEquals(foobar.bar, "This is bar");
 
+    // Check that deleting the import is a no-op (imported properties are
+    // permanent)
+    delete imports.foobar;
+    assert(imports.foobar == foobar);
+
     // check that importing a second time gets the same object
     foobar.somethingElse = "Should remain";
     const foobar2 = imports.foobar;
@@ -85,4 +93,15 @@
     const hidden = imports.subA['.hidden'].hidden;
 }
 
+function testMutualImport() {
+    // We want to check that the copy of the 'a' module imported directly
+    // is the same as the copy that 'b' imports, and that we don't have two
+    // copies because of the A imports B imports A loop.
+
+    let A = imports.mutualImport.a;
+    A.incrementCount();
+    assertEquals(1, A.getCount());
+    assertEquals(1, A.getCountViaB());
+}
+
 gjstestRun();