diff --git a/lib/Runtime/Language/ModuleRecordBase.h b/lib/Runtime/Language/ModuleRecordBase.h index 676b2f4df21..43e4497e956 100644 --- 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 index 682c343f978..61737987be9 100644 --- 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 index afcccaf34e3..df532ba15e7 100644 --- 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 index 8ea0aec7079..932f34aea47 100644 --- 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 new file mode 100644 index 00000000000..a1bf7c6120f --- /dev/null +++ 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 new file mode 100644 index 00000000000..32cf672cf9b --- /dev/null +++ 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 new file mode 100644 index 00000000000..d15a10caf34 --- /dev/null +++ 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(){}