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 JavaScript callback memory leak issue #81105

Merged
merged 1 commit into from
Sep 7, 2023
Merged

Fix JavaScript callback memory leak issue #81105

merged 1 commit into from
Sep 7, 2023

Conversation

SysError99
Copy link
Contributor

Follows #81090 for master branch. I noticed that in master it supports returning value so I put the unref the last until value return occurs.

@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@akien-mga akien-mga added this to the 4.2 milestone Aug 29, 2023
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 29, 2023
@akien-mga akien-mga requested a review from Faless August 29, 2023 07:12
@SysError99
Copy link
Contributor Author

Yes I didn't set Git credentials or set wrongly. I'll have a look and have a check on it again

Faless
Faless approved these changes Sep 4, 2023
View reviewed changes
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Looking great 🏆
Thank you for debugging and fixing this 🥳 !

Just a small nit pick.... the argsProxy.unref(); could go right after the func( call.

platform/web/js/libs/library_godot_javascript_singleton.js Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

@SysError99 SysError99 force-pushed the master-fix-js-mem-leak branch from d7053bc to f528ffb Compare September 7, 2023 12:34
@SysError99 SysError99 requested review from a team as code owners September 7, 2023 12:34
@AThousandShips
Copy link
Member

You have reset the authors of several commits when rebasing

@AThousandShips AThousandShips removed request for a team September 7, 2023 12:51
@SysError99 SysError99 force-pushed the master-fix-js-mem-leak branch from f528ffb to 5aec6c9 Compare September 7, 2023 12:52
@SysError99 SysError99 force-pushed the master-fix-js-mem-leak branch from 5aec6c9 to 6a90164 Compare September 7, 2023 13:00
@SysError99
Copy link
Contributor Author

I tried to rebase it correctly but seemed like Faless's credit is missing :(

@akien-mga
Copy link
Member

I tried to rebase it correctly but seemed like Faless's credit is missing :(

I think it's fine, a small nitpick doesn't necessarily require co-crediting.

@akien-mga akien-mga merged commit 2cd740f into godotengine:master Sep 7, 2023
@akien-mga
Copy link
Member

Thanks!

@SysError99 SysError99 deleted the master-fix-js-mem-leak branch September 7, 2023 14:40
This was referenced Oct 3, 2023
Closed
Merged
@akien-mga
Copy link
Member

akien-mga commented Oct 3, 2023 •
edited
Loading

Note for a potential cherrypick: Regression fix should be included #82736.

The 3.x version already seems correct: #81090.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Faless Faless Faless approved these changes

Assignees
No one assigned
Labels
bug platform:web topic:porting
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@SysError99 @akien-mga @YuriSizov @AThousandShips @Faless