Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Commit

Permalink
[Merge chakra-core/ChakraCore@be5c231aae] [1.6>1.7] [MERGE #3485 @aga…
Browse files Browse the repository at this point in the history 
…rwal-sandeep] OS# 12999605: Fix deleting of DebugContext

Merge pull request #3485 from agarwal-sandeep:debugcontextclose

Don't delete debugContext when ScriptContext is closed as we might be
in middle of a PDM call. When ScriptContext is closed delete all memory
allocated by debugContext and mark it as closed. Delete debugContext in
ScriptContext destructor.
  • Loading branch information
@chakrabot @kfarnung
chakrabot authored and kfarnung committed Aug 10, 2017
1 parent 11c68c9 commit bf0b020
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 34 deletions.
46 changes: 33 additions & 13 deletions deps/chakrashim/core/lib/Runtime/Base/ScriptContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,13 @@ namespace Js
}
#endif

if (this->debugContext != nullptr)
{
Assert(this->debugContext->IsClosed());
HeapDelete(this->debugContext);
this->debugContext = nullptr;
}

#if ENABLE_NATIVE_CODEGEN
if (this->nativeCodeGen != nullptr)
{
Expand Down Expand Up @@ -665,21 +672,20 @@ namespace Js
#endif

this->EnsureClearDebugDocument();

if (this->debugContext != nullptr)
{
if(this->debugContext->GetProbeContainer())
if (this->debugContext->GetProbeContainer() != nullptr)
{
this->debugContext->GetProbeContainer()->UninstallInlineBreakpointProbe(NULL);
this->debugContext->GetProbeContainer()->UninstallInlineBreakpointProbe(nullptr);
this->debugContext->GetProbeContainer()->UninstallDebuggerScriptOptionCallback();
}

// Guard the closing and deleting of DebugContext as in meantime PDM might
// call OnBreakFlagChange
// Guard the closing DebugContext as in meantime PDM might call OnBreakFlagChange
AutoCriticalSection autoDebugContextCloseCS(&debugContextCloseCS);
DebugContext* tempDebugContext = this->debugContext;
this->debugContext = nullptr;
tempDebugContext->Close();
HeapDelete(tempDebugContext);
this->debugContext->Close();
// Not deleting debugContext here as Close above will clear all memory debugContext allocated.
// Actual deletion of debugContext will happen in ScriptContext destructor
}

if (this->diagnosticArena != nullptr)
Expand Down Expand Up @@ -5792,9 +5798,23 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
}
#endif

DebugContext* ScriptContext::GetDebugContext() const
{
Assert(this->debugContext != nullptr);

if (this->debugContext->IsClosed())
{
// Once DebugContext is closed we should assume it's not there
// The actual deletion of debugContext happens in ScriptContext destructor
return nullptr;
}

return this->debugContext;
}

