Combine module Url and Specifier properties (#6446)

* Combined module Url and Specifier properties

* Internalise Root Module Detection

bin/ch/WScriptJsrt.cpp

@@ -600,7 +600,7 @@ Error:
     return returnValue;
 }
 
-JsErrorCode WScriptJsrt::InitializeModuleInfo(JsValueRef specifier, JsModuleRecord moduleRecord)
+JsErrorCode WScriptJsrt::InitializeModuleInfo(JsModuleRecord moduleRecord)
 {
     JsErrorCode errorCode = JsNoError;
     errorCode = ChakraRTInterface::JsSetModuleHostInfo(moduleRecord, JsModuleHostInfo_FetchImportedModuleCallback, (void*)WScriptJsrt::FetchImportedModule);
@@ -616,11 +616,6 @@ JsErrorCode WScriptJsrt::InitializeModuleInfo(JsValueRef specifier, JsModuleReco
             if (errorCode == JsNoError)
             {
                 errorCode = ChakraRTInterface::JsSetModuleHostInfo(moduleRecord, JsModuleHostInfo_InitializeImportMetaCallback, (void*)WScriptJsrt::InitializeImportMetaCallback);
-
-                if (errorCode == JsNoError && moduleRecord != nullptr)
-                {
-                    errorCode = ChakraRTInterface::JsSetModuleHostInfo(moduleRecord, JsModuleHostInfo_HostDefined, specifier);
-                }
             }
         }
     }
