[Merge chakra-core/ChakraCore@6a23c85c43] [1.6>1.7] [MERGE #3533 @jia…

…nchun] module circular reference GetExportedNames/ResolveExport bugs

Merge pull request #3533 from jianchun:modfix

GetExportedNames(): module namespace binds all names from
`GetExportedNames(nullptr)`. However we incorrectly cached result from any
`GetExportedNames(exportStarSet)` call. This causes module namespace
having incomplete names, because a previous intermediate call with non-null
exportStarSet did not include names from that set. This fails 3250-ext-a.
Found this problem while investigating #3250. Fixed by only cache/reuse the
result if exportStarSet is nullptr (root call).

ResolveExport(): parameter `exportStarSet` were used to prevent infinite
loop when looking up name under module circular references. However it
broke the case that we lookup this module record recursively for a
different name (indirect). This causes #3250 to throw SyntaxError for
failing to resolve a binding (resolving f_indirectUninit for dep2, reenter
to resolve c_localUninit1 but rejected by `exportStarSet`). Fixed by
removing it -- another parameter 'resolveSet' already does the job well. It
prevents recursion for the same {moduleRecord, name}, but allows looking up
a different name.

Finally, moved a fragment into try/catch and added an ENTER_SCRIPT_IF. The
given call may throw JavaScriptException which requires in script.

deps/chakrashim/core/lib/Runtime/Language/ModuleNamespace.cpp

@@ -94,7 +94,7 @@ namespace Js
             exportedNames->Map([&](PropertyId propertyId) {
                 JavascriptString* propertyString = scriptContext->GetPropertyString(propertyId);
                 sortedExportedNames->Add(propertyString);
-                if (!moduleRecord->ResolveExport(propertyId, nullptr, nullptr, &moduleNameRecord))
+                if (!moduleRecord->ResolveExport(propertyId, nullptr, &moduleNameRecord))
                 {
                     // ignore ambigious resolution.
 #if DBG

deps/chakrashim/core/lib/Runtime/Language/ModuleRecordBase.h

@@ -41,7 +41,7 @@ namespace Js
         virtual ExportedNames* GetExportedNames(ExportModuleRecordList* exportStarSet) = 0;
         // return false when "ambiguous".
         // otherwise nullptr means "null" where we have circular reference/cannot resolve.
-        virtual bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ExportModuleRecordList* exportStarSet, ModuleNameRecord** exportRecord) = 0;
+        virtual bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord) = 0;
         virtual void ModuleDeclarationInstantiation() = 0;
         virtual Var ModuleEvaluation() = 0;
         virtual bool IsSourceTextModuleRecord() { return false; }

deps/chakrashim/core/lib/Runtime/Language/SourceTextModuleRecord.cpp

@@ -400,11 +400,16 @@ namespace Js
 
     ExportedNames* SourceTextModuleRecord::GetExportedNames(ExportModuleRecordList* exportStarSet)
     {
-        if (exportedNames != nullptr)
+        const bool isRootGetExportedNames = (exportStarSet == nullptr);
+
+        // this->exportedNames only caches root "GetExportedNames(nullptr)"
+        if (isRootGetExportedNames && exportedNames != nullptr)
         {
             return exportedNames;
         }
+
         ArenaAllocator* allocator = scriptContext->GeneralAllocator();
+
         if (exportStarSet == nullptr)
         {
             exportStarSet = Anew(allocator, ExportModuleRecordList, allocator);
@@ -414,6 +419,7 @@ namespace Js
             return nullptr;
         }
         exportStarSet->Prepend(this);
+
         ExportedNames* tempExportedNames = nullptr;
         if (this->localExportRecordList != nullptr)
         {
@@ -468,7 +474,13 @@ namespace Js
 #endif
             });
         }
-        exportedNames = tempExportedNames;
+
+        // this->exportedNames only caches root "GetExportedNames(nullptr)"
+        if (isRootGetExportedNames)
+        {
+            exportedNames = tempExportedNames;
+        }
+
         return tempExportedNames;
     }
 
@@ -488,7 +500,7 @@ namespace Js
                 }
                 else
                 {
-                    childModule->ResolveExport(importName, nullptr, nullptr, importRecord);
+                    childModule->ResolveExport(importName, nullptr, importRecord);
                 }
                 return true;
             }
@@ -500,7 +512,7 @@ namespace Js
 
     // return false when "ambiguous".
     // otherwise nullptr means "null" where we have circular reference/cannot resolve.
-    bool SourceTextModuleRecord::ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ExportModuleRecordList* exportStarSet, ModuleNameRecord** exportRecord)
+    bool SourceTextModuleRecord::ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord)
     {
         ArenaAllocator* allocator = scriptContext->GeneralAllocator();
         if (resolvedExportMap == nullptr)
@@ -512,10 +524,6 @@ namespace Js
             return true;
         }
         // TODO: use per-call/loop allocator?
