[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.

lib/Runtime/Base/ScriptContext.cpp

@@ -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)
         {
@@ -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)
@@ -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();
         }
@@ -5803,7 +5823,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
 
     bool ScriptContext::IsScriptContextInDebugMode() const
     {
-        if (this->debugContext != nullptr)
+        if (this->GetDebugContext() != nullptr)
         {
             return this->GetDebugContext()->IsDebugContextInDebugMode();
         }
@@ -5812,7 +5832,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
 
     bool ScriptContext::IsScriptContextInSourceRundownOrDebugMode() const
     {
-        if (this->debugContext != nullptr)
+        if (this->GetDebugContext() != nullptr)
         {
             return this->GetDebugContext()->IsDebugContextInSourceRundownOrDebugMode();
         }
@@ -5821,7 +5841,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
 
     bool ScriptContext::IsDebuggerRecording() const
     {
-        if (this->debugContext != nullptr)
+        if (this->GetDebugContext() != nullptr)
         {
             return this->GetDebugContext()->IsDebuggerRecording();
         }
@@ -5830,7 +5850,7 @@ void ScriptContext::RegisterPrototypeChainEnsuredToHaveOnlyWritableDataPropertie
 
     void ScriptContext::SetIsDebuggerRecording(bool isDebuggerRecording)
     {
-        if (this->debugContext != nullptr)
+        if (this->GetDebugContext() != nullptr)
         {
             this->GetDebugContext()->SetIsDebuggerRecording(isDebuggerRecording);
         }

lib/Runtime/Base/ScriptContext.h

@@ -993,7 +993,7 @@ private:
 #endif
 
         bool IsDebugContextInitialized() const { return this->isDebugContextInitialized; }
-        DebugContext* GetDebugContext() const { return this->debugContext; }
+        DebugContext* GetDebugContext() const;
         CriticalSection* GetDebugContextCloseCS() { return &debugContextCloseCS; }
 
         uint callCount;

lib/Runtime/Debug/DebugContext.cpp

@@ -10,6 +10,7 @@ namespace Js
         scriptContext(scriptContext),
         hostDebugContext(nullptr),
         diagProbesContainer(nullptr),
+        isClosed(false),
         debuggerMode(DebuggerMode::NotDebugging),
         isDebuggerRecording(true),
         isReparsingSource(false)
@@ -19,7 +20,7 @@ namespace Js
 
     DebugContext::~DebugContext()
     {
-        Assert(this->scriptContext == nullptr);
+        Assert(this->scriptContext != nullptr);
         Assert(this->hostDebugContext == nullptr);
         Assert(this->diagProbesContainer == nullptr);
     }
@@ -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)
         {
@@ -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);
@@ -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;
     }
 
@@ -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;
             }
@@ -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;
                 }
@@ -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
@@ -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
                     {
@@ -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);

lib/Runtime/Debug/DebugContext.h

@@ -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);
 
@@ -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; }
 
@@ -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);