Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix memory leaks in Debugger #1411

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Fix memory leaks in Debugger #1411

merged 1 commit into from
Jan 17, 2025

Conversation

clover2123
Copy link
Contributor

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

@clover2123
Copy link
Contributor Author

The main cause of this memory leak is correlation between GC-Object and Non-GC-Object

ByteCodeBlock : GC-Object
Debugger::BreakpointLocationsInfo : Non-GC-Object

Debugger::BreakpointLocationsInfo should be properly deleted.

escargot/src/debugger/Debugger.h

Lines 109 to 112 in 19498b4

void clearParsingData()
{
m_breakpointLocationsVector.clear();
}

At here, m_breakpointLocationsVector vector has addresses of Debugger::BreakpointLocationsInfo, but only these addresses are cleared instead of deleting each Debugger::BreakpointLocationsInfo

clover2123
clover2123 commented Jan 16, 2025
View reviewed changes
src/api/EscargotPublic.cpp
Comment on lines +1855 to +1861
void DebuggerC::deleteClient()
{
if (m_debuggerClient) {
// delete DebuggerClient that was created and delivered from the user
delete m_debuggerClient;
m_debuggerClient = nullptr;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_debuggerClient is a non-GC-object created by the user, so it should be deleted as well

@clover2123
Copy link
Contributor Author

@zherczeg
I found this memory leak by chance.
Would you please review this patch?

zherczeg
zherczeg approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for fixing this.

@clover2123
Copy link
Contributor Author

@zherczeg thanks for the quick review!

@clover2123 clover2123 marked this pull request as ready for review January 16, 2025 08:16
@clover2123
Fix memory leaks in Debugger
bfe7343 
* shared structure `BreakpointLocationsInfo` between debuggger and ByteCodeBlock can cause memory leaks
* correctly delete each `BreakpointLocationsInfo`

Signed-off-by: HyukWoo Park <[email protected]>
@ksh8281 ksh8281 merged commit a57df75 into Samsung:master Jan 17, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zherczeg zherczeg zherczeg approved these changes

@ksh8281 ksh8281 Awaiting requested review from ksh8281 ksh8281 is a code owner

@bbrto21 bbrto21 Awaiting requested review from bbrto21 bbrto21 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clover2123 @zherczeg @ksh8281