[MERGE #5238 @rhuanjl] Fix module Load order and re-fix circular loading logic

Merge pull request #5238 from rhuanjl:loadOrder

Fixes #5171  but this may take a bit of explaining.

@boingoing please could you take a look at this?

Note I've targeted this at master - though would prefer to target 1.8 if allowed as this is fixing a regression that occurred in 1.8 I've targeted master as I'm assuming 1.8 is closed to functionality changes at this point.

This PR reverts 2dc946c which apparently fixed some circular module loading bugs by reversing module load order.

However the reversed load order resulted in observably different runtime results in some cases - introducing an incompatibility with other runtimes described in issue #5171

**As far as I understand the purpose of the order reversal was to resolve problems where circular imports were not handled correctly, these were:**
1. Issue #4482
But since then #4482 was fixed properly in #4583 - after noting that the order reversal fixed the original POC but did not fix a slightly revised version of the same.
2. A non-specific issue that had resulted in the test-case bug-issue-3257 being disabled - this is an issue that can only occur with either an environment that allows dynamic imports OR an environment that allows multiple root modules to be being loaded at the same time in the same context.

This PR includes an alternative fix for the issue that caused bug-issue-3257 to be disabled. And adds a test case that reproduces the same bug regardless of the load order.

The revised fix for that was to get rid of the concept of numPendingModules that was used to track when all of a module's children were ready.

**Previous logic:**
1. Module is parsed that has children
2. For each child:
- if child already parsed do nothing
- if child not already parsed increment numPendingModules for the parent and add parent to list of modules to notify when child is parsed
3. When a module is parsed that has no pending children notify any parents in its list
4. When a module is notified by a child decrease numPendingModules for it by 1, when it reaches 0 notify its parent OR if it's a root module or a dynamicly imported module instantiate it and notify the host to execute it

**Problem with previous logic**
1. Mod0 parsed, has children Mod1 and Mod2 (schedule async parse job for Mod1 and Mod2)
2. Mod1 also a root module or dynamic module and has Mod0 as a child
3. parse job for Mod1 completes -> notes that Mod0 is already parsed so numPendingModules = 0
4. Host notified to execute Mod1 before Mod2 has been parsed

**New logic**
1. Module is parsed that has children
2. For each child:
- add parent to list of modules to notify when child is parsed
3. Whenever a module is parsed OR a notification is received:
- recursively check if all children and children's children etc. have been parsed
- if yes notify parent OR if dynamic/root module -> notify host to execute

Author: boingoing

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>