[MERGE #6117 @atulkatti] Cleanup parser arena in more module error cases. Also, make sure we clean up parent and child modules where applicable.

atulkatti · atulkatti · commit 16fc8ac63cd7 · 2019-05-15T10:42:22.000-07:00
Merge pull request #6117 from atulkatti:ModulesCleanup.1
diff --git a/lib/Runtime/Language/SourceTextModuleRecord.cpp b/lib/Runtime/Language/SourceTextModuleRecord.cpp
@@ -172,7 +172,7 @@ namespace Js
                     }
 
                     // Cleanup in case of error.
-                    this->ReleaseParserResources();
+                    this->ReleaseParserResourcesForHierarchy();
                     return E_FAIL;
                 }
             }
@@ -189,6 +189,8 @@ namespace Js
                 }
                 *exceptionVar = JavascriptError::CreateFromCompileScriptException(scriptContext, &se, sourceUrl);
             }
+            // Cleanup in case of error.
+            this->ReleaseParserResourcesForHierarchy();
             if (this->parser)
             {
                 this->parseTree = nullptr;
@@ -247,7 +249,7 @@ namespace Js
     {
         Assert(scriptContext->GetConfig()->IsES6ModuleEnabled());
         ParseNodeModule* moduleParseNode = this->parseTree->AsParseNodeModule();
-        SetrequestedModuleList(moduleParseNode->requestedModules);
+        SetRequestedModuleList(moduleParseNode->requestedModules);
         SetImportRecordList(moduleParseNode->importEntries);
         SetStarExportRecordList(moduleParseNode->starExportEntries);
         SetIndirectExportRecordList(moduleParseNode->indirectExportEntries);
@@ -262,6 +264,19 @@ namespace Js
         }
     }
 
+    void SourceTextModuleRecord::ReleaseParserResourcesForHierarchy()
+    {
+        this->ReleaseParserResources();
+
+        if (this->childrenModuleSet != nullptr)
+        {
+            this->childrenModuleSet->EachValue([=](SourceTextModuleRecord* childModuleRecord)
+            {
+                childModuleRecord->ReleaseParserResources();
+            });
+        }
+    }
+
     HRESULT SourceTextModuleRecord::PostParseProcess()
     {
         HRESULT hr = NOERROR;
@@ -276,7 +291,7 @@ namespace Js
         else
         {
             // Cleanup in case of error.
-            this->ReleaseParserResources();
+            this->ReleaseParserResourcesForHierarchy();
         }
 
         return hr;
@@ -306,7 +321,7 @@ namespace Js
                 if (this->errorObject != nullptr)
                 {
                     // Cleanup in case of error.
-                    this->ReleaseParserResources();
+                    this->ReleaseParserResourcesForHierarchy();
                     SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->errorObject, scriptContext, this, false);
                 }
                 else
@@ -327,7 +342,7 @@ namespace Js
             if (FAILED(hr))
             {
                 // Cleanup in case of error.
-                this->ReleaseParserResources();
+                this->ReleaseParserResourcesForHierarchy();
 
                 // We cannot just use the buffer in the specifier string - need to make a copy here.
                 const char16* moduleName = this->GetSpecifierSz();
@@ -366,6 +381,13 @@ namespace Js
                 {
                     GenerateRootFunction();
                 }
+
+                if (this->errorObject != nullptr)
+                {
+                    // Cleanup in case of error.
+                    this->ReleaseParserResourcesForHierarchy();
+                }
+
                 if (!hadNotifyHostReady && !WasEvaluated())
                 {
                     OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyHostAboutModuleReady %s (PrepareForModuleDeclarationInitialization)\n"), this->GetSpecifierSz());
@@ -394,7 +416,7 @@ namespace Js
             }
 
             // Cleanup in case of error.
-            this->ReleaseParserResources();
+            this->ReleaseParserResourcesForHierarchy();
 
             OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentAsNeeded (childException)\n"), this->GetSpecifierSz());
             NotifyParentsAsNeeded();
@@ -632,6 +654,7 @@ namespace Js
                 SourceTextModuleRecord* childModuleRecord = GetChildModuleRecord(exportEntry.moduleRequest->Psz());
                 if (childModuleRecord == nullptr)
                 {
+                    this->ReleaseParserResourcesForHierarchy();
                     JavascriptError::ThrowReferenceError(scriptContext, JSERR_CannotResolveModule, exportEntry.moduleRequest->Psz());
                 }
 
@@ -683,10 +706,12 @@ namespace Js
                 SourceTextModuleRecord* childModule = GetChildModuleRecord(starExportEntry.moduleRequest->Psz());
                 if (childModule == nullptr)
                 {
+                    this->ReleaseParserResourcesForHierarchy();
                     JavascriptError::ThrowReferenceError(GetScriptContext(), JSERR_CannotResolveModule, starExportEntry.moduleRequest->Psz());
                 }
                 if (childModule->errorObject != nullptr)
                 {
+                    this->ReleaseParserResourcesForHierarchy();
                     JavascriptExceptionOperators::Throw(childModule->errorObject, GetScriptContext());
                 }
 
@@ -851,10 +876,6 @@ namespace Js
         Assert(wasDeclarationInitialized);
         // Debugger can reparse the source and generate the byte code again. Don't cleanup the
         // helper information for now.
-
-        // Parser uses a temporary guest arena to keep regex patterns alive. We need to release this arena only after we have no further use
-        // for the regex pattern objects.
-        this->ReleaseParserResources();
     }
 
     bool SourceTextModuleRecord::ModuleDeclarationInstantiation()