-        if (exportStarSet == nullptr)
-        {
-            exportStarSet = Anew(allocator, ExportModuleRecordList, allocator);
-        }
         if (resolveSet == nullptr)
         {
             resolveSet = Anew(allocator, ResolveSet, allocator);
@@ -589,7 +597,7 @@ namespace Js
                 }
                 else
                 {
-                    isAmbiguous = !childModuleRecord->ResolveExport(importNameId, resolveSet, exportStarSet, exportRecord);
+                    isAmbiguous = !childModuleRecord->ResolveExport(importNameId, resolveSet, exportRecord);
                     if (isAmbiguous)
                     {
                         // ambiguous; don't need to search further
@@ -624,13 +632,6 @@ namespace Js
             return false;
         }
 
-        if (exportStarSet->Has(this))
-        {
-            *exportRecord = nullptr;
-            return true;
-        }
-
-        exportStarSet->Prepend(this);
         bool ambiguousResolution = false;
         if (this->starExportRecordList != nullptr)
         {
@@ -648,7 +649,7 @@ namespace Js
                 }
 
                 // if ambigious, return "ambigious"
-                if (!childModule->ResolveExport(exportName, resolveSet, exportStarSet, &currentResolution))
+                if (!childModule->ResolveExport(exportName, resolveSet, &currentResolution))
                 {
                     ambiguousResolution = true;
                     return true;
@@ -796,6 +797,21 @@ namespace Js
             InitializeLocalImports();
 
             InitializeIndirectExports();
+
+            SetWasDeclarationInitialized();
+            if (childrenModuleSet != nullptr)
+            {
+                childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
+                {
+                    Assert(moduleRecord->WasParsed());
+                    moduleRecord->ModuleDeclarationInstantiation();
+                });
+            }
+
+            ENTER_SCRIPT_IF(scriptContext, true, false, false, !scriptContext->GetThreadContext()->IsScriptActive(),
+            {
+                ModuleNamespace::GetModuleNamespace(this);
+            });
         }
         catch (const JavascriptException& err)
         {
@@ -809,17 +825,6 @@ namespace Js
             return;
         }
 
-        SetWasDeclarationInitialized();
-        if (childrenModuleSet != nullptr)
-        {
-            childrenModuleSet->Map([](LPCOLESTR specifier, SourceTextModuleRecord* moduleRecord)
-            {
-                Assert(moduleRecord->WasParsed());
-                moduleRecord->ModuleDeclarationInstantiation();
-            });
-        }
-
-        ModuleNamespace::GetModuleNamespace(this);
         Js::AutoDynamicCodeReference dynamicFunctionReference(scriptContext);
         Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));
         CompileScriptException se;
@@ -965,7 +970,7 @@ namespace Js
                 // We don't need to initialize anything for * import.
                 if (importName != Js::PropertyIds::star_)
                 {
-                    if (!childModule->ResolveExport(importName, nullptr, nullptr, &importRecord)
+                    if (!childModule->ResolveExport(importName, nullptr, &importRecord)
                         || importRecord == nullptr)
                     {
                         JavascriptError* errorObj = scriptContext->GetLibrary()->CreateSyntaxError();
@@ -1097,7 +1102,7 @@ namespace Js
                     this->errorObject = errorObj;
                     return;
                 }
-                if (!childModuleRecord->ResolveExport(propertyId, nullptr, nullptr, &exportRecord) ||
+                if (!childModuleRecord->ResolveExport(propertyId, nullptr, &exportRecord) ||
                     (exportRecord == nullptr))
                 {
                     JavascriptError* errorObj = scriptContext->GetLibrary()->CreateSyntaxError();

deps/chakrashim/core/lib/Runtime/Language/SourceTextModuleRecord.h

@@ -29,7 +29,7 @@ namespace Js
 
         // return false when "ambiguous". 
         // otherwise nullptr means "null" where we have circular reference/cannot resolve.
-        bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ExportModuleRecordList* exportStarSet, ModuleNameRecord** exportRecord) override;
+        bool ResolveExport(PropertyId exportName, ResolveSet* resolveSet, ModuleNameRecord** exportRecord) override;
         bool ResolveImport(PropertyId localName, ModuleNameRecord** importRecord);
         void ModuleDeclarationInstantiation() override;
         Var ModuleEvaluation() override;

deps/chakrashim/core/test/es6/module-3250-bug-dep.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.
+//-------------------------------------------------------------------------------------------------------
+
+// module-3250-bug-dep.js
+import * as ns from './module-3250-bug-dep.js';
+
+export let c_localUninit1;
+export { c_localUninit1 as f_indirectUninit } from './module-3250-bug-dep2.js';
+
+var stringKeys = Object.getOwnPropertyNames(ns);
+assert.areEqual('["c_localUninit1","f_indirectUninit"]', JSON.stringify(stringKeys));

deps/chakrashim/core/test/es6/module-3250-bug-dep2.js

@@ -0,0 +1,7 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// module-3250-bug-dep2.js
+export * from './module-3250-bug-dep.js';

deps/chakrashim/core/test/es6/module-3250-ext-a.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.
+//-------------------------------------------------------------------------------------------------------
+
+// module-3250-ext-a.js
+import * as ns from './module-3250-ext-a.js';
+
+export let a;
+export * from './module-3250-ext-b.js';
+
+var stringKeys = Object.getOwnPropertyNames(ns);
+assert.areEqual('["a","b"]', JSON.stringify(stringKeys));

deps/chakrashim/core/test/es6/module-3250-ext-b.js

@@ -0,0 +1,8 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// module-3250-ext-b.js
+export let b;
+export * from './module-3250-ext-a.js';

deps/chakrashim/core/test/es6/module-functionality.js

@@ -275,6 +275,22 @@ var tests = [
             testRunner.LoadModule(functionBody, 'samethread');
         }
     },
+    {
+        name: "Circular module dependency: starExportList should allow multiple/recursive visits to re-resolve different names",
+        body: function () {
+            let functionBody =
+                `import './module-3250-bug-dep.js';`;
+            testModuleScript(functionBody);
+        }
+    },
+    {
+        name: "Circular module dependency: non-root GetExportNames should not be cached",
+        body: function () {
+            let functionBody =
+                `import './module-3250-ext-a.js';`;
+            testModuleScript(functionBody);
+        }
+    },
     {
         name: "Implicitly re-exporting an import binding (import { foo } from ''; export { foo };)",
         body: function () {