Fix memory leaks in Debugger

* shared structure `BreakpointLocationsInfo` between debuggger and ByteCodeBlock can cause memory leaks
* correctly delete each `BreakpointLocationsInfo`

Signed-off-by: HyukWoo Park <[email protected]>

Author: clover2123

src/api/EscargotPublic.cpp

@@ -1718,6 +1718,7 @@ class DebuggerC : public Debugger {
     virtual void consoleOut(String* output) override;
     virtual String* getClientSource(String** sourceName) override;
     virtual bool getWaitBeforeExitClient() override;
+    virtual void deleteClient() override;
 
     DebuggerC(DebuggerOperationsRef::DebuggerClient* debuggerClient, Context* context)
         : m_debuggerClient(debuggerClient)
@@ -1821,7 +1822,11 @@ void DebuggerC::stopAtBreakpoint(ByteCodeBlock* byteCodeBlock, uint32_t offset,
 
 void DebuggerC::byteCodeReleaseNotification(ByteCodeBlock* byteCodeBlock)
 {
-    m_debuggerClient->codeReleased(reinterpret_cast<DebuggerOperationsRef::WeakCodeRef*>(byteCodeBlock));
+    if (m_debuggerClient) {
+        // Debugger could be removed earlier when this function called
+        // e.g. release global objects such as Context and VMInstance at the end of execution
+        m_debuggerClient->codeReleased(reinterpret_cast<DebuggerOperationsRef::WeakCodeRef*>(byteCodeBlock));
+    }
 }
 
 void DebuggerC::exceptionCaught(String* message, SavedStackTraceDataVector& exceptionTrace)
@@ -1847,6 +1852,15 @@ bool DebuggerC::getWaitBeforeExitClient()
     return false;
 }
 
+void DebuggerC::deleteClient()
+{
+    if (m_debuggerClient) {
+        // delete DebuggerClient that was created and delivered from the user
+        delete m_debuggerClient;
+        m_debuggerClient = nullptr;
+    }
+}
+
 bool DebuggerC::processEvents(ExecutionState* state, Optional<ByteCodeBlock*> byteCodeBlock, bool isBlockingRequest)
 {
     UNUSED_PARAMETER(state);
@@ -3093,6 +3107,19 @@ bool ContextRef::initDebugger(DebuggerOperationsRef::DebuggerClient* debuggerCli
 #endif /* ESCARGOT_DEBUGGER */
 }
 
+bool ContextRef::disableDebugger()
+{
+#ifdef ESCARGOT_DEBUGGER
+    Context* context = toImpl(this);
+    if (context->debugger()) {
+        context->debugger()->deleteClient();
+    }
+    return true;
+#else
+    return false;
+#endif
+}
+
 bool ContextRef::initDebuggerRemote(const char* options)
 {
 #ifdef ESCARGOT_DEBUGGER

src/api/EscargotPublic.h

@@ -870,6 +870,7 @@ class ESCARGOT_EXPORT DebuggerOperationsRef {
     // Base class for debugger callbacks
     class DebuggerClient {
     public:
+        virtual ~DebuggerClient(){};
         virtual void parseCompleted(StringRef* source, StringRef* srcName, std::vector<DebuggerOperationsRef::BreakpointLocationsInfo*>& breakpointLocationsVector) = 0;
         virtual void parseError(StringRef* source, StringRef* srcName, StringRef* error) = 0;
         virtual void codeReleased(WeakCodeRef* weakCodeRef) = 0;
@@ -900,6 +901,7 @@ class ESCARGOT_EXPORT ContextRef {
     void throwException(ValueRef* exceptionValue);
 
     bool initDebugger(DebuggerOperationsRef::DebuggerClient* debuggerClient);
+    bool disableDebugger();
     // available options(separator is ';')
     // "--port=6501", default for TCP debugger
     bool initDebuggerRemote(const char* options);

src/debugger/Debugger.h

@@ -46,7 +46,7 @@ class InterpretedCodeBlock;
 
 class Debugger : public gc {
 public:
-    // The following code is the sam as in EscargotPublic.h
+    // The following code is the same as in EscargotPublic.h
     class WeakCodeRef;
 
     struct BreakpointLocation {
@@ -108,6 +108,10 @@ class Debugger : public gc {
 
     void clearParsingData()
     {
+        for (size_t i = 0; i < m_breakpointLocationsVector.size(); i++) {
+            // delete each shared Debugger::BreakpointLocationsInfo here
+            delete m_breakpointLocationsVector[i];
+        }
         m_breakpointLocationsVector.clear();
     }
 
@@ -163,6 +167,7 @@ class Debugger : public gc {
     virtual void consoleOut(String* output) = 0;
     virtual String* getClientSource(String** sourceName) = 0;
     virtual bool getWaitBeforeExitClient() = 0;
+    virtual void deleteClient() {}
     virtual bool skipSourceCode(String* srcName) const
     {
         return false;

src/interpreter/ByteCode.cpp

@@ -111,9 +111,11 @@ ByteCodeBlock::ByteCodeBlock()
 static void clearByteCodeBlock(ByteCodeBlock* self)
 {
 #ifdef ESCARGOT_DEBUGGER
-    Debugger* debugger = self->codeBlock()->context()->debugger();
-    if (debugger != nullptr && self->codeBlock()->markDebugging()) {
-        debugger->byteCodeReleaseNotification(self);
+    if (!self->m_isOwnerMayFreed) {
+        Debugger* debugger = self->codeBlock()->context()->debugger();
+        if (debugger != nullptr && self->codeBlock()->markDebugging()) {
+            debugger->byteCodeReleaseNotification(self);
+        }
     }
 #endif
     self->m_code.clear();

src/interpreter/ByteCodeGenerator.cpp

@@ -210,11 +210,22 @@ ByteCodeBreakpointContext::ByteCodeBreakpointContext(Debugger* debugger, Interpr
     : m_lastBreakpointLineOffset(0)
     , m_lastBreakpointIndexOffset(0)
     , m_originSourceLineOffset(codeBlock->script()->originSourceLineOffset())
-    , m_breakpointLocations()
+    , m_breakpointLocations(nullptr)
+    , m_sharedWithDebugger(false)
 {
     m_breakpointLocations = new Debugger::BreakpointLocationsInfo(reinterpret_cast<Debugger::WeakCodeRef*>(codeBlock));
     if (debugger && codeBlock->markDebugging() && addBreakpointLocationsInfoToDebugger) {
         debugger->appendBreakpointLocations(m_breakpointLocations);
+        m_sharedWithDebugger = true;
+    }
+}
+
+ByteCodeBreakpointContext::~ByteCodeBreakpointContext()
+{
+    if (!m_sharedWithDebugger) {
+        // directly delete each BreakpointLocationsInfo
+        // because BreakpointLocationsInfo is not shared with Debugger
+        delete m_breakpointLocations;
     }
 }
 #endif /* ESCARGOT_DEBUGGER */

src/interpreter/ByteCodeGenerator.h

@@ -58,8 +58,12 @@ struct ByteCodeBreakpointContext {
     size_t m_originSourceLineOffset; // original source's start line offset
     Debugger::BreakpointLocationsInfo* m_breakpointLocations;
     bool m_parsingEnabled;
+    bool m_sharedWithDebugger;
 
     ByteCodeBreakpointContext(Debugger* debugger, InterpretedCodeBlock* codeBlock, bool addBreakpointLocationsInfoToDebugger = true);
+    ~ByteCodeBreakpointContext();
+
+    MAKE_STACK_ALLOCATED();
 };
 #endif /* ESCARGOT_DEBUGGER */
 

test/cctest/testapi.cpp

@@ -665,6 +665,7 @@ TEST(FunctionObject, Consturct)
     Evaluator::execute(g_context.get(), [](ExecutionStateRef* state, FunctionObjectRef* fn) -> ValueRef* {
         ObjectRef* obj = fn->construct(state, 0, nullptr)->asObject();
         EXPECT_TRUE(obj->instanceOf(state, fn));
+        return ValueRef::createUndefined();
     },
                        fn);
 }
@@ -2873,6 +2874,8 @@ TEST(Debugger, Basic)
     EXPECT_EQ(debuggerParseErrorCount, 1);
     EXPECT_EQ(debuggerTest->stopAtBreakpointCount, 5);
 
+    context->disableDebugger();
+
     context.release();
     instance.release();
 }
@@ -2985,6 +2988,8 @@ TEST(Debugger, ObjectStore)
     source = StringRef::createFromUTF8(debuggerSourceString2, sizeof(debuggerSourceString2) - 1);
     evalScript(context, source, fileName, false);
 
+    context->disableDebugger();
+
     context.release();
     instance.release();
 }