[1.8>1.9] [MERGE #4574 @obastemur] module: fix re-entrant module impo…

…rting Merge pull request #4574 from obastemur:fix_reent
chakra-core · Jan 19, 2018 · 4e2496a · 4e2496a
2 parents 11715ee + cb4a35c
commit 4e2496a
Show file tree

Hide file tree

Showing 7 changed files with 53 additions and 18 deletions.
diff --git a/lib/Runtime/Language/SourceTextModuleRecord.cpp b/lib/Runtime/Language/SourceTextModuleRecord.cpp
@@ -722,8 +722,8 @@ namespace Js
                     Recycler* recycler = GetScriptContext()->GetRecycler();
                     this->parentModuleList = RecyclerNew(recycler, ModuleRecordList, recycler);
                 }
-                bool contains = this->parentModuleList->Contains(parentRecord);
-                if (!contains)
+
+                if (!this->parentModuleList->Contains(parentRecord))
                 {
                     this->parentModuleList->Add(parentRecord);
                     parentRecord->numPendingChildrenModule++;
@@ -751,10 +751,18 @@ 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;
-                LPCOLESTR moduleName = specifier->Psz();
                 bool itemFound = childrenModuleSet->TryGetValue(moduleName, &moduleRecord);
                 if (!itemFound)
                 {
@@ -779,6 +787,8 @@ namespace Js
                 }
                 return false;
             });
+            moduleRecords->Clear();
+
             if (FAILED(hr))
             {
                 if (this->errorObject == nullptr)
@@ -827,20 +837,15 @@ namespace Js
             SetWasDeclarationInitialized();
             if (childrenModuleSet != nullptr)
             {
-                childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
+                childrenModuleSet->EachValue([=](SourceTextModuleRecord* childModuleRecord)
                 {
-                    Assert(moduleRecord->WasParsed());
-                    moduleRecord->shouldGenerateRootFunction =
-                            moduleRecord->ModuleDeclarationInstantiation();
+                    Assert(childModuleRecord->WasParsed());
+                    childModuleRecord->ModuleDeclarationInstantiation();
                 });
 
-                childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
+                childrenModuleSet->EachValue([=](SourceTextModuleRecord* childModuleRecord)
                 {
-                    if (moduleRecord->shouldGenerateRootFunction)
-                    {
-                        moduleRecord->shouldGenerateRootFunction = false;
-                        moduleRecord->GenerateRootFunction();
-                    }
+                    childModuleRecord->GenerateRootFunction();
                 });
             }
 
@@ -866,6 +871,13 @@ namespace Js
 
     void SourceTextModuleRecord::GenerateRootFunction()
     {
+        // On cyclic dependency, we may endup generating the root function twice
+        // so make sure we don't
+        if (this->rootFunction != nullptr)
+        {
+            return;
+        }
+
         ScriptContext* scriptContext = GetScriptContext();
         Js::AutoDynamicCodeReference dynamicFunctionReference(scriptContext);
         CompileScriptException se;

diff --git a/lib/Runtime/Language/SourceTextModuleRecord.h b/lib/Runtime/Language/SourceTextModuleRecord.h
@@ -117,7 +117,6 @@ namespace Js
         // TODO: move non-GC fields out to avoid false reference?
         // This is the parsed tree resulted from compilation.
         Field(bool) wasParsed;
-        Field(bool) shouldGenerateRootFunction;
         Field(bool) wasDeclarationInitialized;
         Field(bool) parentsNotified;
         Field(bool) isRootModule;
@@ -178,4 +177,4 @@ namespace Js
         uint moduleId;
         Field(Var)* localExportSlotsAddr;
     };
-}
+}
diff --git a/test/es6/bug_issue_3257_mod/mod0.js b/test/es6/bug_issue_3257_mod/mod0.js
@@ -3,7 +3,7 @@
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
-import '../bug_issue_3257_mod1.js';
-import '../bug_issue_3257_mod2/mod2.js';
+import 'bug_issue_3257_mod1.js';
+import 'bug_issue_3257_mod2/mod2.js';
 
 console.log("mod0");
diff --git a/test/es6/bug_issue_3257_mod2/mod2.js b/test/es6/bug_issue_3257_mod2/mod2.js
@@ -3,5 +3,5 @@
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
-import '../bug_issue_3257_mod/mod0.js';
+import 'bug_issue_3257_mod/mod0.js';
 console.log("mod2");
diff --git a/test/es6/module-bugfixes.js b/test/es6/module-bugfixes.js
@@ -61,6 +61,13 @@ var tests = [
             testRunner.LoadModule(functionBody);
         }
     },
+    {
+        name: "Issue 4570: Module that appears multiple times in dependency tree",
+        body: function() {
+            let functionBody = "import 'module_4570_dep1.js';"
+            testRunner.LoadModule(functionBody);
+        }
+    }
 ];
 
 testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
diff --git a/test/es6/module_4570_dep1.js b/test/es6/module_4570_dep1.js
@@ -0,0 +1,11 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+//tests import of module that has two different names
+import Thing1 from './module_4570_dep2.js';
+import Thing2 from '././module_4570_dep2.js';
+
+Thing1();
+Thing2();
diff --git a/test/es6/module_4570_dep2.js b/test/es6/module_4570_dep2.js
@@ -0,0 +1,6 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+export default function(){}