Skip to content

Commit be5c231

Browse files
agarwal-sandeepagarwal-sandeep
committed
[1.6>1.7] [MERGE #3485 @agarwal-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.
2 parents c19c1c0 + b5cb587 commit be5c231

File tree

4 files changed

+70
-34
lines changed

4 files changed

+70
-34
lines changed

‎lib/Runtime/Base/ScriptContext.cpp

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,13 @@ namespace Js
482482
}
483483
#endif
484484

485+
if (this->debugContext != nullptr)
486+
{
487+
Assert(this->debugContext->IsClosed());
488+
HeapDelete(this->debugContext);
489+
this->debugContext = nullptr;
490+
}
491+
485492
#if ENABLE_NATIVE_CODEGEN
486493
if (this->nativeCodeGen != nullptr)
487494
{
@@ -665,21 +672,20 @@ namespace Js
665672
#endif
666673

667674
this->EnsureClearDebugDocument();
675+
668676
if (this->debugContext != nullptr)
669677
{
670-
if(this->debugContext->GetProbeContainer())
678+
if (this->debugContext->GetProbeContainer() != nullptr)
671679
{
672-
this->debugContext->GetProbeContainer()->UninstallInlineBreakpointProbe(NULL);
680+
this->debugContext->GetProbeContainer()->UninstallInlineBreakpointProbe(nullptr);
673681
this->debugContext->GetProbeContainer()->UninstallDebuggerScriptOptionCallback();
674682
}
675683

676-
// Guard the closing and deleting of DebugContext as in meantime PDM might
677-
// call OnBreakFlagChange
684+
// Guard the closing DebugContext as in meantime PDM might call OnBreakFlagChange
678685
AutoCriticalSection autoDebugContextCloseCS(&debugContextCloseCS);
679-
DebugContext* tempDebugContext = this->debugContext;
680-
this->debugContext = nullptr;
681-
tempDebugContext->Close();
682-
HeapDelete(tempDebugContext);
686+
this->debugContext->Close();
687+
// Not deleting debugContext here as Close above will clear all memory debugContext allocated.
688+
// Actual deletion of debugContext will happen in ScriptContext destructor
683689
}
684690

685691
if (this->diagnosticArena != nullptr)
@@ -5792,9 +5798,23 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
57925798
}
57935799
#endif
57945800

