vm: hint module identifier in instantiate errors

PR-URL: https://github.com/nodejs/node/pull/60199
Fixes: https://github.com/nodejs/node/issues/60157
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

src/module_wrap.cc

@@ -144,12 +144,12 @@ ModuleWrap::ModuleWrap(Realm* realm,
                        Local<Object> context_object,
                        Local<Value> synthetic_evaluation_step)
     : BaseObject(realm, object),
+      url_(Utf8Value(realm->isolate(), url).ToString()),
       module_(realm->isolate(), module),
       module_hash_(module->GetIdentityHash()) {
   realm->env()->hash_to_module_map.emplace(module_hash_, this);
 
   object->SetInternalField(kModuleSlot, module);
-  object->SetInternalField(kURLSlot, url);
   object->SetInternalField(kModuleSourceObjectSlot,
                            v8::Undefined(realm->isolate()));
   object->SetInternalField(kSyntheticEvaluationStepsSlot,
@@ -968,8 +968,7 @@ void ModuleWrap::GetModuleSourceObject(
       obj->object()->GetInternalField(kModuleSourceObjectSlot).As<Value>();
 
   if (module_source_object->IsUndefined()) {
-    Local<String> url = obj->object()->GetInternalField(kURLSlot).As<String>();
+    THROW_ERR_SOURCE_PHASE_NOT_DEFINED(isolate, obj->url_);
-    THROW_ERR_SOURCE_PHASE_NOT_DEFINED(isolate, url);
     return;
   }
 
@@ -1043,10 +1042,8 @@ MaybeLocal<Object> ModuleWrap::ResolveSourceCallback(
           ->GetInternalField(ModuleWrap::kModuleSourceObjectSlot)
           .As<Value>();
   if (module_source_object->IsUndefined()) {
-    Local<String> url = resolved_module->object()
+    THROW_ERR_SOURCE_PHASE_NOT_DEFINED(Isolate::GetCurrent(),
-                            ->GetInternalField(ModuleWrap::kURLSlot)
+                                       resolved_module->url_);
-                            .As<String>();
-    THROW_ERR_SOURCE_PHASE_NOT_DEFINED(Isolate::GetCurrent(), url);
     return {};
   }
   CHECK(module_source_object->IsObject());
@@ -1078,17 +1075,21 @@ Maybe<ModuleWrap*> ModuleWrap::ResolveModule(
     return Nothing<ModuleWrap*>();
   }
   if (!dependent->IsLinked()) {
-    THROW_ERR_VM_MODULE_LINK_FAILURE(
+    THROW_ERR_VM_MODULE_LINK_FAILURE(env,
-        env,
+                                     "request for '%s' can not be resolved on "
-        "request for '%s' is from a module not been linked",
+                                     "module '%s' that is not linked",
-        cache_key.specifier);
+                                     cache_key.specifier,
+                                     dependent->url_);
     return Nothing<ModuleWrap*>();
   }
 
   auto it = dependent->resolve_cache_.find(cache_key);
   if (it == dependent->resolve_cache_.end()) {
     THROW_ERR_VM_MODULE_LINK_FAILURE(
-        env, "request for '%s' is not in cache", cache_key.specifier);
+        env,
+        "request for '%s' is not cached on module '%s'",
+        cache_key.specifier,
+        dependent->url_);
     return Nothing<ModuleWrap*>();
   }
 

src/module_wrap.h

@@ -99,7 +99,6 @@ class ModuleWrap : public BaseObject {
  public:
   enum InternalFields {
     kModuleSlot = BaseObject::kInternalFieldCount,
-    kURLSlot,
     kModuleSourceObjectSlot,
     kSyntheticEvaluationStepsSlot,
     kContextObjectSlot,   // Object whose creation context is the target Context
@@ -215,6 +214,7 @@ class ModuleWrap : public BaseObject {
       v8::Local<v8::FixedArray> import_attributes,
       v8::Local<v8::Module> referrer);
 
+  std::string url_;
   v8::Global<v8::Module> module_;
   ResolveCache resolve_cache_;
   contextify::ContextifyContext* contextify_context_ = nullptr;

src/node_errors.h

@@ -295,12 +295,11 @@ inline v8::Local<v8::Object> ERR_BUFFER_TOO_LARGE(v8::Isolate* isolate) {
 }
 
 inline void THROW_ERR_SOURCE_PHASE_NOT_DEFINED(v8::Isolate* isolate,
-                                               v8::Local<v8::String> url) {
+                                               const std::string& url) {
-  std::string message = std::string(*v8::String::Utf8Value(isolate, url));
   return THROW_ERR_SOURCE_PHASE_NOT_DEFINED(
       isolate,
-      "Source phase import object is not defined for module %s",
+      "Source phase import object is not defined for module '%s'",
-      message.c_str());
+      url);
 }
 
 inline v8::Local<v8::Object> ERR_STRING_TOO_LONG(v8::Isolate* isolate) {

test/parallel/test-vm-module-linkmodulerequests.js

@@ -65,3 +65,14 @@ test('mismatch linkage', () => {
     code: 'ERR_MODULE_LINK_MISMATCH',
   });
 });
+
+test('instantiate error should hint about module identifier', () => {
+  const foo = new SourceTextModule('import bar from "bar"', { identifier: 'file://foo' });
+  const bar = new SourceTextModule('import "unknown"', { identifier: 'file://bar' });
+
+  foo.linkRequests([bar]);
+  assert.throws(() => foo.instantiate(), {
+    message: `request for 'unknown' can not be resolved on module 'file://bar' that is not linked`,
+    code: 'ERR_VM_MODULE_LINK_FAILURE',
+  });
+});