Fix ES module deserialization (#942)

A module's imports are not serialized along with the module itself and
that left the deserialized module with dangling references. Fix that by
checking the module cache first, the module loader second.

A glaring problem with cache checking is that the cached module doesn't
have to be the module it was at the time of serialization.

Why not call out to the module loader right away? Because then a module
can get loaded twice and that's arguably even worse.

The alternative of serializing modules transitively doesn't work for
C modules and is also prone to loading the same module twice.

Fixes: #913

Author: bnoordhuis

api-test.c

@@ -3,6 +3,7 @@
 #endif
 #include <assert.h>
 #include <stdlib.h>
+#include <string.h>
 #include "quickjs.h"
 
 #define MAX_TIME 10
@@ -180,12 +181,66 @@ static void is_array(void)
     JS_FreeRuntime(rt);
 }
 
+static int loader_calls;
+
+static JSModuleDef *loader(JSContext *ctx, const char *name, void *opaque)
+{
+    loader_calls++;
+    assert(!strcmp(name, "b"));
+    static const char code[] = "export function f(x){}";
+    JSValue ret = JS_Eval(ctx, code, strlen(code), "b",
+                          JS_EVAL_TYPE_MODULE|JS_EVAL_FLAG_COMPILE_ONLY);
+    assert(!JS_IsException(ret));
+    JSModuleDef *m = JS_VALUE_GET_PTR(ret);
+    assert(m);
+    JS_FreeValue(ctx, ret);
+    return m;
+}
+
+static void module_serde(void)
+{
+    JSRuntime *rt = JS_NewRuntime();
+    JS_SetDumpFlags(rt, JS_DUMP_MODULE_RESOLVE);
+    JS_SetModuleLoaderFunc(rt, NULL, loader, NULL);
+    JSContext *ctx = JS_NewContext(rt);
+    static const char code[] = "import {f} from 'b'; f()";
+    assert(loader_calls == 0);
+    JSValue mod = JS_Eval(ctx, code, strlen(code), "a",
+                          JS_EVAL_TYPE_MODULE|JS_EVAL_FLAG_COMPILE_ONLY);
+    assert(loader_calls == 1);
+    assert(!JS_IsException(mod));
+    assert(JS_IsModule(mod));
+    size_t len = 0;
+    uint8_t *buf = JS_WriteObject(ctx, &len, mod,
+                                  JS_WRITE_OBJ_BYTECODE|JS_WRITE_OBJ_REFERENCE);
+    assert(buf);
+    assert(len > 0);
+    JS_FreeValue(ctx, mod);
+    assert(loader_calls == 1);
+    mod = JS_ReadObject(ctx, buf, len, JS_READ_OBJ_BYTECODE);
+    free(buf);
+    assert(loader_calls == 1); // 'b' is returned from cache
+    assert(!JS_IsException(mod));
+    JSValue ret = JS_EvalFunction(ctx, mod);
+    assert(!JS_IsException(ret));
+    assert(JS_IsPromise(ret));
+    JSValue result = JS_PromiseResult(ctx, ret);
+    assert(!JS_IsException(result));
+    assert(JS_IsUndefined(result));
+    JS_FreeValue(ctx, result);
+    JS_FreeValue(ctx, ret);
+    JS_FreeValue(ctx, mod);
+    JS_FreeContext(ctx);
+    JS_FreeRuntime(rt);
+}
+
 int main(void)
 {
     sync_call();
     async_call();
     async_call_stack_overflow();
     raw_context_global_var();
     is_array();
+    module_serde();
     return 0;
 }

quickjs.c

@@ -35150,8 +35150,21 @@ static JSValue JS_ReadModule(BCReaderState *s)
             goto fail;
         for(i = 0; i < m->req_module_entries_count; i++) {
             JSReqModuleEntry *rme = &m->req_module_entries[i];
+            JSModuleDef **pm = &rme->module;
             if (bc_get_atom(s, &rme->module_name))
                 goto fail;
+            // Resolves a module either from the cache or by requesting
+            // it from the module loader. From cache is not ideal because
+            // the module may not be the one it was a time of serialization
+            // but directly petitioning the module loader is not correct
+            // either because then the same module can get loaded twice.
+            // JS_WriteModule() does not serialize modules transitively
+            // because that doesn't work for C modules and is also prone
+            // to loading the same JS module twice.
+            *pm = js_host_resolve_imported_module_atom(s->ctx, m->module_name,
+                                                       rme->module_name);
+            if (!*pm)
+                goto fail;
         }
     }
 

quickjs.h

@@ -662,6 +662,11 @@ static inline bool JS_IsObject(JSValue v)
     return JS_VALUE_GET_TAG(v) == JS_TAG_OBJECT;
 }
 
+static inline bool JS_IsModule(JSValue v)
+{
+    return JS_VALUE_GET_TAG(v) == JS_TAG_MODULE;
+}
+
 JS_EXTERN JSValue JS_Throw(JSContext *ctx, JSValue obj);
 JS_EXTERN JSValue JS_GetException(JSContext *ctx);
 JS_EXTERN bool JS_HasException(JSContext *ctx);