From a57df7576be63b318ad310947026a0212b20b150 Mon Sep 17 00:00:00 2001 From: HyukWoo Park Date: Thu, 16 Jan 2025 15:57:58 +0900 Subject: [PATCH] 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 --- src/api/EscargotPublic.cpp | 29 ++++++++++++++++++++++++++- src/api/EscargotPublic.h | 2 ++ src/debugger/Debugger.h | 7 ++++++- src/interpreter/ByteCode.cpp | 8 +++++--- src/interpreter/ByteCodeGenerator.cpp | 13 +++++++++++- src/interpreter/ByteCodeGenerator.h | 4 ++++ test/cctest/testapi.cpp | 5 +++++ 7 files changed, 62 insertions(+), 6 deletions(-) diff --git a/src/api/EscargotPublic.cpp b/src/api/EscargotPublic.cpp index f2e127775..93525c4da 100644 --- a/src/api/EscargotPublic.cpp +++ b/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(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(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, 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 diff --git a/src/api/EscargotPublic.h b/src/api/EscargotPublic.h index fc3b822a1..adb64fcc5 100644 --- a/src/api/EscargotPublic.h +++ b/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& 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); diff --git a/src/debugger/Debugger.h b/src/debugger/Debugger.h index 980590267..442bb130c 100644 --- a/src/debugger/Debugger.h +++ b/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; diff --git a/src/interpreter/ByteCode.cpp b/src/interpreter/ByteCode.cpp index b806e695b..14ec2078a 100644 --- a/src/interpreter/ByteCode.cpp +++ b/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(); diff --git a/src/interpreter/ByteCodeGenerator.cpp b/src/interpreter/ByteCodeGenerator.cpp index d3aed89c3..fd9130f28 100644 --- a/src/interpreter/ByteCodeGenerator.cpp +++ b/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(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 */ diff --git a/src/interpreter/ByteCodeGenerator.h b/src/interpreter/ByteCodeGenerator.h index 3307748a8..2357a928a 100644 --- a/src/interpreter/ByteCodeGenerator.h +++ b/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 */ diff --git a/test/cctest/testapi.cpp b/test/cctest/testapi.cpp index 2d48dd13f..34b12dea1 100644 --- a/test/cctest/testapi.cpp +++ b/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(); }