Correct module load order. Fixes: #5171

And fix logic for circular dependencies to handle to multiple root modules.

Author: rhuanjl

lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -42,7 +42,6 @@ namespace Js
         isRootModule(false),
         hadNotifyHostReady(false),
         localExportSlots(nullptr),
-        numPendingChildrenModule(0),
         moduleId(InvalidModuleIndex),
         localSlotCount(InvalidSlotCount),
         promise(nullptr),
@@ -225,6 +224,11 @@ namespace Js
 
     void SourceTextModuleRecord::NotifyParentsAsNeeded()
     {
+        if (notifying)
+        {
+            return;
+        }
+        notifying = true;
         // Notify the parent modules that this child module is either in fault state or finished.
         if (this->parentModuleList != nullptr)
         {
@@ -233,6 +237,7 @@ namespace Js
                 parentModule->OnChildModuleReady(this, this->errorObject);
             });
         }
+        notifying = false;
         SetParentsNotified();
     }
 
@@ -326,7 +331,7 @@ namespace Js
         OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("PrepareForModuleDeclarationInitialization(%s)\n"), this->GetSpecifierSz());
         HRESULT hr = NO_ERROR;
 
-        if (numPendingChildrenModule == 0)
+        if (ConfirmChildrenParsed())
         {
             OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded\n"));
             NotifyParentsAsNeeded();
@@ -355,12 +360,6 @@ namespace Js
                 }
             }
         }
-#if DBG
-        else
-        {
-            OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\tnumPendingChildrenModule=(%d)\n"), numPendingChildrenModule);
-        }
-#endif
         return hr;
     }
 
@@ -399,12 +398,6 @@ namespace Js
         }
         else
         {
-            if (numPendingChildrenModule == 0)
-            {
-                return NOERROR; // this is only in case of recursive module reference. Let the higher stack frame handle this module.
-            }
-            numPendingChildrenModule--;
-
             hr = PrepareForModuleDeclarationInitialization();
         }
         return hr;
@@ -715,21 +708,52 @@ namespace Js
         {
             parentRecord->childrenModuleSet->AddNew(moduleName, this);
 
-            if (!this->WasParsed())
+            if (this->parentModuleList == nullptr)
             {
-                if (this->parentModuleList == nullptr)
+                Recycler* recycler = GetScriptContext()->GetRecycler();
+                this->parentModuleList = RecyclerNew(recycler, ModuleRecordList, recycler);
+            }
+
+            if (!this->parentModuleList->Contains(parentRecord))
+            {
+                this->parentModuleList->Add(parentRecord);
+            }
+        }
+    }
+
+    bool SourceTextModuleRecord::ConfirmChildrenParsed()
+    {
+        if (!this->WasParsed())
+        {
+            return false;
+        }
+        if (confirmedReady || this->ParentsNotified())
+        {
+            return true;
+        }
+        bool result = true;
+        confirmedReady = true;
+        EnsureChildModuleSet(GetScriptContext());
+        childrenModuleSet->EachValue([&](SourceTextModuleRecord* childModuleRecord) {
+            if (childModuleRecord->ParentsNotified())
+            {
+                return false;
+            }
+            else
+            {
+                if (childModuleRecord->ConfirmChildrenParsed())
                 {
-                    Recycler* recycler = GetScriptContext()->GetRecycler();
-                    this->parentModuleList = RecyclerNew(recycler, ModuleRecordList, recycler);
+                    return false;
                 }
-
-                if (!this->parentModuleList->Contains(parentRecord))
+                else
                 {
-                    this->parentModuleList->Add(parentRecord);
-                    parentRecord->numPendingChildrenModule++;
+                    result = false;
+                    return true;
                 }
             }
-        }
+        });
+        confirmedReady = false;
+        return result;
     }
 
     void SourceTextModuleRecord::EnsureChildModuleSet(ScriptContext *scriptContext)
