[modules][api] Stop filtering import assertions based on list provided by the host

The change https://chromium-review.googlesource.com/c/v8/v8/+/2572173
implemented HostGetSupportedImportAssertions [1] in a fairly literal
sense, where the host supplies a list of supported import assertions
and V8 filters the import assertions in a ModuleRequest and exposes
only supported assertions via its API surface.

However, we've decided that the interop guarantees provided
by doing the filtering in V8 are probably not worth the added
complexity. Thus, this change removes the filtering. Going forward,
hosts will be expected to ignore unknown asserions received from V8.

This is mostly a revert of
https://chromium-review.googlesource.com/c/v8/v8/+/2572173, with
v8::Isolate::CreateParams::supported_import_assertions being kept
for now (since we first have to delete the Blink code that sets it),
and a new comment in v8.h instructing hosts to ignore unknown
assertions.

[1] https://tc39.es/proposal-import-assertions/#sec-hostgetsupportedimportassertions

Bug: v8:10958
Change-Id: I7e8e2a7fbfe2d5bf891805cff6c3160d0e6825cd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2643563Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Mythri Alle <mythria@chromium.org>
Commit-Queue: Dan Clark <daniec@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#72299}

include/v8.h

@@ -1564,6 +1564,13 @@ class V8_EXPORT ModuleRequest : public Data {
    * The keys and values are of type v8::String, and the source offsets are of
    * type Int32. Use Module::SourceOffsetToLocation to convert the source
    * offsets to Locations with line/column numbers.
+   *
+   * All assertions present in the module request will be supplied in this
+   * list, regardless of whether they are supported by the host. Per
+   * https://tc39.es/proposal-import-assertions/#sec-hostgetsupportedimportassertions,
+   * hosts are expected to ignore assertions that they do not support (as
+   * opposed to, for example, triggering an error if an unsupported assertion is
+   * present).
    */
   Local<FixedArray> GetImportAssertions() const;
 
@@ -8437,18 +8444,9 @@ class V8_EXPORT Isolate {
      */
     std::shared_ptr<CppHeapCreateParams> cpp_heap_params;
 
-    /**
-     * This list is provided by the embedder to indicate which import assertions
-     * they want to handle. Only import assertions whose keys are present in
-     * supported_import_assertions will be included in the import assertions
-     * lists of ModuleRequests that will be passed to the embedder. If
-     * supported_import_assertions is left empty, then the embedder will not
-     * receive any import assertions.
-     *
-     * This corresponds to the list returned by the HostGetSupportedAssertions
-     * host-defined abstract operation:
-     * https://tc39.es/proposal-import-assertions/#sec-hostgetsupportedimportassertions
-     */
+    V8_DEPRECATE_SOON(
+        "Setting this has no effect. Embedders should ignore import assertions "
+        "that they do not use.")
     std::vector<std::string> supported_import_assertions;
   };
 

src/api/api.cc

@@ -8567,9 +8567,6 @@ void Isolate::Initialize(Isolate* isolate,
   i_isolate->set_api_external_references(params.external_references);
   i_isolate->set_allow_atomics_wait(params.allow_atomics_wait);
 
-  i_isolate->set_supported_import_assertions(
-      params.supported_import_assertions);
-
   i_isolate->heap()->ConfigureHeap(params.constraints);
   if (params.cpp_heap_params) {
     i_isolate->heap()->ConfigureCppHeap(params.cpp_heap_params);

src/ast/modules.cc

@@ -127,62 +127,23 @@ Handle<PrimitiveHeapObject> ToStringOrUndefined(LocalIsolate* isolate,
 template <typename LocalIsolate>
 Handle<ModuleRequest> SourceTextModuleDescriptor::AstModuleRequest::Serialize(
     LocalIsolate* isolate) const {
-  // Copy the assertions to a FixedArray, filtering out as we go the import
-  // assertions that are not supported by the embedder.
-  //
-  // This is O(m * n) where m is the number of import assertions in this
-  // request and n is the number of supported assertions.  Both m and n are
-  // expected to be quite small, with m being 0 in the common case and n
-  // currently expected to be at most 1 since "type" is the only import
-  // assertion in use at the time of this writing. So for now we go with
-  // this simple nested loop approach, which should not result in a larger
-  // than necessary performance cost unless more than a few additional
-  // import assertions are standardized.
-  //
-  // The import assertions will be stored in the new array in the form: [key1,
-  // value1, location1, key2, value2, location2, ...].
-  const std::vector<std::string>& supported_assertions =
-      isolate->supported_import_assertions();
+  // The import assertions will be stored in this array in the form:
+  // [key1, value1, location1, key2, value2, location2, ...]
   Handle<FixedArray> import_assertions_array =
       isolate->factory()->NewFixedArray(static_cast<int>(
           import_assertions()->size() * ModuleRequest::kAssertionEntrySize));
-  int filtered_assertions_index = 0;
-  for (auto iter = import_assertions()->cbegin();
-       iter != import_assertions()->cend(); ++iter) {
-    for (const std::string& supported_assertion : supported_assertions) {
-      Handle<String> assertion_key = iter->first->string();
-      if (assertion_key->IsEqualTo(Vector<const char>(
-              supported_assertion.c_str(), supported_assertion.length()))) {
-        import_assertions_array->set(filtered_assertions_index,
-                                     *iter->first->string());
-        import_assertions_array->set(filtered_assertions_index + 1,
-                                     *iter->second.first->string());
-        import_assertions_array->set(filtered_assertions_index + 2,
-                                     Smi::FromInt(iter->second.second.beg_pos));
-        filtered_assertions_index += ModuleRequest::kAssertionEntrySize;
-        break;
-      }
-    }
-  }
 
-  Handle<FixedArray> shortened_import_assertions_array;
-  if (filtered_assertions_index < import_assertions_array->length()) {
-    // If we did filter out any assertions, create a FixedArray with the correct
-    // length. This should be rare, since it would be unexpected for developers
-    // to commonly use unsupported assertions.  Note, we don't use
-    // FixedArray::ShrinkOrEmpty here since FixedArray::Shrink isn't available
-    // on a LocalIsolate/LocalHeap.
-    shortened_import_assertions_array =
-        isolate->factory()->NewFixedArray(filtered_assertions_index);
-    import_assertions_array->CopyTo(0, *shortened_import_assertions_array, 0,
-                                    filtered_assertions_index);
-  } else {
-    shortened_import_assertions_array = import_assertions_array;
+  int i = 0;
+  for (auto iter = import_assertions()->cbegin();
+       iter != import_assertions()->cend();
+       ++iter, i += ModuleRequest::kAssertionEntrySize) {
+    import_assertions_array->set(i, *iter->first->string());
+    import_assertions_array->set(i + 1, *iter->second.first->string());
+    import_assertions_array->set(i + 2,
+                                 Smi::FromInt(iter->second.second.beg_pos));
   }
-
   return v8::internal::ModuleRequest::New(isolate, specifier()->string(),
-                                          shortened_import_assertions_array,
-                                          position());
+                                          import_assertions_array, position());
 }
 template Handle<ModuleRequest>
 SourceTextModuleDescriptor::AstModuleRequest::Serialize(Isolate* isolate) const;

src/execution/isolate.h

@@ -1592,14 +1592,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   void set_allow_atomics_wait(bool set) { allow_atomics_wait_ = set; }
   bool allow_atomics_wait() { return allow_atomics_wait_; }
 
-  void set_supported_import_assertions(
-      const std::vector<std::string>& supported_import_assertions) {
-    supported_import_assertions_ = supported_import_assertions;
-  }
-  const std::vector<std::string>& supported_import_assertions() const {
-    return supported_import_assertions_;
-  }
-
   // Register a finalizer to be called at isolate teardown.
   void RegisterManagedPtrDestructor(ManagedPtrDestructor* finalizer);
 
@@ -1833,7 +1825,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   PromiseHook promise_hook_ = nullptr;
   HostImportModuleDynamicallyCallback host_import_module_dynamically_callback_ =
       nullptr;
-  std::vector<std::string> supported_import_assertions_;
   HostInitializeImportMetaObjectCallback
       host_initialize_import_meta_object_callback_ = nullptr;
   base::Mutex rail_mutex_;

src/execution/local-isolate.cc

@@ -20,8 +20,7 @@ LocalIsolate::LocalIsolate(Isolate* isolate, ThreadKind kind)
       thread_id_(ThreadId::Current()),
       stack_limit_(kind == ThreadKind::kMain
                        ? isolate->stack_guard()->real_climit()
-                       : GetCurrentStackPosition() - FLAG_stack_size * KB),
-      supported_import_assertions_(isolate->supported_import_assertions()) {}
+                       : GetCurrentStackPosition() - FLAG_stack_size * KB) {}
 
 LocalIsolate::~LocalIsolate() = default;
 

src/execution/local-isolate.h

@@ -85,10 +85,6 @@ class V8_EXPORT_PRIVATE LocalIsolate final : private HiddenLocalFactory {
 
   bool is_main_thread() const { return heap_.is_main_thread(); }
 
-  const std::vector<std::string>& supported_import_assertions() const {
-    return supported_import_assertions_;
-  }
-
  private:
   friend class v8::internal::LocalFactory;
 
@@ -101,7 +97,6 @@ class V8_EXPORT_PRIVATE LocalIsolate final : private HiddenLocalFactory {
   std::unique_ptr<LocalLogger> logger_;
   ThreadId const thread_id_;
   Address const stack_limit_;
-  std::vector<std::string> supported_import_assertions_;
 };
 
 template <base::MutexSharedType kIsShared>

test/cctest/test-modules.cc

@@ -148,13 +148,13 @@ MaybeLocal<Module> ResolveCallbackWithImportAssertions(
   } else if (specifier->StrictEquals(v8_str("./bar.js"))) {
     CHECK_EQ(3, import_assertions->Length());
     Local<String> assertion_key =
-        import_assertions->Get(context, 0).As<Value>().As<String>();
+        import_assertions->Get(env.local(), 0).As<Value>().As<String>();
     CHECK(v8_str("a")->StrictEquals(assertion_key));
     Local<String> assertion_value =
-        import_assertions->Get(context, 1).As<Value>().As<String>();
+        import_assertions->Get(env.local(), 1).As<Value>().As<String>();
     CHECK(v8_str("b")->StrictEquals(assertion_value));
     Local<Data> assertion_source_offset_object =
-        import_assertions->Get(context, 2);
+        import_assertions->Get(env.local(), 2);
     Local<Int32> assertion_source_offset_int32 =
         assertion_source_offset_object.As<Value>()
             ->ToInt32(context)
@@ -176,25 +176,18 @@ TEST(ModuleInstantiationWithImportAssertions) {
   bool prev_top_level_await = i::FLAG_harmony_top_level_await;
   bool prev_import_assertions = i::FLAG_harmony_import_assertions;
   i::FLAG_harmony_import_assertions = true;
-
-  v8::Isolate::CreateParams create_params;
-  create_params.array_buffer_allocator = CcTest::array_buffer_allocator();
-  create_params.supported_import_assertions = {"extra0", "a", "extra1"};
-  v8::Isolate* isolate = v8::Isolate::New(create_params);
-
   for (auto top_level_await : {true, false}) {
     i::FLAG_harmony_top_level_await = top_level_await;
-    v8::Isolate::Scope isolate_scope(isolate);
+    Isolate* isolate = CcTest::isolate();
     HandleScope scope(isolate);
-    Local<v8::Context> context = v8::Context::New(isolate);
-    v8::Context::Scope context_scope(context);
+    LocalContext env;
     v8::TryCatch try_catch(isolate);
 
     Local<Module> module;
     {
       Local<String> source_text = v8_str(
           "import './foo.js' assert { };\n"
-          "export {} from './bar.js' assert { a: 'b', c: 'd' };");
+          "export {} from './bar.js' assert { a: 'b' };");
       ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
       ScriptCompiler::Source source(source_text, origin);
       module = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
@@ -202,7 +195,7 @@ TEST(ModuleInstantiationWithImportAssertions) {
       Local<FixedArray> module_requests = module->GetModuleRequests();
       CHECK_EQ(2, module_requests->Length());
       Local<ModuleRequest> module_request_0 =
-          module_requests->Get(context, 0).As<ModuleRequest>();
+          module_requests->Get(env.local(), 0).As<ModuleRequest>();
       CHECK(v8_str("./foo.js")->StrictEquals(module_request_0->GetSpecifier()));
       int offset = module_request_0->GetSourceOffset();
       CHECK_EQ(7, offset);
@@ -212,7 +205,7 @@ TEST(ModuleInstantiationWithImportAssertions) {
       CHECK_EQ(0, module_request_0->GetImportAssertions()->Length());
 
       Local<ModuleRequest> module_request_1 =
-          module_requests->Get(context, 1).As<ModuleRequest>();
+          module_requests->Get(env.local(), 1).As<ModuleRequest>();
       CHECK(v8_str("./bar.js")->StrictEquals(module_request_1->GetSpecifier()));
       offset = module_request_1->GetSourceOffset();
       CHECK_EQ(45, offset);
@@ -224,13 +217,13 @@ TEST(ModuleInstantiationWithImportAssertions) {
           module_request_1->GetImportAssertions();
       CHECK_EQ(3, import_assertions_1->Length());
       Local<String> assertion_key =
-          import_assertions_1->Get(context, 0).As<String>();
+          import_assertions_1->Get(env.local(), 0).As<String>();
       CHECK(v8_str("a")->StrictEquals(assertion_key));
       Local<String> assertion_value =
-          import_assertions_1->Get(context, 1).As<String>();
+          import_assertions_1->Get(env.local(), 1).As<String>();
       CHECK(v8_str("b")->StrictEquals(assertion_value));
       int32_t assertion_source_offset =
-          import_assertions_1->Get(context, 2).As<Int32>()->Value();
+          import_assertions_1->Get(env.local(), 2).As<Int32>()->Value();
       CHECK_EQ(65, assertion_source_offset);
       loc = module->SourceOffsetToLocation(assertion_source_offset);
       CHECK_EQ(1, loc.GetLineNumber());
@@ -255,72 +248,6 @@ TEST(ModuleInstantiationWithImportAssertions) {
           ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
     }
 
-    CHECK(
-        module->InstantiateModule(context, ResolveCallbackWithImportAssertions)
-            .FromJust());
-    CHECK_EQ(Module::kInstantiated, module->GetStatus());
-
-    MaybeLocal<Value> result = module->Evaluate(context);
-    CHECK_EQ(Module::kEvaluated, module->GetStatus());
-    if (i::FLAG_harmony_top_level_await) {
-      Local<Promise> promise = Local<Promise>::Cast(result.ToLocalChecked());
-      CHECK_EQ(promise->State(), v8::Promise::kFulfilled);
-      CHECK(promise->Result()->IsUndefined());
-    } else {
-      CHECK(!result.IsEmpty());
-      ExpectInt32("Object.expando", 42);
-    }
-    CHECK(!try_catch.HasCaught());
-  }
-
-  isolate->Dispose();
-  i::FLAG_harmony_top_level_await = prev_top_level_await;
-  i::FLAG_harmony_import_assertions = prev_import_assertions;
-}
-
-TEST(ModuleInstantiationWithImportAssertionsWithoutSupportedAssertions) {
-  bool prev_top_level_await = i::FLAG_harmony_top_level_await;
-  bool prev_import_assertions = i::FLAG_harmony_import_assertions;
-  i::FLAG_harmony_import_assertions = true;
-  for (auto top_level_await : {true, false}) {
-    i::FLAG_harmony_top_level_await = top_level_await;
-    Isolate* isolate = CcTest::isolate();
-    HandleScope scope(isolate);
-    LocalContext env;
-    v8::TryCatch try_catch(isolate);
-
-    Local<Module> module;
-    {
-      Local<String> source_text =
-          v8_str("import './foo.js' assert { a: 'b' };");
-      ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
-      ScriptCompiler::Source source(source_text, origin);
-      module = ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
-      CHECK_EQ(Module::kUninstantiated, module->GetStatus());
-      Local<FixedArray> module_requests = module->GetModuleRequests();
-      CHECK_EQ(1, module_requests->Length());
-      Local<ModuleRequest> module_request_0 =
-          module_requests->Get(env.local(), 0).As<ModuleRequest>();
-      CHECK(v8_str("./foo.js")->StrictEquals(module_request_0->GetSpecifier()));
-      int offset = module_request_0->GetSourceOffset();
-      CHECK_EQ(7, offset);
-      Location loc = module->SourceOffsetToLocation(offset);
-      CHECK_EQ(0, loc.GetLineNumber());
-      CHECK_EQ(7, loc.GetColumnNumber());
-      // No supported assertions were provided in the Isolate's CreateParams, so
-      // no import assertions should be visible on the API surface.
-      CHECK_EQ(0, module_request_0->GetImportAssertions()->Length());
-    }
-
-    // foo.js
-    {
-      Local<String> source_text = v8_str("Object.expando = 40");
-      ScriptOrigin origin = ModuleOrigin(v8_str("foo.js"), CcTest::isolate());
-      ScriptCompiler::Source source(source_text, origin);
-      fooModule =
-          ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
-    }
-
     CHECK(module
               ->InstantiateModule(env.local(),
                                   ResolveCallbackWithImportAssertions)
@@ -335,7 +262,7 @@ TEST(ModuleInstantiationWithImportAssertionsWithoutSupportedAssertions) {
       CHECK(promise->Result()->IsUndefined());
     } else {
       CHECK(!result.IsEmpty());
-      ExpectInt32("Object.expando", 40);
+      ExpectInt32("Object.expando", 42);
     }
     CHECK(!try_catch.HasCaught());
   }