bool ScriptContext::IsScriptContextInNonDebugMode() const
{
if (this->debugContext != nullptr)
if (this->GetDebugContext() != nullptr)
{
return this->GetDebugContext()->IsDebugContextInNonDebugMode();
}
Expand All @@ -5803,7 +5823,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie

bool ScriptContext::IsScriptContextInDebugMode() const
{
if (this->debugContext != nullptr)
if (this->GetDebugContext() != nullptr)
{
return this->GetDebugContext()->IsDebugContextInDebugMode();
}
Expand All @@ -5812,7 +5832,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie

bool ScriptContext::IsScriptContextInSourceRundownOrDebugMode() const
{
if (this->debugContext != nullptr)
if (this->GetDebugContext() != nullptr)
{
return this->GetDebugContext()->IsDebugContextInSourceRundownOrDebugMode();
}
Expand All @@ -5821,7 +5841,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie

bool ScriptContext::IsDebuggerRecording() const
{
if (this->debugContext != nullptr)
if (this->GetDebugContext() != nullptr)
{
return this->GetDebugContext()->IsDebuggerRecording();
}
Expand All @@ -5830,7 +5850,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie

void ScriptContext::SetIsDebuggerRecording(bool isDebuggerRecording)
{
if (this->debugContext != nullptr)
if (this->GetDebugContext() != nullptr)
{
this->GetDebugContext()->SetIsDebuggerRecording(isDebuggerRecording);
}
Expand Down
2 changes: 1 addition & 1 deletion deps/chakrashim/core/lib/Runtime/Base/ScriptContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -993,7 +993,7 @@ namespace Js
#endif

bool IsDebugContextInitialized() const { return this->isDebugContextInitialized; }
DebugContext* GetDebugContext() const { return this->debugContext; }
DebugContext* GetDebugContext() const;
CriticalSection* GetDebugContextCloseCS() { return &debugContextCloseCS; }

uint callCount;
Expand Down
46 changes: 30 additions & 16 deletions deps/chakrashim/core/lib/Runtime/Debug/DebugContext.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Js
scriptContext(scriptContext),
hostDebugContext(nullptr),
diagProbesContainer(nullptr),
isClosed(false),
debuggerMode(DebuggerMode::NotDebugging),
isDebuggerRecording(true),
isReparsingSource(false)
Expand All @@ -19,7 +20,7 @@ namespace Js

DebugContext::~DebugContext()
{
Assert(this->scriptContext == nullptr);
Assert(this->scriptContext != nullptr);
Assert(this->hostDebugContext == nullptr);
Assert(this->diagProbesContainer == nullptr);
}
Expand All @@ -33,8 +34,18 @@ namespace Js

void DebugContext::Close()
{
if (this->isClosed)
{
return;
}

AssertMsg(this->scriptContext->IsActuallyClosed(), "Closing DebugContext before ScriptContext close might have consequences");

this->isClosed = true;

// Release all memory and do all cleanup. No operation should be done after isClosed is set

Assert(this->scriptContext != nullptr);
this->scriptContext = nullptr;

if (this->diagProbesContainer != nullptr)
{
Expand All @@ -50,6 +61,11 @@ namespace Js
}
}

bool DebugContext::IsSelfOrScriptContextClosed() const
{
return (this->IsClosed() || this->scriptContext->IsClosed());
}

void DebugContext::SetHostDebugContext(HostDebugContext * hostDebugContext)
{
Assert(this->hostDebugContext == nullptr);
Expand All @@ -60,10 +76,13 @@ namespace Js

bool DebugContext::CanRegisterFunction() const
{
if (this->hostDebugContext == nullptr || this->scriptContext == nullptr || this->scriptContext->IsClosed() || this->IsDebugContextInNonDebugMode())
if (this->IsSelfOrScriptContextClosed() ||
this->hostDebugContext == nullptr ||
this->IsDebugContextInNonDebugMode())
{
return false;
}

return true;
}

Expand Down Expand Up @@ -195,14 +214,10 @@ namespace Js
return hr;
}

// Cache ScriptContext as multiple calls below can go out of engine and ScriptContext can be closed which will delete DebugContext
Js::ScriptContext* cachedScriptContext = this->scriptContext;

utf8SourceInfoList->MapUntil([&](int index, Js::Utf8SourceInfo * sourceInfo) -> bool
{
if (cachedScriptContext->IsClosed())
if (this->IsSelfOrScriptContextClosed())
{
// ScriptContext could be closed in previous iteration
hr = E_FAIL;
return true;
}
Expand Down Expand Up @@ -261,9 +276,8 @@ namespace Js
bool fHasDoneSourceRundown = false;
for (int i = 0; i < pFunctionsToRegister->Count(); i++)
{
if (cachedScriptContext->IsClosed())
if (this->IsSelfOrScriptContextClosed())
{
// ScriptContext could be closed in previous iteration
hr = E_FAIL;
return true;
}
Expand All @@ -276,7 +290,7 @@ namespace Js

if (shouldReparseFunctions)
{
BEGIN_JS_RUNTIME_CALL_EX_AND_TRANSLATE_EXCEPTION_AND_ERROROBJECT_TO_HRESULT_NESTED(cachedScriptContext, false)
BEGIN_JS_RUNTIME_CALL_EX_AND_TRANSLATE_EXCEPTION_AND_ERROROBJECT_TO_HRESULT_NESTED(this->scriptContext, false)
{
functionInfo->GetParseableFunctionInfo()->Parse();
// This is the first call to the function, ensure dynamic profile info
Expand All @@ -293,7 +307,7 @@ namespace Js
// Parsing the function may change its FunctionProxy.
Js::ParseableFunctionInfo *parseableFunctionInfo = functionInfo->GetParseableFunctionInfo();

if (!fHasDoneSourceRundown && shouldPerformSourceRundown && !cachedScriptContext->IsClosed())
if (!fHasDoneSourceRundown && shouldPerformSourceRundown && !this->IsSelfOrScriptContextClosed())
{
BEGIN_TRANSLATE_OOM_TO_HRESULT_NESTED
{
Expand Down Expand Up @@ -321,13 +335,13 @@ namespace Js
return false;
});

if (!cachedScriptContext->IsClosed())
if (!this->IsSelfOrScriptContextClosed())
{
if (shouldPerformSourceRundown && cachedScriptContext->HaveCalleeSources() && this->hostDebugContext != nullptr)
if (shouldPerformSourceRundown && this->scriptContext->HaveCalleeSources() && this->hostDebugContext != nullptr)
{
cachedScriptContext->MapCalleeSources([=](Js::Utf8SourceInfo* calleeSourceInfo)
this->scriptContext->MapCalleeSources([=](Js::Utf8SourceInfo* calleeSourceInfo)
{
if (!cachedScriptContext->IsClosed())
if (!this->IsSelfOrScriptContextClosed())
{
// This call goes out of engine
this->hostDebugContext->ReParentToCaller(calleeSourceInfo);
Expand Down
10 changes: 6 additions & 4 deletions deps/chakrashim/core/lib/Runtime/Debug/DebugContext.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ namespace Js
void Initialize();
HRESULT RundownSourcesAndReparse(bool shouldPerformSourceRundown, bool shouldReparseFunctions);
void RegisterFunction(Js::ParseableFunctionInfo * func, LPCWSTR title);
bool IsClosed() const { return this->isClosed; };
bool IsSelfOrScriptContextClosed() const;
void Close();
void SetHostDebugContext(HostDebugContext * hostDebugContext);

Expand All @@ -61,8 +63,7 @@ namespace Js
void SetIsDebuggerRecording(bool isDebuggerRecording) { this->isDebuggerRecording = isDebuggerRecording; }

ProbeContainer* GetProbeContainer() const { return this->diagProbesContainer; }

HostDebugContext * GetHostDebugContext() const { return hostDebugContext; }
HostDebugContext * GetHostDebugContext() const { return this->hostDebugContext; }

bool GetIsReparsingSource() const { return this->isReparsingSource; }

Expand All @@ -71,8 +72,9 @@ namespace Js
HostDebugContext* hostDebugContext;
ProbeContainer* diagProbesContainer;
DebuggerMode debuggerMode;
bool isDebuggerRecording;
bool isReparsingSource;
bool isClosed : 1;
bool isDebuggerRecording : 1;
bool isReparsingSource : 1;

// Private Functions
void WalkAndAddUtf8SourceInfo(Js::Utf8SourceInfo* sourceInfo, JsUtil::List<Js::Utf8SourceInfo *, Recycler, false, Js::CopyRemovePolicy, RecyclerPointerComparer> *utf8SourceInfoList);
Expand Down

0 comments on commit bf0b020

Please sign in to comment.