Skip to content

Commit

Permalink
[MERGE #6117 @atulkatti] Cleanup parser arena in more module error ca…
Browse files Browse the repository at this point in the history 
…ses. Also, make sure we clean up parent and child modules where applicable.

Merge pull request #6117 from atulkatti:ModulesCleanup.1
  • Loading branch information
@atulkatti
atulkatti committed May 15, 2019
2 parents 49f594b + 287b269 commit 16fc8ac
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 13 deletions.
54 changes: 42 additions & 12 deletions lib/Runtime/Language/SourceTextModuleRecord.cpp
View file
Expand Up @@ -172,7 +172,7 @@ namespace Js
}

// Cleanup in case of error.
this->ReleaseParserResources();
this->ReleaseParserResourcesForHierarchy();
return E_FAIL;
}
}
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -276,7 +291,7 @@ namespace Js
else
{
// Cleanup in case of error.
this->ReleaseParserResources();
this->ReleaseParserResourcesForHierarchy();
}

return hr;
Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -963,7 +989,7 @@ namespace Js
if (this->errorObject != nullptr)
{
// Cleanup in case of error.
this->ReleaseParserResources();
this->ReleaseParserResourcesForHierarchy();

if (this->promise != nullptr)
{
Expand Down Expand Up @@ -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);
}
});
Expand Down Expand Up @@ -1137,6 +1165,7 @@ namespace Js
// 2G is too big already.
if (localExportCount >= INT_MAX)
{
this->ReleaseParserResourcesForHierarchy();
JavascriptError::ThrowRangeError(scriptContext, JSERR_TooManyImportExports);
}
localExportCount++;
Expand All @@ -1160,6 +1189,7 @@ namespace Js
currentSlotCount++;
if (currentSlotCount >= INT_MAX)
{
this->ReleaseParserResourcesForHierarchy();
JavascriptError::ThrowRangeError(scriptContext, JSERR_TooManyImportExports);
}
}
Expand Down
4 changes: 3 additions & 1 deletion lib/Runtime/Language/SourceTextModuleRecord.h
View file
Expand Up @@ -17,6 +17,7 @@ namespace Js
{
public:
friend class ModuleNamespace;
friend class JavascriptLibrary;

SourceTextModuleRecord(ScriptContext* scriptContext);
IdentPtrList* GetRequestedModuleList() const { return requestedModuleList; }
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -160,6 +161,7 @@ namespace Js
HRESULT PostParseProcess();
HRESULT PrepareForModuleDeclarationInitialization();
void ReleaseParserResources();
void ReleaseParserResourcesForHierarchy();
void ImportModuleListsFromParser();
HRESULT OnChildModuleReady(SourceTextModuleRecord* childModule, Var errorObj);
void NotifyParentsAsNeeded();
Expand Down
11 changes: 11 additions & 0 deletions lib/Runtime/Library/JavascriptLibrary.cpp
View file
Expand Up @@ -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;
}

Expand Down

0 comments on commit 16fc8ac

Please sign in to comment.