@@ -751,16 +775,9 @@ namespace Js
         if (requestedModuleList != nullptr)
         {
             EnsureChildModuleSet(scriptContext);
-            ArenaAllocator* allocator = scriptContext->GeneralAllocator();
-            SList<LPCOLESTR> * moduleRecords = Anew(allocator, SList<LPCOLESTR>, allocator);
 
-            // Reverse the order for the host. So, host can read the files top-down
             requestedModuleList->MapUntil([&](IdentPtr specifier) {
                 LPCOLESTR moduleName = specifier->Psz();
-                return !moduleRecords->Prepend(moduleName);
-            });
-
-            moduleRecords->MapUntil([&](LPCOLESTR moduleName) {
                 ModuleRecordBase* moduleRecordBase = nullptr;
                 SourceTextModuleRecord* moduleRecord = nullptr;
                 bool itemFound = childrenModuleSet->TryGetValue(moduleName, &moduleRecord);
@@ -787,7 +804,6 @@ namespace Js
                 }
                 return false;
             });
-            moduleRecords->Clear();
 
             if (FAILED(hr))
             {

lib/Runtime/Language/SourceTextModuleRecord.h

@@ -43,6 +43,7 @@ namespace Js
 
         HRESULT ResolveExternalModuleDependencies();
         void EnsureChildModuleSet(ScriptContext *scriptContext);
+        bool ConfirmChildrenParsed();
 
         void* GetHostDefined() const { return hostDefined; }
         void SetHostDefined(void* hostObj) { hostDefined = hostObj; }
@@ -116,6 +117,8 @@ namespace Js
         const static uint InvalidSlotIndex = 0xffffffff;
         // TODO: move non-GC fields out to avoid false reference?
         // This is the parsed tree resulted from compilation.
+        Field(bool) confirmedReady = false;
+        Field(bool) notifying = false;
         Field(bool) wasParsed;
         Field(bool) wasDeclarationInitialized;
         Field(bool) parentsNotified;
@@ -136,7 +139,6 @@ namespace Js
         Field(LocalExportMap*) localExportMapByExportName;  // from propertyId to index map: for bytecode gen.
         Field(LocalExportMap*) localExportMapByLocalName;  // from propertyId to index map: for bytecode gen.
         Field(LocalExportIndexList*) localExportIndexList; // from index to propertyId: for typehandler.
-        Field(uint) numPendingChildrenModule;
         Field(ExportedNames*) exportedNames;
         Field(ResolvedExportMap*) resolvedExportMap;
 

test/es6module/module-bugfixes.js

@@ -67,6 +67,32 @@ var tests = [
             let functionBody = "import 'module_4570_dep1.js';"
             testRunner.LoadModule(functionBody);
         }
+    },
+    {
+        name: "Issue 5171: Incorrect module load order",
+        body: function() {
+            WScript.RegisterModuleSource("obj.js", `export const obj = {a:false, b: false, c: false};`);
+            WScript.RegisterModuleSource("a.js",`
+                import {obj} from "obj.js";
+                assert.isTrue(obj.b);
+                assert.isFalse(obj.c);
+                assert.isFalse(obj.a);
+                obj.a = true;`);
+            WScript.RegisterModuleSource("b.js",`
+                import {obj} from "obj.js";
+                assert.isFalse(obj.b);
+                assert.isFalse(obj.c);
+                assert.isFalse(obj.a);
+                obj.b = true;`);
+            WScript.RegisterModuleSource("c.js",`
+                import {obj} from "obj.js";
+                assert.isTrue(obj.b);
+                assert.isFalse(obj.c);
+                assert.isTrue(obj.a);
+                obj.c = true;`);
+            const start = 'import "b.js"; import "a.js"; import "c.js";';
+            testRunner.LoadModule(start);
+        }
     }
 ];
 

test/es6module/multiple-roots-circular.js

@@ -0,0 +1,15 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// Tests that circular overlapping module dependencies are all loaded before execution
+
+WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
+
+WScript.RegisterModuleSource("mod0.js", "print('pass')");
+WScript.RegisterModuleSource("mod1.js", "import 'mod2.js';");
+WScript.RegisterModuleSource("mod2.js", "import 'mod0.js';");
+
+WScript.LoadScriptFile("mod2.js", "module");
+WScript.LoadScriptFile("mod1.js", "module");

test/es6module/rlexe.xml

@@ -139,6 +139,12 @@
       <baseline>bug_issue_3257.baseline</baseline>
       <tags>exclude_sanitize_address</tags>
     </default>
+  </test>
+    <test>
+    <default>
+      <files>multiple-roots-circular.js</files>
+      <tags>exclude_sanitize_address</tags>
+    </default>
   </test>
   <test>
     <default>