Update module APIs to return Maybe<bool>

All APIs that can throw exceptions should return Maybe<> values

BUG=none
R=neis@chromium.org,gsathya@chromium.org

Change-Id: I6a6e5888cd71257bb02bdcfcc587c909d0c1d8f4
Reviewed-on: https://chromium-review.googlesource.com/517785
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45557}

include/v8.h

@@ -1032,7 +1032,6 @@ class ScriptOrigin {
   Local<Value> source_map_url_;
 };
 
-
 /**
  * A compiled JavaScript script, not yet tied to a Context.
  */
@@ -1095,11 +1094,15 @@ class V8_EXPORT Module {
   /**
    * ModuleDeclarationInstantiation
    *
-   * Returns false if an exception occurred during instantiation. (In the case
-   * where the callback throws an exception, that exception is propagated.)
+   * Returns an empty Maybe<bool> if an exception occurred during
+   * instantiation. (In the case where the callback throws an exception, that
+   * exception is propagated.)
    */
-  V8_WARN_UNUSED_RESULT bool Instantiate(Local<Context> context,
-                                         ResolveCallback callback);
+  V8_DEPRECATE_SOON("Use Maybe<bool> version",
+                    bool Instantiate(Local<Context> context,
+                                     ResolveCallback callback));
+  V8_WARN_UNUSED_RESULT Maybe<bool> InstantiateModule(Local<Context> context,
+                                                      ResolveCallback callback);
 
   /**
    * ModuleEvaluation
@@ -1121,14 +1124,14 @@ class V8_EXPORT DynamicImportResult {
    * Resolves the promise with the namespace object of the given
    * module.
    */
-  V8_WARN_UNUSED_RESULT bool FinishDynamicImportSuccess(Local<Context> context,
-                                                        Local<Module> module);
+  V8_WARN_UNUSED_RESULT Maybe<bool> FinishDynamicImportSuccess(
+      Local<Context> context, Local<Module> module);
 
   /**
    * Rejects the promise with the given exception.
    */
-  V8_WARN_UNUSED_RESULT bool FinishDynamicImportFailure(Local<Context> context,
-                                                        Local<Value> exception);
+  V8_WARN_UNUSED_RESULT Maybe<bool> FinishDynamicImportFailure(
+      Local<Context> context, Local<Value> exception);
 };
 
 /**

src/api.cc

@@ -148,10 +148,6 @@ namespace v8 {
   PREPARE_FOR_EXECUTION_WITH_CONTEXT(context, class_name, function_name,       \
                                      Nothing<T>(), i::HandleScope, false)
 
-#define PREPARE_FOR_EXECUTION_BOOL(context, class_name, function_name)   \
-  PREPARE_FOR_EXECUTION_WITH_CONTEXT(context, class_name, function_name, \
-                                     false, i::HandleScope, false)
-
 #ifdef DEBUG
 #define ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate)                    \
   i::VMState<v8::OTHER> __state__((isolate));                       \
@@ -185,9 +181,6 @@ namespace v8 {
 #define RETURN_ON_FAILED_EXECUTION_PRIMITIVE(T) \
   EXCEPTION_BAILOUT_CHECK_SCOPED(isolate, Nothing<T>())
 
-#define RETURN_ON_FAILED_EXECUTION_BOOL() \
-  EXCEPTION_BAILOUT_CHECK_SCOPED(isolate, false)
-
 #define RETURN_TO_LOCAL_UNCHECKED(maybe_local, T) \
   return maybe_local.FromMaybe(Local<T>());
 
@@ -2059,9 +2052,10 @@ Local<UnboundScript> Script::GetUnboundScript() {
       i::Handle<i::SharedFunctionInfo>(i::JSFunction::cast(*obj)->shared()));
 }
 
-bool DynamicImportResult::FinishDynamicImportSuccess(Local<Context> context,
-                                                     Local<Module> module) {
-  PREPARE_FOR_EXECUTION_BOOL(context, Module, FinishDynamicImportSuccess);
+Maybe<bool> DynamicImportResult::FinishDynamicImportSuccess(
+    Local<Context> context, Local<Module> module) {
+  PREPARE_FOR_EXECUTION_PRIMITIVE(context, Module, FinishDynamicImportSuccess,
+                                  bool);
   auto promise = Utils::OpenHandle(this);
   i::Handle<i::Module> module_obj = Utils::OpenHandle(*module);
   i::Handle<i::JSModuleNamespace> module_namespace =
@@ -2072,13 +2066,14 @@ bool DynamicImportResult::FinishDynamicImportSuccess(Local<Context> context,
                          isolate->factory()->undefined_value(), arraysize(argv),
                          argv)
           .is_null();
-  RETURN_ON_FAILED_EXECUTION_BOOL();
-  return true;
+  RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool);
+  return Just(true);
 }
 
-bool DynamicImportResult::FinishDynamicImportFailure(Local<Context> context,
-                                                     Local<Value> exception) {
-  PREPARE_FOR_EXECUTION_BOOL(context, Module, FinishDynamicImportFailure);
+Maybe<bool> DynamicImportResult::FinishDynamicImportFailure(
+    Local<Context> context, Local<Value> exception) {
+  PREPARE_FOR_EXECUTION_PRIMITIVE(context, Module, FinishDynamicImportFailure,
+                                  bool);
   auto promise = Utils::OpenHandle(this);
   // We pass true to trigger the debugger's on exception handler.
   i::Handle<i::Object> argv[] = {promise, Utils::OpenHandle(*exception),
@@ -2088,8 +2083,8 @@ bool DynamicImportResult::FinishDynamicImportFailure(Local<Context> context,
                          isolate->factory()->undefined_value(), arraysize(argv),
                          argv)
           .is_null();
-  RETURN_ON_FAILED_EXECUTION_BOOL();
-  return true;
+  RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool);
+  return Just(true);
 }
 
 int Module::GetModuleRequestsLength() const {
@@ -2111,11 +2106,16 @@ int Module::GetIdentityHash() const { return Utils::OpenHandle(this)->hash(); }
 
 bool Module::Instantiate(Local<Context> context,
                          Module::ResolveCallback callback) {
-  PREPARE_FOR_EXECUTION_BOOL(context, Module, Instantiate);
+  return InstantiateModule(context, callback).FromMaybe(false);
+}
+
+Maybe<bool> Module::InstantiateModule(Local<Context> context,
+                                      Module::ResolveCallback callback) {
+  PREPARE_FOR_EXECUTION_PRIMITIVE(context, Module, InstantiateModule, bool);
   has_pending_exception =
       !i::Module::Instantiate(Utils::OpenHandle(this), context, callback);
-  RETURN_ON_FAILED_EXECUTION_BOOL();
-  return true;
+  RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool);
+  return Just(true);
 }
 
 MaybeLocal<Value> Module::Evaluate(Local<Context> context) {

src/counters.h

@@ -621,7 +621,7 @@ class RuntimeCallTimer final {
   V(Module_FinishDynamicImportSuccess)                     \
   V(Module_FinishDynamicImportFailure)                     \
   V(Module_Evaluate)                                       \
-  V(Module_Instantiate)                                    \
+  V(Module_InstantiateModule)                              \
   V(NumberObject_New)                                      \
   V(NumberObject_NumberValue)                              \
   V(Object_CallAsConstructor)                              \

src/d8.cc

@@ -826,12 +826,14 @@ void Shell::DoHostImportModuleDynamically(void* import_data) {
     root_module = module_it->second.Get(isolate);
   } else if (!FetchModuleTree(realm, absolute_path).ToLocal(&root_module)) {
     CHECK(try_catch.HasCaught());
-    CHECK(result->FinishDynamicImportFailure(realm, try_catch.Exception()));
+    CHECK(result->FinishDynamicImportFailure(realm, try_catch.Exception())
+              .FromJust());
     return;
   }
 
   MaybeLocal<Value> maybe_result;
-  if (root_module->Instantiate(realm, ResolveModuleCallback)) {
+  if (root_module->InstantiateModule(realm, ResolveModuleCallback)
+          .FromMaybe(false)) {
     maybe_result = root_module->Evaluate(realm);
     EmptyMessageQueues(isolate);
   }
@@ -839,12 +841,13 @@ void Shell::DoHostImportModuleDynamically(void* import_data) {
   Local<Value> module;
   if (!maybe_result.ToLocal(&module)) {
     DCHECK(try_catch.HasCaught());
-    CHECK(result->FinishDynamicImportFailure(realm, try_catch.Exception()));
+    CHECK(result->FinishDynamicImportFailure(realm, try_catch.Exception())
+              .FromJust());
     return;
   }
 
   DCHECK(!try_catch.HasCaught());
-  CHECK(result->FinishDynamicImportSuccess(realm, root_module));
+  CHECK(result->FinishDynamicImportSuccess(realm, root_module).FromJust());
 }
 
 bool Shell::ExecuteModule(Isolate* isolate, const char* file_name) {
@@ -869,7 +872,8 @@ bool Shell::ExecuteModule(Isolate* isolate, const char* file_name) {
   }
 
   MaybeLocal<Value> maybe_result;
-  if (root_module->Instantiate(realm, ResolveModuleCallback)) {
+  if (root_module->InstantiateModule(realm, ResolveModuleCallback)
+          .FromMaybe(false)) {
     maybe_result = root_module->Evaluate(realm);
     EmptyMessageQueues(isolate);
   }

src/isolate.cc

@@ -3353,9 +3353,11 @@ void Isolate::RunHostImportModuleDynamicallyCallback(
   if (host_import_module_dynamically_callback_ == nullptr) {
     Handle<Object> exception =
         factory()->NewError(error_function(), MessageTemplate::kUnsupported);
-    CHECK(result->FinishDynamicImportFailure(
-        v8::Utils::ToLocal(handle(context(), this)),
-        v8::Utils::ToLocal(exception)));
+    CHECK(result
+              ->FinishDynamicImportFailure(
+                  v8::Utils::ToLocal(handle(context(), this)),
+                  v8::Utils::ToLocal(exception))
+              .FromJust());
     return;
   }
 

test/cctest/test-modules.cc

@@ -70,7 +70,8 @@ TEST(ModuleInstantiationFailures) {
   // Instantiation should fail.
   {
     v8::TryCatch inner_try_catch(isolate);
-    CHECK(!module->Instantiate(env.local(), FailAlwaysResolveCallback));
+    CHECK(module->InstantiateModule(env.local(), FailAlwaysResolveCallback)
+              .IsNothing());
     CHECK(inner_try_catch.HasCaught());
     CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("boom")));
   }
@@ -82,7 +83,9 @@ TEST(ModuleInstantiationFailures) {
   g_count = 0;
   {
     v8::TryCatch inner_try_catch(isolate);
-    CHECK(!module->Instantiate(env.local(), FailOnSecondCallResolveCallback));
+    CHECK(
+        module->InstantiateModule(env.local(), FailOnSecondCallResolveCallback)
+            .IsNothing());
     CHECK(inner_try_catch.HasCaught());
     CHECK(inner_try_catch.Exception()->StrictEquals(v8_str("booom")));
   }
@@ -111,8 +114,10 @@ TEST(ModuleEvaluation) {
   ScriptCompiler::Source source(source_text, origin);
   Local<Module> module =
       ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
-  CHECK(module->Instantiate(env.local(),
-                            CompileSpecifierAsModuleResolveCallback));
+  CHECK(module
+            ->InstantiateModule(env.local(),
+                                CompileSpecifierAsModuleResolveCallback)
+            .FromJust());
   CHECK(!module->Evaluate(env.local()).IsEmpty());
   ExpectInt32("Object.expando", 10);
 
@@ -151,8 +156,10 @@ TEST(ModuleEvaluationCompletion1) {
     ScriptCompiler::Source source(source_text, origin);
     Local<Module> module =
         ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
-    CHECK(module->Instantiate(env.local(),
-                              CompileSpecifierAsModuleResolveCallback));
+    CHECK(module
+              ->InstantiateModule(env.local(),
+                                  CompileSpecifierAsModuleResolveCallback)
+              .FromJust());
     CHECK(module->Evaluate(env.local()).ToLocalChecked()->IsUndefined());
   }
 
@@ -190,8 +197,10 @@ TEST(ModuleEvaluationCompletion2) {
     ScriptCompiler::Source source(source_text, origin);
     Local<Module> module =
         ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
-    CHECK(module->Instantiate(env.local(),
-                              CompileSpecifierAsModuleResolveCallback));
+    CHECK(module
+              ->InstantiateModule(env.local(),
+                                  CompileSpecifierAsModuleResolveCallback)
+              .FromJust());
     CHECK(module->Evaluate(env.local())
               .ToLocalChecked()
               ->StrictEquals(v8_str("gaga")));

test/inspector/isolate-data.cc

@@ -100,8 +100,10 @@ void IsolateData::RegisterModule(v8::Local<v8::Context> context,
   v8::Local<v8::Module> module;
   if (!v8::ScriptCompiler::CompileModule(isolate(), source).ToLocal(&module))
     return;
-  if (!module->Instantiate(context, &IsolateData::ModuleResolveCallback))
+  if (!module->InstantiateModule(context, &IsolateData::ModuleResolveCallback)
+           .FromMaybe(false)) {
     return;
+  }
   v8::Local<v8::Value> result;
   if (!module->Evaluate(context).ToLocal(&result)) return;
   modules_[name] = v8::Global<v8::Module>(isolate_, module);