@@ -661,9 +656,12 @@ JsErrorCode WScriptJsrt::LoadModuleFromString(LPCSTR fileName, LPCSTR fileConten
     // otherwise we'll use the old one.
     if (moduleRecordEntry == moduleRecordMap.end())
     {
-        JsValueRef specifier;
-        errorCode = ChakraRTInterface::JsCreateString(
-            fileName, strlen(fileName), &specifier);
+        JsValueRef specifier = nullptr;
+        if (isFile && fullName)
+        {
+            errorCode = ChakraRTInterface::JsCreateString(
+                fullName, strlen(fullName), &specifier);
+        }
         if (errorCode == JsNoError)
         {
             errorCode = ChakraRTInterface::JsInitializeModuleRecord(
@@ -671,7 +669,7 @@ JsErrorCode WScriptJsrt::LoadModuleFromString(LPCSTR fileName, LPCSTR fileConten
         }
         if (errorCode == JsNoError)
         {
-            errorCode = InitializeModuleInfo(specifier, requestModule);
+            errorCode = InitializeModuleInfo(requestModule);
         }
         if (errorCode == JsNoError)
         {
@@ -694,14 +692,6 @@ JsErrorCode WScriptJsrt::LoadModuleFromString(LPCSTR fileName, LPCSTR fileConten
     // ParseModuleSource is sync, while additional fetch & evaluation are async.
     unsigned int fileContentLength = (fileContent == nullptr) ? 0 : (unsigned int)strlen(fileContent);
  
-    if (isFile && fullName)
-    {
-        JsValueRef moduleUrl;
-        ChakraRTInterface::JsCreateString(fullName, strlen(fullName), &moduleUrl);
-        errorCode = ChakraRTInterface::JsSetModuleHostInfo(requestModule, JsModuleHostInfo_Url, moduleUrl);
-        IfJsrtErrorFail(errorCode, errorCode);
-    }
- 
     errorCode = ChakraRTInterface::JsParseModuleSource(requestModule, dwSourceCookie, (LPBYTE)fileContent,
         fileContentLength, JsParseModuleSourceFlags_DataIsUTF8, &errorObject);
     if ((errorCode != JsNoError) && errorObject != JS_INVALID_REFERENCE && fileContent != nullptr && !HostConfigFlags::flags.IgnoreScriptErrorCode && moduleErrMap[requestModule] == RootModule)
@@ -1230,7 +1220,7 @@ bool WScriptJsrt::Initialize()
     IfJsrtErrorFail(CreatePropertyIdFromString("console", &consoleName), false);
     IfJsrtErrorFail(ChakraRTInterface::JsSetProperty(global, consoleName, console, true), false);
 
-    IfJsrtErrorFail(InitializeModuleCallbacks(), false);
+    IfJsrtErrorFail(InitializeModuleInfo(nullptr), false);
 
     // When the command-line argument `-Test262` is set,
     // WScript will have the extra support API below and $262 will be
@@ -1261,11 +1251,6 @@ Error:
     return hr == S_OK;
 }
 
-JsErrorCode WScriptJsrt::InitializeModuleCallbacks()
-{
-    return InitializeModuleInfo(nullptr, nullptr);
-}
-
 bool WScriptJsrt::Uninitialize()
 {
     // moduleRecordMap is a global std::map, its destructor may access overridden
@@ -2085,7 +2070,7 @@ JsErrorCode WScriptJsrt::FetchImportedModuleHelper(JsModuleRecord referencingMod
     if (errorCode == JsNoError)
     {
         GetDir(fullPath, &moduleDirMap[moduleRecord]);
-        InitializeModuleInfo(specifier, moduleRecord);
+        InitializeModuleInfo(moduleRecord);
         std::string pathKey = std::string(fullPath);
         moduleRecordMap[pathKey] = moduleRecord;
         moduleErrMap[moduleRecord] = ImportedModule;
@@ -2134,7 +2119,7 @@ JsErrorCode WScriptJsrt::NotifyModuleReadyCallback(_In_opt_ JsModuleRecord refer
     {
         ChakraRTInterface::JsSetException(exceptionVar);
         JsValueRef specifier = JS_INVALID_REFERENCE;
-        ChakraRTInterface::JsGetModuleHostInfo(referencingModule, JsModuleHostInfo_HostDefined, &specifier);
+        ChakraRTInterface::JsGetModuleHostInfo(referencingModule, JsModuleHostInfo_Url, &specifier);
         AutoString fileName;
         if (specifier != JS_INVALID_REFERENCE)
         {
@@ -2170,7 +2155,7 @@ JsErrorCode __stdcall WScriptJsrt::InitializeImportMetaCallback(_In_opt_ JsModul
     if (importMetaVar != nullptr)
     {
         JsValueRef specifier = JS_INVALID_REFERENCE;
-        ChakraRTInterface::JsGetModuleHostInfo(referencingModule, JsModuleHostInfo_HostDefined, &specifier);
+        ChakraRTInterface::JsGetModuleHostInfo(referencingModule, JsModuleHostInfo_Url, &specifier);
 
         JsPropertyIdRef urlPropId;
         if (JsNoError == CreatePropertyIdFromString("url", &urlPropId))

bin/ch/WScriptJsrt.h

@@ -66,7 +66,6 @@ public:
     static JsErrorCode FetchImportedModuleFromScript(_In_ DWORD_PTR dwReferencingSourceContext, _In_ JsValueRef specifier, _Outptr_result_maybenull_ JsModuleRecord* dependentModuleRecord);
     static JsErrorCode NotifyModuleReadyCallback(_In_opt_ JsModuleRecord referencingModule, _In_opt_ JsValueRef exceptionVar);
     static JsErrorCode CALLBACK InitializeImportMetaCallback(_In_opt_ JsModuleRecord referencingModule, _In_opt_ JsValueRef importMetaVar);
-    static JsErrorCode InitializeModuleCallbacks();
     static void CALLBACK PromiseContinuationCallback(JsValueRef task, void *callbackState);
     static void CALLBACK PromiseRejectionTrackerCallback(JsValueRef promise, JsValueRef reason, bool handled, void *callbackState);
 
@@ -130,7 +129,7 @@ private:
 
     static JsValueRef CALLBACK EmptyCallback(JsValueRef callee, bool isConstructCall, JsValueRef *arguments, unsigned short argumentCount, void *callbackState);
     static JsErrorCode CALLBACK LoadModuleFromString(LPCSTR fileName, LPCSTR fileContent, LPCSTR fullName = nullptr, bool isFile = false);
-    static JsErrorCode CALLBACK InitializeModuleInfo(JsValueRef specifier, JsModuleRecord moduleRecord);
+    static JsErrorCode CALLBACK InitializeModuleInfo(JsModuleRecord moduleRecord);
 
     static JsValueRef CALLBACK LoadBinaryFileCallback(JsValueRef callee, bool isConstructCall, JsValueRef *arguments, unsigned short argumentCount, void *callbackState);
     static JsValueRef CALLBACK LoadTextFileCallback(JsValueRef callee, bool isConstructCall, JsValueRef *arguments, unsigned short argumentCount, void *callbackState);

lib/Jsrt/ChakraCore.h

@@ -319,17 +319,16 @@ JsCreateEnhancedFunction(
 /// <remarks>
 ///     Bootstrap the module loading process by creating a new module record.
 /// </remarks>
-/// <param name="referencingModule">The parent module of the new module - nullptr for a root module.</param>
-/// <param name="normalizedSpecifier">The normalized specifier for the module.</param>
-/// <param name="moduleRecord">The new module record. The host should not try to call this API twice
-///                            with the same normalizedSpecifier.</param>
+/// <param name="referencingModule">Unused parameter - exists for backwards compatability, supply nullptr</param>
+/// <param name="normalizedSpecifier">The normalized specifier or url for the module - used in script errors, optional.</param>
+/// <param name="moduleRecord">The new module record.</param>
 /// <returns>
 ///     The code <c>JsNoError</c> if the operation succeeded, a failure code otherwise.
 /// </returns>
 CHAKRA_API
 JsInitializeModuleRecord(
     _In_opt_ JsModuleRecord referencingModule,
-    _In_ JsValueRef normalizedSpecifier,
+    _In_opt_ JsValueRef normalizedSpecifier,
     _Outptr_result_maybenull_ JsModuleRecord* moduleRecord);
 
 /// <summary>

lib/Jsrt/Core/JsrtCore.cpp

@@ -21,28 +21,21 @@
 CHAKRA_API
 JsInitializeModuleRecord(
     _In_opt_ JsModuleRecord referencingModule,
-    _In_ JsValueRef normalizedSpecifier,
+    _In_opt_ JsValueRef normalizedSpecifier,
     _Outptr_result_maybenull_ JsModuleRecord* moduleRecord)
 {
     PARAM_NOT_NULL(moduleRecord);
 
-    Js::SourceTextModuleRecord* childModuleRecord = nullptr;
+    Js::SourceTextModuleRecord* newModuleRecord = nullptr;
 
     JsErrorCode errorCode = ContextAPIWrapper_NoRecord<true>([&](Js::ScriptContext *scriptContext) -> JsErrorCode {
-        childModuleRecord = Js::SourceTextModuleRecord::Create(scriptContext);
-        if (referencingModule == nullptr)
-        {
-            childModuleRecord->SetIsRootModule();
-        }
-        if (normalizedSpecifier != JS_INVALID_REFERENCE)
-        {
-            childModuleRecord->SetSpecifier(normalizedSpecifier);
-        }
+        newModuleRecord = Js::SourceTextModuleRecord::Create(scriptContext);
+        newModuleRecord->SetSpecifier(normalizedSpecifier);
         return JsNoError;
     });
     if (errorCode == JsNoError)
     {
-        *moduleRecord = childModuleRecord;
+        *moduleRecord = newModuleRecord;
     }
     else
     {
@@ -85,9 +78,9 @@ JsParseModuleSource(
         {
             const char16 *moduleUrlSz = nullptr;
             size_t moduleUrlLen = 0;
-            if (moduleRecord->GetModuleUrl())
+            if (moduleRecord->GetSpecifier())
             {
-                Js::JavascriptString *moduleUrl = Js::VarTo<Js::JavascriptString>(moduleRecord->GetModuleUrl());
+                Js::JavascriptString *moduleUrl = Js::VarTo<Js::JavascriptString>(moduleRecord->GetSpecifier());
                 moduleUrlSz = moduleUrl->GetSz();
                 moduleUrlLen = moduleUrl->GetLength();
             }
@@ -196,7 +189,7 @@ JsSetModuleHostInfo(
             currentContext->GetHostScriptContext()->SetInitializeImportMetaCallback(reinterpret_cast<InitializeImportMetaCallback>(hostInfo));
             break;
         case JsModuleHostInfo_Url:
-            moduleRecord->SetModuleUrl(hostInfo);
+            moduleRecord->SetSpecifier(hostInfo);
             break;
         default:
             return JsInvalidModuleHostInfoKind;
@@ -246,7 +239,7 @@ JsGetModuleHostInfo(
             *hostInfo = reinterpret_cast<void*>(currentContext->GetHostScriptContext()->GetInitializeImportMetaCallback());
             break;
         case JsModuleHostInfo_Url:
-            *hostInfo = reinterpret_cast<void*>(moduleRecord->GetModuleUrl());
+            *hostInfo = reinterpret_cast<void*>(moduleRecord->GetSpecifier());
             break;
         default:
             return JsInvalidModuleHostInfoKind;

lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -31,7 +31,6 @@ namespace Js
         localExportMapByLocalName(nullptr),
         localExportIndexList(nullptr),
         normalizedSpecifier(nullptr),
-        moduleUrl(nullptr),
         errorObject(nullptr),
         hostDefined(nullptr),
         exportedNames(nullptr),
@@ -108,6 +107,13 @@ namespace Js
             return E_NOTIMPL;
         }
 
+        // Mark module as root module if it currently has no parents
+        // Note, if there are circular imports it may gain parents later
+        if (parentModuleList == nullptr)
+        {
+            SetIsRootModule();
+        }
+
         // Host indicates that the current module failed to load.
         if (sourceText == nullptr)
         {
@@ -183,11 +189,8 @@ namespace Js
         {
             if (*exceptionVar == nullptr)
             {
-                const WCHAR * sourceUrl = nullptr;
-                if (this->GetModuleUrl())
-                {
-                  sourceUrl = this->GetModuleUrlSz();
-                }
+                const WCHAR * sourceUrl = this->GetSpecifierSz();
+
                 *exceptionVar = JavascriptError::CreateFromCompileScriptException(scriptContext, &se, sourceUrl);
             }
             // Cleanup in case of error.
@@ -208,8 +211,8 @@ namespace Js
                 SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->errorObject, this->scriptContext, this, false);
             }
 
-            // Notify host if current module is dynamically-loaded module, or is root module and the host hasn't been notified
-            if (this->promise != nullptr || (isRootModule && !hadNotifyHostReady))
+            // Notify host if current module is root module and the host hasn't been notified
+            if (isRootModule && !hadNotifyHostReady)
             {
                 OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyHostAboutModuleReady %s (ParseSource error)\n"), this->GetSpecifierSz());
                 LEAVE_SCRIPT_IF_ACTIVE(scriptContext,
@@ -371,7 +374,7 @@ namespace Js
             OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded\n"));
             NotifyParentsAsNeeded();
 
-            if (!WasDeclarationInitialized() && (isRootModule || this->promise != nullptr))
+            if (!WasDeclarationInitialized() && isRootModule)
             {
                 // TODO: move this as a promise call? if parser is called from a different thread
                 // We'll need to call the bytecode gen in the main thread as we are accessing GC.
@@ -427,7 +430,7 @@ namespace Js
                 SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->errorObject, this->scriptContext, this, false);
             }
 
-            if (this->promise != nullptr || (isRootModule && !hadNotifyHostReady))
+            if (isRootModule && !hadNotifyHostReady)
             {
                 OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyHostAboutModuleReady %s (OnChildModuleReady)\n"), this->GetSpecifierSz());
                 LEAVE_SCRIPT_IF_ACTIVE(scriptContext,
@@ -947,11 +950,8 @@ namespace Js
 
         if (rootFunction == nullptr)
         {
-            const WCHAR * sourceUrl = nullptr;
-            if (this->GetModuleUrl())
-            {
-                sourceUrl = this->GetModuleUrlSz();
-            }
+            const WCHAR * sourceUrl = this->GetSpecifierSz();
+
             this->errorObject = JavascriptError::CreateFromCompileScriptException(scriptContext, &se, sourceUrl);
             OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentAsNeeded rootFunction == nullptr\n"));
             NotifyParentsAsNeeded();

lib/Runtime/Language/SourceTextModuleRecord.h

@@ -52,11 +52,11 @@ namespace Js
 
         void SetSpecifier(Var specifier) { this->normalizedSpecifier = specifier; }
         Var GetSpecifier() const { return normalizedSpecifier; }
-        const char16 *GetSpecifierSz() const { return VarTo<JavascriptString>(this->normalizedSpecifier)->GetSz(); }
-
-        void SetModuleUrl(Var moduleUrl) { this->moduleUrl = moduleUrl; }
-        Var GetModuleUrl() const { return moduleUrl;}
-        const char16 *GetModuleUrlSz() const { return VarTo<JavascriptString>(this->moduleUrl)->GetSz(); }
+        const char16 *GetSpecifierSz() const
+        {
+            return this->normalizedSpecifier != nullptr ? 
+                VarTo<JavascriptString>(this->normalizedSpecifier)->GetSz() : _u("module"); 
+        }
 
         Var GetErrorObject() const { return errorObject; }
 
@@ -152,7 +152,6 @@ namespace Js
 
         Field(Js::JavascriptFunction*) rootFunction;
         Field(void*) hostDefined;
-        Field(Var) moduleUrl;
         Field(Var) normalizedSpecifier;
         Field(Var) errorObject;
         Field(Field(Var)*) localExportSlots;