[1.8>1.9] [MERGE #4551 @obastemur] module: support for circular import

Merge pull request #4551 from obastemur:module_circular Alternative to #4550 , fixes #4482 Saw the test case brought by @rhuanjl to #4482 and the proposed fix at #4550 . After looking at the code, looks like couple of tiny changes are needed to module support. I don't have much context on modules though. This PR is just a weekend after breakfast hacking. /cc @akroshg @boingoing @rhuanjl #### how it works Separate `ModuleDeclarationInstantiation` into `ModuleDeclarationInstantiation` and `GenerateRootFunction`. This way, in case `childrenModuleSet` has circular dependents, we may instantiate all the modules prior to triggering the rest
chakra-core · Jan 16, 2018 · 5983ef6 · 5983ef6
2 parents 3444116 + 6785a49
commit 5983ef6
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 25 deletions.
diff --git a/lib/Runtime/Language/ModuleRecordBase.h b/lib/Runtime/Language/ModuleRecordBase.h
@@ -15,8 +15,8 @@ namespace Js
         ModuleNameRecord(const ModuleNameRecord& other)
             :module(other.module), bindingName(other.bindingName)
         {}
-        ModuleNameRecord(ModuleRecordBase* module, PropertyId bindingName) 
-            :module(module), bindingName(bindingName) 
+        ModuleNameRecord(ModuleRecordBase* module, PropertyId bindingName)
+            :module(module), bindingName(bindingName)
         {}
         ModuleNameRecord() {}
         Field(ModuleRecordBase*) module;
@@ -42,7 +42,8 @@ namespace Js
         // return false when "ambiguous".
         // otherwise nullptr means "null" where we have circular reference/cannot resolve.
         virtual bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord) = 0;
-        virtual void ModuleDeclarationInstantiation() = 0;
+        virtual bool ModuleDeclarationInstantiation() = 0;
+        virtual void GenerateRootFunction() = 0;
         virtual Var ModuleEvaluation() = 0;
         virtual bool IsSourceTextModuleRecord() { return false; }
 

diff --git a/lib/Runtime/Language/SourceTextModuleRecord.cpp b/lib/Runtime/Language/SourceTextModuleRecord.cpp
@@ -277,7 +277,10 @@ namespace Js
             HRESULT hr = NOERROR;
             if (!WasDeclarationInitialized())
             {
-                ModuleDeclarationInstantiation();
+                if (ModuleDeclarationInstantiation())
+                {
+                    GenerateRootFunction();
+                }
 
                 if (this->errorObject != nullptr)
                 {
@@ -336,7 +339,10 @@ namespace Js
                 bool isScriptActive = scriptContext->GetThreadContext()->IsScriptActive();
                 Assert(!isScriptActive || this->promise != nullptr);
 
-                ModuleDeclarationInstantiation();
+                if (ModuleDeclarationInstantiation())
+                {
+                    GenerateRootFunction();
+                }
                 if (!hadNotifyHostReady && !WasEvaluated())
                 {
                     OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyHostAboutModuleReady %s (PrepareForModuleDeclarationInitialization)\n"), this->GetSpecifierSz());
@@ -800,14 +806,14 @@ namespace Js
         // helper information for now.
     }
 
-    void SourceTextModuleRecord::ModuleDeclarationInstantiation()
+    bool SourceTextModuleRecord::ModuleDeclarationInstantiation()
     {
         OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("ModuleDeclarationInstantiation(%s)\n"), this->GetSpecifierSz());
         ScriptContext* scriptContext = GetScriptContext();
 
         if (this->WasDeclarationInitialized())
         {
-            return;
+            return false;
         }
 
         try
@@ -825,7 +831,17 @@ namespace Js
                 childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
                 {
                     Assert(moduleRecord->WasParsed());
-                    moduleRecord->ModuleDeclarationInstantiation();
+                    moduleRecord->shouldGenerateRootFunction =
+                            moduleRecord->ModuleDeclarationInstantiation();
+                });
+
+                childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
+                {
+                    if (moduleRecord->shouldGenerateRootFunction)
+                    {
+                        moduleRecord->shouldGenerateRootFunction = false;
+                        moduleRecord->GenerateRootFunction();
+                    }
                 });
             }
 
@@ -843,12 +859,21 @@ namespace Js
         {
             OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded (errorObject)\n"));
             NotifyParentsAsNeeded();
-            return;
+            return false;
         }
 