@@ -900,7 +921,7 @@ namespace Js
         {
             OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded (errorObject)\n"));
             // Cleanup in case of error.
-            this->ReleaseParserResources();
+            this->ReleaseParserResourcesForHierarchy();
             NotifyParentsAsNeeded();
             return false;
         }
@@ -925,6 +946,11 @@ namespace Js
         Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));
 
         this->rootFunction = scriptContext->GenerateRootFunction(parseTree, sourceIndex, this->parser, this->pSourceInfo->GetParseFlags(), &se, _u("module"));
+
+        // Parser uses a temporary guest arena to keep regex patterns alive. We need to release this arena only after we have no further use
+        // for the regex pattern objects.
+        this->ReleaseParserResources();
+
         if (rootFunction == nullptr)
         {
             const WCHAR * sourceUrl = nullptr;
@@ -963,7 +989,7 @@ namespace Js
         if (this->errorObject != nullptr)
         {
             // Cleanup in case of error.
-            this->ReleaseParserResources();
+            this->ReleaseParserResourcesForHierarchy();
 
             if (this->promise != nullptr)
             {
@@ -1007,6 +1033,8 @@ namespace Js
                     // if child module has been dynamically imported and has exception need to throw
                     if (childModuleRecord->GetErrorObject() != nullptr)
                     {
+                        this->ReleaseParserResourcesForHierarchy();
+
                         JavascriptExceptionOperators::Throw(childModuleRecord->GetErrorObject(), this->scriptContext);
                     }
                 });
@@ -1137,6 +1165,7 @@ namespace Js
                     // 2G is too big already.
                     if (localExportCount >= INT_MAX)
                     {
+                        this->ReleaseParserResourcesForHierarchy();
                         JavascriptError::ThrowRangeError(scriptContext, JSERR_TooManyImportExports);
                     }
                     localExportCount++;
@@ -1160,6 +1189,7 @@ namespace Js
                         currentSlotCount++;
                         if (currentSlotCount >= INT_MAX)
                         {
+                            this->ReleaseParserResourcesForHierarchy();
                             JavascriptError::ThrowRangeError(scriptContext, JSERR_TooManyImportExports);
                         }
                     }
