deps: V8: backport 777fa98

Original commit message: Make SetSyntheticModuleExport throw instead of crash for nonexistent export name Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError when called with an export name that was not supplied when constructing that SyntheticModule. Instead, the current implementation crashes with a failed CHECK(). Add a new Module::SyntheticModuleSetExport that throws (without an ensuing crash) for this case, and deprecate the old Module::SetSyntheticModuleExport. Bug: v8:9828 Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996 Reviewed-by: Adam Klein <[email protected]> Reviewed-by: Georg Neis <[email protected]> Commit-Queue: Dan Clark <[email protected]> Cr-Commit-Position: refs/heads/master@{#64438} Refs: v8/v8@777fa98 PR-URL: nodejs#30062 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 · Jan 7, 2020 · 72493af · 72493af
1 parent 4442359
commit 72493af
Show file tree

Hide file tree

Showing 7 changed files with 114 additions and 19 deletions.
diff --git a/common.gypi b/common.gypi
@@ -38,7 +38,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.16',
+    'v8_embedder_string': '-node.17',
 
     ##### V8 defaults for Node.js #####
 

diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h
@@ -1385,11 +1385,19 @@ class V8_EXPORT Module {
   /**
    * Set this module's exported value for the name export_name to the specified
    * export_value. This method must be called only on Modules created via
-   * CreateSyntheticModule. export_name must be one of the export_names that
-   * were passed in that CreateSyntheticModule call.
-   */
-  void SetSyntheticModuleExport(Local<String> export_name,
-                                Local<Value> export_value);
+   * CreateSyntheticModule.  An error will be thrown if export_name is not one
+   * of the export_names that were passed in that CreateSyntheticModule call.
+   * Returns Just(true) on success, Nothing<bool>() if an error was thrown.
+   */
+  V8_WARN_UNUSED_RESULT Maybe<bool> SetSyntheticModuleExport(
+      Isolate* isolate, Local<String> export_name, Local<Value> export_value);
+  V8_DEPRECATE_SOON(
+      "Use the preceding SetSyntheticModuleExport with an Isolate parameter, "
+      "instead of the one that follows.  The former will throw a runtime "
+      "error if called for an export that doesn't exist (as per spec); "
+      "the latter will crash with a failed CHECK().",
+      void SetSyntheticModuleExport(Local<String> export_name,
+                                    Local<Value> export_value));
 };
 
 /**

diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc
@@ -2333,6 +2333,28 @@ Local<Module> Module::CreateSyntheticModule(
           i_module_name, i_export_names, evaluation_steps)));
 }
 
+Maybe<bool> Module::SetSyntheticModuleExport(Isolate* isolate,
+                                             Local<String> export_name,
+                                             Local<v8::Value> export_value) {
+  auto i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  i::Handle<i::String> i_export_name = Utils::OpenHandle(*export_name);
+  i::Handle<i::Object> i_export_value = Utils::OpenHandle(*export_value);
+  i::Handle<i::Module> self = Utils::OpenHandle(this);
+  Utils::ApiCheck(self->IsSyntheticModule(),
+                  "v8::Module::SyntheticModuleSetExport",
+                  "v8::Module::SyntheticModuleSetExport must only be called on "
+                  "a SyntheticModule");
+  ENTER_V8_NO_SCRIPT(i_isolate, isolate->GetCurrentContext(), Module,
+                     SetSyntheticModuleExport, Nothing<bool>(), i::HandleScope);
+  has_pending_exception =
+      i::SyntheticModule::SetExport(i_isolate,
+                                    i::Handle<i::SyntheticModule>::cast(self),
+                                    i_export_name, i_export_value)
+          .IsNothing();
+  RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool);
+  return Just(true);
+}
+
 void Module::SetSyntheticModuleExport(Local<String> export_name,
                                       Local<v8::Value> export_value) {
   i::Handle<i::String> i_export_name = Utils::OpenHandle(*export_name);
@@ -2342,9 +2364,9 @@ void Module::SetSyntheticModuleExport(Local<String> export_name,
                   "v8::Module::SetSyntheticModuleExport",
                   "v8::Module::SetSyntheticModuleExport must only be called on "
                   "a SyntheticModule");
-  i::SyntheticModule::SetExport(self->GetIsolate(),
-                                i::Handle<i::SyntheticModule>::cast(self),
-                                i_export_name, i_export_value);
+  i::SyntheticModule::SetExportStrict(self->GetIsolate(),
+                                      i::Handle<i::SyntheticModule>::cast(self),
+                                      i_export_name, i_export_value);
 }
 
 namespace {

diff --git a/deps/v8/src/logging/counters.h b/deps/v8/src/logging/counters.h
@@ -779,6 +779,7 @@ class RuntimeCallTimer final {
   V(Message_GetStartColumn)                                \
   V(Module_Evaluate)                                       \
   V(Module_InstantiateModule)                              \
+  V(Module_SetSyntheticModuleExport)                       \
   V(NumberObject_New)                                      \
   V(NumberObject_NumberValue)                              \
   V(Object_CallAsConstructor)                              \

diff --git a/deps/v8/src/objects/synthetic-module.cc b/deps/v8/src/objects/synthetic-module.cc
@@ -17,16 +17,36 @@ namespace internal {
 
 // Implements SetSyntheticModuleBinding:
 // https://heycam.github.io/webidl/#setsyntheticmoduleexport
-void SyntheticModule::SetExport(Isolate* isolate,
-                                Handle<SyntheticModule> module,
-                                Handle<String> export_name,
-                                Handle<Object> export_value) {
+Maybe<bool> SyntheticModule::SetExport(Isolate* isolate,
+                                       Handle<SyntheticModule> module,
+                                       Handle<String> export_name,
+                                       Handle<Object> export_value) {
   Handle<ObjectHashTable> exports(module->exports(), isolate);
   Handle<Object> export_object(exports->Lookup(export_name), isolate);
-  CHECK(export_object->IsCell());
+
+  if (!export_object->IsCell()) {
+    isolate->Throw(*isolate->factory()->NewReferenceError(
+        MessageTemplate::kModuleExportUndefined, export_name));
+    return Nothing<bool>();
+  }
+
   Handle<Cell> export_cell(Handle<Cell>::cast(export_object));
   // Spec step 2: Set the mutable binding of export_name to export_value
   export_cell->set_value(*export_value);
+
+  return Just(true);
+}
+
+void SyntheticModule::SetExportStrict(Isolate* isolate,
+                                      Handle<SyntheticModule> module,
+                                      Handle<String> export_name,
+                                      Handle<Object> export_value) {
+  Handle<ObjectHashTable> exports(module->exports(), isolate);
+  Handle<Object> export_object(exports->Lookup(export_name), isolate);
+  CHECK(export_object->IsCell());
+  Maybe<bool> set_export_result =
+      SetExport(isolate, module, export_name, export_value);
+  CHECK(set_export_result.FromJust());
 }
 
 // Implements Synthetic Module Record's ResolveExport concrete method:

diff --git a/deps/v8/src/objects/synthetic-module.h b/deps/v8/src/objects/synthetic-module.h
@@ -29,9 +29,21 @@ class SyntheticModule : public Module {
   DECL_ACCESSORS(export_names, FixedArray)
   DECL_ACCESSORS(evaluation_steps, Foreign)
 
-  static void SetExport(Isolate* isolate, Handle<SyntheticModule> module,
-                        Handle<String> export_name,
-                        Handle<Object> export_value);
+  // Set module's exported value for the specified export_name to the specified
+  // export_value.  An error will be thrown if export_name is not one
+  // of the export_names that were supplied during module construction.
+  // Returns Just(true) on success, Nothing<bool>() if an error was thrown.
+  static Maybe<bool> SetExport(Isolate* isolate, Handle<SyntheticModule> module,
+                               Handle<String> export_name,
+                               Handle<Object> export_value);
+  // The following redundant method should be deleted when the deprecated
+  // version of v8::SetSyntheticModuleExport is removed.  It differs from
+  // SetExport in that it crashes rather than throwing an error if the caller
+  // attempts to set an export_name that was not present during construction of
+  // the module.
+  static void SetExportStrict(Isolate* isolate, Handle<SyntheticModule> module,
+                              Handle<String> export_name,
+                              Handle<Object> export_value);
 
   // Layout description.
   DEFINE_FIELD_OFFSET_CONSTANTS(Module::kHeaderSize,

diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc
@@ -23628,7 +23628,9 @@ v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallbackFail(
 
 v8::MaybeLocal<Value> SyntheticModuleEvaluationStepsCallbackSetExport(
     Local<Context> context, Local<Module> module) {
-  module->SetSyntheticModuleExport(v8_str("test_export"), v8_num(42));
+  Maybe<bool> set_export_result = module->SetSyntheticModuleExport(
+      context->GetIsolate(), v8_str("test_export"), v8_num(42));
+  CHECK(set_export_result.FromJust());
   return v8::Undefined(reinterpret_cast<v8::Isolate*>(context->GetIsolate()));
 }
 
@@ -23835,7 +23837,9 @@ TEST(SyntheticModuleSetExports) {
   // undefined.
   CHECK(foo_cell->value().IsUndefined());
 
-  module->SetSyntheticModuleExport(foo_string, bar_string);
+  Maybe<bool> set_export_result =
+      module->SetSyntheticModuleExport(isolate, foo_string, bar_string);
+  CHECK(set_export_result.FromJust());
 
   // After setting the export the Cell should still have the same idenitity.
   CHECK_EQ(exports->Lookup(v8::Utils::OpenHandle(*foo_string)), *foo_cell);
@@ -23846,6 +23850,34 @@ TEST(SyntheticModuleSetExports) {
             ->Equals(*v8::Utils::OpenHandle(*bar_string)));
 }
 
+TEST(SyntheticModuleSetMissingExport) {
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  auto i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  v8::Isolate::Scope iscope(isolate);
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::Context> context = v8::Context::New(isolate);
+  v8::Context::Scope cscope(context);
+
+  Local<String> foo_string = v8_str("foo");
+  Local<String> bar_string = v8_str("bar");
+
+  Local<Module> module = CreateAndInstantiateSyntheticModule(
+      isolate, v8_str("SyntheticModuleSetExports-TestSyntheticModule"), context,
+      std::vector<v8::Local<v8::String>>(),
+      UnexpectedSyntheticModuleEvaluationStepsCallback);
+
+  i::Handle<i::SyntheticModule> i_module =
+      i::Handle<i::SyntheticModule>::cast(v8::Utils::OpenHandle(*module));
+  i::Handle<i::ObjectHashTable> exports(i_module->exports(), i_isolate);
+
+  TryCatch try_catch(isolate);
+  Maybe<bool> set_export_result =
+      module->SetSyntheticModuleExport(isolate, foo_string, bar_string);
+  CHECK(set_export_result.IsNothing());
+  CHECK(try_catch.HasCaught());
+}
+
 TEST(SyntheticModuleEvaluationStepsNoThrow) {
   synthetic_module_callback_count = 0;
   LocalContext env;