Add error handling for dynamic imports to libdeno (#2678)

6 files changed, 183 insertions, 25 deletions

diff --git a/core/libdeno/deno.h b/core/libdeno/deno.h
index 745285554..fe5214848 100644
--- a/core/libdeno/deno.h
+++ b/core/libdeno/deno.h
@@ -36,7 +36,7 @@ typedef void (*deno_recv_cb)(void* user_data, deno_buf control_buf,
 
 typedef int deno_dyn_import_id;
 // Called when dynamic import is called in JS: import('foo')
-// Embedder must call deno_dyn_import() with the specified id and
+// Embedder must call deno_dyn_import_done() with the specified id and
 // the module.
 typedef void (*deno_dyn_import_cb)(void* user_data, const char* specifier,
                                    const char* referrer, deno_dyn_import_id id);
@@ -127,8 +127,12 @@ void deno_mod_evaluate(Deno* d, void* user_data, deno_mod id);
 
 // Call exactly once for every deno_dyn_import_cb.
 // Note this call will execute JS.
-void deno_dyn_import(Deno* d, void* user_data, deno_dyn_import_id id,
-                     deno_mod mod_id);
+// Either mod_id is zero and error_str is not null OR mod_id is valid and
+// error_str is null.
+// TODO(ry) The errors arising from dynamic import are not exactly the same as
+// those arising from ops in Deno.
+void deno_dyn_import_done(Deno* d, void* user_data, deno_dyn_import_id id,
+                          deno_mod mod_id, const char* error_str);
 
 #ifdef __cplusplus
 }  // extern "C"
diff --git a/core/libdeno/exceptions.cc b/core/libdeno/exceptions.cc
index 7484e4eb4..5f4d578b6 100644
--- a/core/libdeno/exceptions.cc
+++ b/core/libdeno/exceptions.cc
@@ -200,6 +200,7 @@ void HandleException(v8::Local<v8::Context> context,
   std::string json_str = EncodeExceptionAsJSON(context, exception);
   CHECK_NOT_NULL(d);
   d->last_exception_ = json_str;
+  d->last_exception_handle_.Reset(isolate, exception);
 }
 
 void HandleExceptionMessage(v8::Local<v8::Context> context,
@@ -218,6 +219,15 @@ void HandleExceptionMessage(v8::Local<v8::Context> context,
   d->last_exception_ = json_str;
 }
 
+void ClearException(v8::Local<v8::Context> context) {
+  v8::Isolate* isolate = context->GetIsolate();
+  DenoIsolate* d = DenoIsolate::FromIsolate(isolate);
+  CHECK_NOT_NULL(d);
+
+  d->last_exception_.clear();
+  d->last_exception_handle_.Reset();
+}
+
 void ThrowInvalidArgument(v8::Isolate* isolate) {
   isolate->ThrowException(v8::Exception::TypeError(v8_str("Invalid Argument")));
 }
diff --git a/core/libdeno/exceptions.h b/core/libdeno/exceptions.h
index 413bcb7ef..2c0947be2 100644
--- a/core/libdeno/exceptions.h
+++ b/core/libdeno/exceptions.h
@@ -19,6 +19,8 @@ void HandleException(v8::Local<v8::Context> context,
 void HandleExceptionMessage(v8::Local<v8::Context> context,
                             v8::Local<v8::Message> message);
 
+void ClearException(v8::Local<v8::Context> context);
+
 void ThrowInvalidArgument(v8::Isolate* isolate);
 }  // namespace deno
 
diff --git a/core/libdeno/internal.h b/core/libdeno/internal.h
index 7702c3a16..50e85017e 100644
--- a/core/libdeno/internal.h
+++ b/core/libdeno/internal.h
@@ -51,6 +51,7 @@ class DenoIsolate {
   }
 
   ~DenoIsolate() {
+    last_exception_handle_.Reset();
     shared_ab_.Reset();
     if (locker_) {
       delete locker_;
@@ -111,6 +112,7 @@ class DenoIsolate {
   v8::Persistent<v8::Context> context_;
   std::map<int, v8::Persistent<v8::Value>> pending_promise_map_;
   std::string last_exception_;
+  v8::Persistent<v8::Value> last_exception_handle_;
   v8::Persistent<v8::Function> recv_;
   v8::StartupData snapshot_;
   v8::Persistent<v8::ArrayBuffer> global_import_buf_;
diff --git a/core/libdeno/modules.cc b/core/libdeno/modules.cc
index 77b330c3e..0d3a9c70f 100644
--- a/core/libdeno/modules.cc
+++ b/core/libdeno/modules.cc
@@ -2,6 +2,7 @@
 #include "exceptions.h"
 #include "internal.h"
 
+using deno::ClearException;
 using deno::DenoIsolate;
 using deno::HandleException;
 using v8::Boolean;
@@ -140,20 +141,35 @@ void deno_mod_evaluate(Deno* d_, void* user_data, deno_mod id) {
   v8::Context::Scope context_scope(context);
 
   auto* info = d->GetModuleInfo(id);
-  Local<Module> module = info->handle.Get(isolate);
-
-  CHECK_EQ(Module::kInstantiated, module->GetStatus());
+  auto module = info->handle.Get(isolate);
+  auto status = module->GetStatus();
+
+  if (status == Module::kInstantiated) {
+    bool ok = !module->Evaluate(context).IsEmpty();
+    status = module->GetStatus();  // Update status after evaluating.
+    CHECK_IMPLIES(ok, status == Module::kEvaluated);
+    CHECK_IMPLIES(!ok, status == Module::kErrored);
+  }
 
-  auto maybe_result = module->Evaluate(context);
-  if (maybe_result.IsEmpty()) {
-    CHECK_EQ(Module::kErrored, module->GetStatus());
-    HandleException(context, module->GetException());
+  switch (status) {
+    case Module::kEvaluated:
+      ClearException(context);
+      break;
+    case Module::kErrored:
+      HandleException(context, module->GetException());
+      break;
+    default:
+      FATAL("Unexpected module status: %d", static_cast<int>(status));
   }
 }
 
-void deno_dyn_import(Deno* d_, void* user_data, deno_dyn_import_id import_id,
-                     deno_mod mod_id) {
+void deno_dyn_import_done(Deno* d_, void* user_data,
+                          deno_dyn_import_id import_id, deno_mod mod_id,
+                          const char* error_str) {
   auto* d = unwrap(d_);
+  CHECK((mod_id == 0 && error_str != nullptr) ||
+        (mod_id != 0 && error_str == nullptr) ||
+        (mod_id == 0 && !d->last_exception_handle_.IsEmpty()));
   deno::UserDataScope user_data_scope(d, user_data);
 
   auto* isolate = d->isolate_;
@@ -163,8 +179,6 @@ void deno_dyn_import(Deno* d_, void* user_data, deno_dyn_import_id import_id,
   auto context = d->context_.Get(d->isolate_);
   v8::Context::Scope context_scope(context);
 
-  v8::TryCatch try_catch(isolate);
-
   auto it = d->dyn_import_map_.find(import_id);
   if (it == d->dyn_import_map_.end()) {
     CHECK(false);  // TODO(ry) error on bad import_id.
@@ -183,18 +197,22 @@ void deno_dyn_import(Deno* d_, void* user_data, deno_dyn_import_id import_id,
 
   if (info == nullptr) {
     // Resolution error.
-    promise->Reject(context, v8::Null(isolate)).ToChecked();
+    if (error_str != nullptr) {
+      auto msg = deno::v8_str(error_str);
+      auto exception = v8::Exception::TypeError(msg);
+      promise->Reject(context, exception).ToChecked();
+    } else {
+      auto e = d->last_exception_handle_.Get(isolate);
+      ClearException(context);
+      promise->Reject(context, e).ToChecked();
+    }
   } else {
     // Resolution success
     Local<Module> module = info->handle.Get(isolate);
-    CHECK_GE(module->GetStatus(), v8::Module::kInstantiated);
+    CHECK_EQ(module->GetStatus(), v8::Module::kEvaluated);
     Local<Value> module_namespace = module->GetModuleNamespace();
     promise->Resolve(context, module_namespace).ToChecked();
   }
-
-  if (try_catch.HasCaught()) {
-    HandleException(context, try_catch.Exception());
-  }
 }
 
 }  // extern "C"
diff --git a/core/libdeno/modules_test.cc b/core/libdeno/modules_test.cc
index 490dccdbe..987e88791 100644
--- a/core/libdeno/modules_test.cc
+++ b/core/libdeno/modules_test.cc
@@ -158,7 +158,7 @@ TEST(ModulesTest, DynamicImportSuccess) {
     dyn_import_count++;
     EXPECT_STREQ(specifier, "foo");
     EXPECT_STREQ(referrer, "a.js");
-    deno_dyn_import(d, d, import_id, b);
+    deno_dyn_import_done(d, d, import_id, b, nullptr);
   };
   const char* src =
       "(async () => { \n"
@@ -179,6 +179,8 @@ TEST(ModulesTest, DynamicImportSuccess) {
   EXPECT_EQ(nullptr, deno_last_exception(d));
   deno_mod_instantiate(d, d, b, nullptr);
   EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_evaluate(d, d, b);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
   deno_mod_evaluate(d, d, a);
   EXPECT_EQ(nullptr, deno_last_exception(d));
   deno_check_promise_errors(d);
@@ -198,7 +200,7 @@ TEST(ModulesTest, DynamicImportError) {
     EXPECT_STREQ(specifier, "foo");
     EXPECT_STREQ(referrer, "a.js");
     // We indicate there was an error resolving by returning mod_id 0.
-    deno_dyn_import(d, d, import_id, 0);
+    deno_dyn_import_done(d, d, import_id, 0, "foo not found");
   };
   const char* src =
       "(async () => { \n"
@@ -218,6 +220,8 @@ TEST(ModulesTest, DynamicImportError) {
   // Now we should get an error.
   deno_check_promise_errors(d);
   EXPECT_NE(deno_last_exception(d), nullptr);
+  std::string e(deno_last_exception(d));
+  EXPECT_NE(e.find("Uncaught TypeError: foo not found"), std::string::npos);
   deno_delete(d);
   EXPECT_EQ(0, exec_count);
   EXPECT_EQ(1, dyn_import_count);
@@ -234,7 +238,7 @@ TEST(ModulesTest, DynamicImportAsync) {
     dyn_import_count++;
     EXPECT_STREQ(specifier, "foo");
     EXPECT_STREQ(referrer, "a.js");
-    // We don't call deno_dyn_import until later.
+    // We don't call deno_dyn_import_done until later.
     import_ids.push_back(import_id);
   };
   const char* src =
@@ -270,13 +274,15 @@ TEST(ModulesTest, DynamicImportAsync) {
   EXPECT_EQ(nullptr, deno_last_exception(d));
   deno_mod_instantiate(d, d, b, nullptr);
   EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_evaluate(d, d, b);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
 
   // Now we resolve the import.
   EXPECT_EQ(1u, import_ids.size());
   auto import_id = import_ids.back();
   import_ids.pop_back();
 
-  deno_dyn_import(d, d, import_id, b);
+  deno_dyn_import_done(d, d, import_id, b, nullptr);
 
   EXPECT_EQ(nullptr, deno_last_exception(d));
   deno_check_promise_errors(d);
@@ -288,7 +294,7 @@ TEST(ModulesTest, DynamicImportAsync) {
 
   import_id = import_ids.back();
   import_ids.pop_back();
-  deno_dyn_import(d, d, import_id, b);
+  deno_dyn_import_done(d, d, import_id, b, nullptr);
 
   EXPECT_EQ(nullptr, deno_last_exception(d));
   deno_check_promise_errors(d);
@@ -300,3 +306,119 @@ TEST(ModulesTest, DynamicImportAsync) {
 
   deno_delete(d);
 }
+
+TEST(ModulesTest, DynamicImportThrows) {
+  exec_count = 0;
+  static int dyn_import_count = 0;
+  static deno_mod b = 0;
+  static std::vector<deno_dyn_import_id> import_ids = {};
+  auto dyn_import_cb = [](auto user_data, const char* specifier,
+                          const char* referrer, deno_dyn_import_id import_id) {
+    dyn_import_count++;
+    EXPECT_STREQ(specifier, "b.js");
+    EXPECT_STREQ(referrer, "a.js");
+    // We don't call deno_dyn_import_done until later.
+    import_ids.push_back(import_id);
+  };
+  Deno* d = deno_new(deno_config{0, snapshot, empty, recv_cb, dyn_import_cb});
+
+  // Instantiate and evaluate the root module. This should succeed.
+  const char* a_src =
+      "(async () => { \n"
+      "  let mod = await import('b.js'); \n"
+      // unreachable
+      "  Deno.core.send(new Uint8Array([4])); \n"
+      "})(); \n";
+  static deno_mod a = deno_mod_new(d, true, "a.js", a_src);
+  EXPECT_NE(a, 0);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_instantiate(d, d, a, nullptr);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_evaluate(d, d, a);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_check_promise_errors(d);
+  EXPECT_EQ(deno_last_exception(d), nullptr);
+
+  // Instantiate b.js, which should succeed.
+  const char* b_src = "throw new Error('foo')";
+  b = deno_mod_new(d, false, "b.js", b_src);
+  EXPECT_NE(b, 0);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_instantiate(d, d, b, nullptr);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+
+  // Evaluate b.js. It throws in the global scope, so deno_last_exception()
+  // should be non-null afterwards.
+  deno_mod_evaluate(d, d, b);
+  EXPECT_NE(deno_last_exception(d), nullptr);
+
+  // Resolve the dynamic import of b.js. Since deno_mod_evaluate() failed,
+  // we indicate failure to deno_dyn_import_done() by setting mod_id to 0.
+  // The last error should be picked up and cleared by deno_dyn_import_done().
+  EXPECT_EQ(1u, import_ids.size());
+  auto import_id = import_ids.back();
+  import_ids.pop_back();
+  deno_dyn_import_done(d, d, import_id, 0, nullptr);
+  EXPECT_EQ(deno_last_exception(d), nullptr);
+
+  // Since the dynamically imported module threw an error,
+  // it should show up as an unhandled promise rejection.
+  deno_check_promise_errors(d);
+  EXPECT_NE(deno_last_exception(d), nullptr);
+  std::string e(deno_last_exception(d));
+  EXPECT_NE(e.find("Uncaught Error: foo"), std::string::npos);
+
+  EXPECT_EQ(1, dyn_import_count);
+  EXPECT_EQ(0, exec_count);
+
+  deno_delete(d);
+}
+
+TEST(ModulesTest, DynamicImportSyntaxError) {
+  exec_count = 0;
+  static int dyn_import_count = 0;
+  auto dyn_import_cb = [](auto user_data, const char* specifier,
+                          const char* referrer, deno_dyn_import_id import_id) {
+    auto d = reinterpret_cast<Deno*>(user_data);
+    dyn_import_count++;
+    EXPECT_STREQ(specifier, "b.js");
+    EXPECT_STREQ(referrer, "a.js");
+
+    // Compile b.js, which should fail because of the syntax error.
+    deno_mod b = deno_mod_new(d, false, "b.js", "syntax error");
+    EXPECT_EQ(b, 0);
+    EXPECT_NE(nullptr, deno_last_exception(d));
+
+    // `deno_dyn_import_done` should consume the last exception, and use it
+    // to reject the dynamic import promise.
+    deno_dyn_import_done(d, d, import_id, 0, nullptr);
+    EXPECT_EQ(nullptr, deno_last_exception(d));
+  };
+  Deno* d = deno_new(deno_config{0, snapshot, empty, recv_cb, dyn_import_cb});
+
+  // Instantiate and evaluate the root module. This should succeed.
+  const char* src =
+      "(async () => { \n"
+      "  let mod = await import('b.js'); \n"
+      // unreachable
+      "  Deno.core.send(new Uint8Array([4])); \n"
+      "})(); \n";
+  static deno_mod a = deno_mod_new(d, true, "a.js", src);
+  EXPECT_NE(a, 0);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_instantiate(d, d, a, nullptr);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+  deno_mod_evaluate(d, d, a);
+  EXPECT_EQ(nullptr, deno_last_exception(d));
+
+  // The failed dynamic import should cause an unhandled promise rejection.
+  deno_check_promise_errors(d);
+  EXPECT_NE(deno_last_exception(d), nullptr);
+  EXPECT_NE(std::string(deno_last_exception(d)).find("Syntax"),
+            std::string::npos);
+
+  EXPECT_EQ(1, dyn_import_count);
+  EXPECT_EQ(0, exec_count);
+
+  deno_delete(d);
+}