diff --git a/lib/Runtime/Language/SourceTextModuleRecord.h b/lib/Runtime/Language/SourceTextModuleRecord.h
@@ -17,6 +17,7 @@ namespace Js
     {
     public:
         friend class ModuleNamespace;
+        friend class JavascriptLibrary;
 
         SourceTextModuleRecord(ScriptContext* scriptContext);
         IdentPtrList* GetRequestedModuleList() const { return requestedModuleList; }
@@ -70,7 +71,7 @@ namespace Js
         void SetLocalExportRecordList(ModuleImportOrExportEntryList* localExports) { localExportRecordList = localExports; }
         void SetIndirectExportRecordList(ModuleImportOrExportEntryList* indirectExports) { indirectExportRecordList = indirectExports; }
         void SetStarExportRecordList(ModuleImportOrExportEntryList* starExports) { starExportRecordList = starExports; }
-        void SetrequestedModuleList(IdentPtrList* requestModules) { requestedModuleList = requestModules; }
+        void SetRequestedModuleList(IdentPtrList* requestModules) { requestedModuleList = requestModules; }
 
         ScriptContext* GetScriptContext() const { return scriptContext; }
         HRESULT ParseSource(__in_bcount(sourceLength) byte* sourceText, uint32 sourceLength, SRCINFO * srcInfo, Var* exceptionVar, bool isUtf8);
@@ -160,6 +161,7 @@ namespace Js
         HRESULT PostParseProcess();
         HRESULT PrepareForModuleDeclarationInitialization();
         void ReleaseParserResources();
+        void ReleaseParserResourcesForHierarchy();
         void ImportModuleListsFromParser();
         HRESULT OnChildModuleReady(SourceTextModuleRecord* childModule, Var errorObj);
         void NotifyParentsAsNeeded();
diff --git a/lib/Runtime/Library/JavascriptLibrary.cpp b/lib/Runtime/Library/JavascriptLibrary.cpp
@@ -128,6 +128,17 @@ namespace Js
 
     void JavascriptLibrary::Uninitialize()
     {
+#if DBG
+        if (moduleRecordList != nullptr)
+        {
+            // This should mostly be a no-op except for error cases where we may not have had a chance to cleaup a module record yet.
+            // Do this only in debug builds to avoid reporting of a memory leak. In release builds this doesn't matter as we will cleanup anyways.
+            for (int index = 0; index < moduleRecordList->Count(); index++)
+            {
+                moduleRecordList->Item(index)->ReleaseParserResources();
+            }
+        }
+#endif
         this->globalObject = nullptr;
     }
 

Original file line number	Diff line number	Diff line change
`@@ -172,7 +172,7 @@ namespace Js`
`172`	`172`	`}`
`173`	`173`
`174`	`174`	`// Cleanup in case of error.`
`175`		`- this->ReleaseParserResources();`
	`175`	`+ this->ReleaseParserResourcesForHierarchy();`
`176`	`176`	`return E_FAIL;`
`177`	`177`	`}`
`178`	`178`	`}`
`@@ -189,6 +189,8 @@ namespace Js`
`189`	`189`	`}`
`190`	`190`	`*exceptionVar = JavascriptError::CreateFromCompileScriptException(scriptContext, &se, sourceUrl);`
`191`	`191`	`}`
	`192`	`+ // Cleanup in case of error.`
	`193`	`+ this->ReleaseParserResourcesForHierarchy();`
`192`	`194`	`if (this->parser)`
`193`	`195`	`{`
`194`	`196`	`this->parseTree = nullptr;`
`@@ -247,7 +249,7 @@ namespace Js`
`247`	`249`	`{`
`248`	`250`	`Assert(scriptContext->GetConfig()->IsES6ModuleEnabled());`
`249`	`251`	`ParseNodeModule* moduleParseNode = this->parseTree->AsParseNodeModule();`
`250`		`- SetrequestedModuleList(moduleParseNode->requestedModules);`
	`252`	`+ SetRequestedModuleList(moduleParseNode->requestedModules);`
`251`	`253`	`SetImportRecordList(moduleParseNode->importEntries);`
`252`	`254`	`SetStarExportRecordList(moduleParseNode->starExportEntries);`
`253`	`255`	`SetIndirectExportRecordList(moduleParseNode->indirectExportEntries);`
`@@ -262,6 +264,19 @@ namespace Js`
`262`	`264`	`}`
`263`	`265`	`}`
`264`	`266`
	`267`	`+ void SourceTextModuleRecord::ReleaseParserResourcesForHierarchy()`
	`268`	`+ {`
	`269`	`+ this->ReleaseParserResources();`
	`270`	`+`
	`271`	`+ if (this->childrenModuleSet != nullptr)`
	`272`	`+ {`
	`273`	`+ this->childrenModuleSet->EachValue([=](SourceTextModuleRecord* childModuleRecord)`
	`274`	`+ {`
	`275`	`+ childModuleRecord->ReleaseParserResources();`
	`276`	`+ });`
	`277`	`+ }`
	`278`	`+ }`
	`279`	`+`
`265`	`280`	`HRESULT SourceTextModuleRecord::PostParseProcess()`
`266`	`281`	`{`
`267`	`282`	`HRESULT hr = NOERROR;`
`@@ -276,7 +291,7 @@ namespace Js`
`276`	`291`	`else`
`277`	`292`	`{`
`278`	`293`	`// Cleanup in case of error.`
`279`		`- this->ReleaseParserResources();`
	`294`	`+ this->ReleaseParserResourcesForHierarchy();`
`280`	`295`	`}`
`281`	`296`
`282`	`297`	`return hr;`
`@@ -306,7 +321,7 @@ namespace Js`
`306`	`321`	`if (this->errorObject != nullptr)`
`307`	`322`	`{`
`308`	`323`	`// Cleanup in case of error.`
`309`		`- this->ReleaseParserResources();`
	`324`	`+ this->ReleaseParserResourcesForHierarchy();`
`310`	`325`	`SourceTextModuleRecord::ResolveOrRejectDynamicImportPromise(false, this->errorObject, scriptContext, this, false);`
`311`	`326`	`}`
`312`	`327`	`else`
`@@ -327,7 +342,7 @@ namespace Js`
`327`	`342`	`if (FAILED(hr))`
`328`	`343`	`{`
`329`	`344`	`// Cleanup in case of error.`
`330`		`- this->ReleaseParserResources();`
	`345`	`+ this->ReleaseParserResourcesForHierarchy();`
`331`	`346`
`332`	`347`	`// We cannot just use the buffer in the specifier string - need to make a copy here.`
`333`	`348`	`const char16* moduleName = this->GetSpecifierSz();`
`@@ -366,6 +381,13 @@ namespace Js`
`366`	`381`	`{`
`367`	`382`	`GenerateRootFunction();`
`368`	`383`	`}`
	`384`	`+`
	`385`	`+ if (this->errorObject != nullptr)`
	`386`	`+ {`
	`387`	`+ // Cleanup in case of error.`
	`388`	`+ this->ReleaseParserResourcesForHierarchy();`
	`389`	`+ }`
	`390`	`+`
`369`	`391`	`if (!hadNotifyHostReady && !WasEvaluated())`
`370`	`392`	`{`
`371`	`393`	`OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyHostAboutModuleReady %s (PrepareForModuleDeclarationInitialization)\n"), this->GetSpecifierSz());`
`@@ -394,7 +416,7 @@ namespace Js`
`394`	`416`	`}`
`395`	`417`
`396`	`418`	`// Cleanup in case of error.`
`397`		`- this->ReleaseParserResources();`
	`419`	`+ this->ReleaseParserResourcesForHierarchy();`
`398`	`420`
`399`	`421`	`OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentAsNeeded (childException)\n"), this->GetSpecifierSz());`
`400`	`422`	`NotifyParentsAsNeeded();`
`@@ -632,6 +654,7 @@ namespace Js`
`632`	`654`	`SourceTextModuleRecord* childModuleRecord = GetChildModuleRecord(exportEntry.moduleRequest->Psz());`
`633`	`655`	`if (childModuleRecord == nullptr)`
`634`	`656`	`{`
	`657`	`+ this->ReleaseParserResourcesForHierarchy();`
`635`	`658`	`JavascriptError::ThrowReferenceError(scriptContext, JSERR_CannotResolveModule, exportEntry.moduleRequest->Psz());`
`636`	`659`	`}`
`637`	`660`
`@@ -683,10 +706,12 @@ namespace Js`
`683`	`706`	`SourceTextModuleRecord* childModule = GetChildModuleRecord(starExportEntry.moduleRequest->Psz());`
`684`	`707`	`if (childModule == nullptr)`
`685`	`708`	`{`
	`709`	`+ this->ReleaseParserResourcesForHierarchy();`
`686`	`710`	`JavascriptError::ThrowReferenceError(GetScriptContext(), JSERR_CannotResolveModule, starExportEntry.moduleRequest->Psz());`
`687`	`711`	`}`
`688`	`712`	`if (childModule->errorObject != nullptr)`
`689`	`713`	`{`
	`714`	`+ this->ReleaseParserResourcesForHierarchy();`
`690`	`715`	`JavascriptExceptionOperators::Throw(childModule->errorObject, GetScriptContext());`
`691`	`716`	`}`
`692`	`717`
`@@ -851,10 +876,6 @@ namespace Js`
`851`	`876`	`Assert(wasDeclarationInitialized);`
`852`	`877`	`// Debugger can reparse the source and generate the byte code again. Don't cleanup the`
`853`	`878`	`// helper information for now.`
`854`		`-`
`855`		`- // Parser uses a temporary guest arena to keep regex patterns alive. We need to release this arena only after we have no further use`
`856`		`- // for the regex pattern objects.`
`857`		`- this->ReleaseParserResources();`
`858`	`879`	`}`
`859`	`880`
`860`	`881`	`bool SourceTextModuleRecord::ModuleDeclarationInstantiation()`
`@@ -900,7 +921,7 @@ namespace Js`
`900`	`921`	`{`
`901`	`922`	`OUTPUT_TRACE_DEBUGONLY(Js::ModulePhase, _u("\t>NotifyParentsAsNeeded (errorObject)\n"));`
`902`	`923`	`// Cleanup in case of error.`
`903`		`- this->ReleaseParserResources();`
	`924`	`+ this->ReleaseParserResourcesForHierarchy();`
`904`	`925`	`NotifyParentsAsNeeded();`
`905`	`926`	`return false;`
`906`	`927`	`}`
`@@ -925,6 +946,11 @@ namespace Js`
`925`	`946`	`Assert(this == scriptContext->GetLibrary()->GetModuleRecord(this->pSourceInfo->GetSrcInfo()->moduleID));`
`926`	`947`
`927`	`948`	`this->rootFunction = scriptContext->GenerateRootFunction(parseTree, sourceIndex, this->parser, this->pSourceInfo->GetParseFlags(), &se, _u("module"));`
	`949`	`+`
	`950`	`+ // Parser uses a temporary guest arena to keep regex patterns alive. We need to release this arena only after we have no further use`
	`951`	`+ // for the regex pattern objects.`
	`952`	`+ this->ReleaseParserResources();`
	`953`	`+`
`928`	`954`	`if (rootFunction == nullptr)`
`929`	`955`	`{`
`930`	`956`	`const WCHAR * sourceUrl = nullptr;`
`@@ -963,7 +989,7 @@ namespace Js`
`963`	`989`	`if (this->errorObject != nullptr)`
`964`	`990`	`{`
`965`	`991`	`// Cleanup in case of error.`
`966`		`- this->ReleaseParserResources();`
	`992`	`+ this->ReleaseParserResourcesForHierarchy();`
`967`	`993`
`968`	`994`	`if (this->promise != nullptr)`
`969`	`995`	`{`
`@@ -1007,6 +1033,8 @@ namespace Js`
`1007`	`1033`	`// if child module has been dynamically imported and has exception need to throw`
`1008`	`1034`	`if (childModuleRecord->GetErrorObject() != nullptr)`
`1009`	`1035`	`{`
	`1036`	`+ this->ReleaseParserResourcesForHierarchy();`
	`1037`	`+`
`1010`	`1038`	`JavascriptExceptionOperators::Throw(childModuleRecord->GetErrorObject(), this->scriptContext);`
`1011`	`1039`	`}`
`1012`	`1040`	`});`
`@@ -1137,6 +1165,7 @@ namespace Js`
`1137`	`1165`	`// 2G is too big already.`
`1138`	`1166`	`if (localExportCount >= INT_MAX)`
`1139`	`1167`	`{`
	`1168`	`+ this->ReleaseParserResourcesForHierarchy();`
`1140`	`1169`	`JavascriptError::ThrowRangeError(scriptContext, JSERR_TooManyImportExports);`
`1141`	`1170`	`}`
`1142`	`1171`	`localExportCount++;`
`@@ -1160,6 +1189,7 @@ namespace Js`
`1160`	`1189`	`currentSlotCount++;`
`1161`	`1190`	`if (currentSlotCount >= INT_MAX)`
`1162`	`1191`	`{`
	`1192`	`+ this->ReleaseParserResourcesForHierarchy();`
`1163`	`1193`	`JavascriptError::ThrowRangeError(scriptContext, JSERR_TooManyImportExports);`
`1164`	`1194`	`}`
`1165`	`1195`	`}`