+        return true;
+    }
+
+    void SourceTextModuleRecord::GenerateRootFunction()
+    {
+        ScriptContext* scriptContext = GetScriptContext();
         Js::AutoDynamicCodeReference dynamicFunctionReference(scriptContext);
-        Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));
         CompileScriptException se;
+
+        Assert(this->WasDeclarationInitialized());
+        Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));
+
         this->rootFunction = scriptContext->GenerateRootFunction(parseTree, sourceIndex, this->parser, this->pSourceInfo->GetParseFlags(), &se, _u("module"));
         if (rootFunction == nullptr)
         {

diff --git a/lib/Runtime/Language/SourceTextModuleRecord.h b/lib/Runtime/Language/SourceTextModuleRecord.h
@@ -27,11 +27,12 @@ namespace Js
         virtual ExportedNames* GetExportedNames(ExportModuleRecordList* exportStarSet) override;
         virtual bool IsSourceTextModuleRecord() override { return true; } // we don't really have other kind of modulerecord at this time.
 
-        // return false when "ambiguous". 
+        // return false when "ambiguous".
         // otherwise nullptr means "null" where we have circular reference/cannot resolve.
         bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord) override;
         bool ResolveImport(PropertyId localName, ModuleNameRecord** importRecord);
-        void ModuleDeclarationInstantiation() override;
+        bool ModuleDeclarationInstantiation() override;
+        void GenerateRootFunction();
         Var ModuleEvaluation() override;
         virtual ModuleNamespace* GetNamespace();
         virtual void SetNamespace(ModuleNamespace* moduleNamespace);
@@ -114,8 +115,9 @@ namespace Js
         const static uint InvalidSlotCount = 0xffffffff;
         const static uint InvalidSlotIndex = 0xffffffff;
         // TODO: move non-GC fields out to avoid false reference?
-        // This is the parsed tree resulted from compilation. 
+        // This is the parsed tree resulted from compilation.
         Field(bool) wasParsed;
+        Field(bool) shouldGenerateRootFunction;
         Field(bool) wasDeclarationInitialized;
         Field(bool) parentsNotified;
         Field(bool) isRootModule;

diff --git a/test/es6/module-bugfixes.js b/test/es6/module-bugfixes.js
@@ -42,18 +42,25 @@ var tests = [
             testRunner.LoadModule(functionBody, 'samethread');
         }
     },
-    {
-        name: "Memory leak test on syntax error",
-        body: function() {
-            try {
-                WScript.LoadModule('');
-                WScript.LoadModule('1');
-                WScript.LoadModule('const a = () -> {};');
-            } catch(e) {
-                // no-op
-            }
-        }
-    },
+    {
+        name: "Memory leak test on syntax error",
+        body: function() {
+            try {
+                WScript.LoadModule('');
+                WScript.LoadModule('1');
+                WScript.LoadModule('const a = () -> {};');
+            } catch(e) {
+                // no-op
+            }
+        }
+    },
+    {
+        name: "Issue 4482: Indirect circular module dependencies",
+        body: function() {
+            let functionBody = "import 'module_4482_dep1.js';"
+            testRunner.LoadModule(functionBody);
+        }
+    },
 ];
 
 testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
diff --git a/test/es6/module_4482_dep1.js b/test/es6/module_4482_dep1.js
@@ -0,0 +1,23 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+//this test case's circular dependencies would segfault
+//with CC's orriginal module implementation if JIT was enabled
+//issue was that:
+//i) dep 1 requires dep2 and dep3
+//ii) MDI would therefore be triggerred for dep2 (with dep3 queued)
+//iii) MDI for dep2 would not explicitly require dep3 as the import is via dep1
+//iv) second half of MDI for dep2 would attempt to reference the function Thing2 defined in dep 3
+//v) Thing2 would not yet exist = segfault
+
+
+import Thing1 from './module_4482_dep2.js';
+import Thing2 from './module_4482_dep3.js';
+
+export { Thing1, Thing2 };
+
+export default
+function main()
+{}
diff --git a/test/es6/module_4482_dep2.js b/test/es6/module_4482_dep2.js
@@ -0,0 +1,13 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+//thing1.js
+import { Thing2 } from './module_4482_dep1.js';
+
+export default
+function Thing1()
+{
+    Thing2();
+}
diff --git a/test/es6/module_4482_dep3.js b/test/es6/module_4482_dep3.js
@@ -0,0 +1,10 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+
+import { Thing1 } from './module_4482_dep1.js';
+
+export default
+function Thing2(){}