5801+
DebugContext* ScriptContext::GetDebugContext() const
5802+
{
5803+
Assert(this->debugContext != nullptr);
5804+
5805+
if (this->debugContext->IsClosed())
5806+
{
5807+
// Once DebugContext is closed we should assume it's not there
5808+
// The actual deletion of debugContext happens in ScriptContext destructor
5809+
return nullptr;
5810+
}
5811+
5812+
return this->debugContext;
5813+
}
5814+
57955815
bool ScriptContext::IsScriptContextInNonDebugMode() const
57965816
{
5797-
if (this->debugContext != nullptr)
5817+
if (this->GetDebugContext() != nullptr)
57985818
{
57995819
return this->GetDebugContext()->IsDebugContextInNonDebugMode();
58005820
}
@@ -5803,7 +5823,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
58035823

58045824
bool ScriptContext::IsScriptContextInDebugMode() const
58055825
{
5806-
if (this->debugContext != nullptr)
5826+
if (this->GetDebugContext() != nullptr)
58075827
{
58085828
return this->GetDebugContext()->IsDebugContextInDebugMode();
58095829
}
@@ -5812,7 +5832,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
58125832

58135833
bool ScriptContext::IsScriptContextInSourceRundownOrDebugMode() const
58145834
{
5815-
if (this->debugContext != nullptr)
5835+
if (this->GetDebugContext() != nullptr)
58165836
{
58175837
return this->GetDebugContext()->IsDebugContextInSourceRundownOrDebugMode();
58185838
}
@@ -5821,7 +5841,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
58215841

58225842
bool ScriptContext::IsDebuggerRecording() const
58235843
{
5824-
if (this->debugContext != nullptr)
5844+
if (this->GetDebugContext() != nullptr)
58255845
{
58265846
return this->GetDebugContext()->IsDebuggerRecording();
58275847
}
@@ -5830,7 +5850,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
58305850

58315851
void ScriptContext::SetIsDebuggerRecording(bool isDebuggerRecording)
58325852
{
5833-
if (this->debugContext != nullptr)
5853+
if (this->GetDebugContext() != nullptr)
58345854
{
58355855
this->GetDebugContext()->SetIsDebuggerRecording(isDebuggerRecording);
58365856
}

‎lib/Runtime/Base/ScriptContext.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -993,7 +993,7 @@ namespace Js
993993
#endif
994994

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

999999
uint callCount;

‎lib/Runtime/Debug/DebugContext.cpp

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ namespace Js
1010
scriptContext(scriptContext),
1111
hostDebugContext(nullptr),
1212
diagProbesContainer(nullptr),
13+
isClosed(false),
1314
debuggerMode(DebuggerMode::NotDebugging),
1415
isDebuggerRecording(true),
1516
isReparsingSource(false)
@@ -19,7 +20,7 @@ namespace Js
1920

2021
DebugContext::~DebugContext()
2122
{
22-
Assert(this->scriptContext == nullptr);
23+
Assert(this->scriptContext != nullptr);
2324
Assert(this->hostDebugContext == nullptr);
2425
Assert(this->diagProbesContainer == nullptr);
2526
}
@@ -33,8 +34,18 @@ namespace Js
3334

3435
void DebugContext::Close()
3536
{
37+
if (this->isClosed)
38+
{
39+
return;
40+
}
41+
42+
AssertMsg(this->scriptContext->IsActuallyClosed(), "Closing DebugContext before ScriptContext close might have consequences");
43+
44+
this->isClosed = true;
45+
46+
// Release all memory and do all cleanup. No operation should be done after isClosed is set
47+
3648
Assert(this->scriptContext != nullptr);
37-
this->scriptContext = nullptr;
3849

3950
if (this->diagProbesContainer != nullptr)
4051
{
@@ -50,6 +61,11 @@ namespace Js
5061
}
5162
}
5263

64+
bool DebugContext::IsSelfOrScriptContextClosed() const
65+
{
66+
return (this->IsClosed() || this->scriptContext->IsClosed());
67+
}
68+
5369
void DebugContext::SetHostDebugContext(HostDebugContext * hostDebugContext)
5470
{
5571
Assert(this->hostDebugContext == nullptr);
@@ -60,10 +76,13 @@ namespace Js
6076

6177
bool DebugContext::CanRegisterFunction() const
6278
{
63-
if (this->hostDebugContext == nullptr || this->scriptContext == nullptr || this->scriptContext->IsClosed() || this->IsDebugContextInNonDebugMode())
79+
if (this->IsSelfOrScriptContextClosed() ||
80+
this->hostDebugContext == nullptr ||
81+
this->IsDebugContextInNonDebugMode())
6482
{
6583
return false;
6684
}
85+
6786
return true;
6887
}
6988

@@ -195,14 +214,10 @@ namespace Js
195214
return hr;
196215
}
197216

198-
// Cache ScriptContext as multiple calls below can go out of engine and ScriptContext can be closed which will delete DebugContext
199-
Js::ScriptContext* cachedScriptContext = this->scriptContext;
200-
201217
utf8SourceInfoList->MapUntil([&](int index, Js::Utf8SourceInfo * sourceInfo) -> bool
202218
{
203-
if (cachedScriptContext->IsClosed())
219+
if (this->IsSelfOrScriptContextClosed())
204220
{
205-
// ScriptContext could be closed in previous iteration
206221
hr = E_FAIL;
207222
return true;
208223
}
@@ -261,9 +276,8 @@ namespace Js
261276
bool fHasDoneSourceRundown = false;
262277
for (int i = 0; i < pFunctionsToRegister->Count(); i++)
263278
{
264-
if (cachedScriptContext->IsClosed())
279+
if (this->IsSelfOrScriptContextClosed())
265280
{
266-
// ScriptContext could be closed in previous iteration
267281
hr = E_FAIL;
268282
return true;
269283
}
@@ -276,7 +290,7 @@ namespace Js
276290

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

296-
if (!fHasDoneSourceRundown && shouldPerformSourceRundown && !cachedScriptContext->IsClosed())
310+
if (!fHasDoneSourceRundown && shouldPerformSourceRundown && !this->IsSelfOrScriptContextClosed())
297311
{
298312
BEGIN_TRANSLATE_OOM_TO_HRESULT_NESTED
299313
{
@@ -321,13 +335,13 @@ namespace Js
321335
return false;
322336
});
323337

324-
if (!cachedScriptContext->IsClosed())
338+
if (!this->IsSelfOrScriptContextClosed())
325339
{
326-
if (shouldPerformSourceRundown && cachedScriptContext->HaveCalleeSources() && this->hostDebugContext != nullptr)
340+
if (shouldPerformSourceRundown && this->scriptContext->HaveCalleeSources() && this->hostDebugContext != nullptr)
327341
{
328-
cachedScriptContext->MapCalleeSources([=](Js::Utf8SourceInfo* calleeSourceInfo)
342+
this->scriptContext->MapCalleeSources([=](Js::Utf8SourceInfo* calleeSourceInfo)
329343
{
330-
if (!cachedScriptContext->IsClosed())
344+
if (!this->IsSelfOrScriptContextClosed())
331345
{
332346
// This call goes out of engine
333347
this->hostDebugContext->ReParentToCaller(calleeSourceInfo);

‎lib/Runtime/Debug/DebugContext.h

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ namespace Js
4747
void Initialize();
4848
HRESULT RundownSourcesAndReparse(bool shouldPerformSourceRundown, bool shouldReparseFunctions);
4949
void RegisterFunction(Js::ParseableFunctionInfo * func, LPCWSTR title);
50+
bool IsClosed() const { return this->isClosed; };
51+
bool IsSelfOrScriptContextClosed() const;
5052
void Close();
5153
void SetHostDebugContext(HostDebugContext * hostDebugContext);
5254

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

6365
ProbeContainer* GetProbeContainer() const { return this->diagProbesContainer; }
64-
65-
HostDebugContext * GetHostDebugContext() const { return hostDebugContext; }
66+
HostDebugContext * GetHostDebugContext() const { return this->hostDebugContext; }
6667

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

@@ -71,8 +72,9 @@ namespace Js
7172
HostDebugContext* hostDebugContext;
7273
ProbeContainer* diagProbesContainer;
7374
DebuggerMode debuggerMode;
74-
bool isDebuggerRecording;
75-
bool isReparsingSource;
75+
bool isClosed : 1;
76+
bool isDebuggerRecording : 1;
77+
bool isReparsingSource : 1;
7678

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

0 commit comments

Comments
 (0)