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.

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

Incorrect ref API usage fixed. #20913

Closed

Conversation

dryganets
Copy link
Contributor

@dryganets dryganets commented Aug 29, 2018

release method of local_ref and global_ref doesn't call deallocator, in fact, it leaves the caller responsible for deletion of the reference, while otherwise the reference is released on scope left.

Fixes #18292.

Test Plan:

If there is a mistake in the change it won't even run.
Pass the big array (bigger than 512 elements) and call importTypes method on it. If it doesn't crash the problem is fixed.

Release Notes:

[ANDROID] [BUGFIX] [JNI] - Fixes spontaneous local_ref table overflow.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 29, 2018
@dryganets
Copy link
Contributor Author

Holdon on that, I need to finish some testing, I might go too aggressive - not sure about the global_ref yet, need to make sure it doesn't break anything.

@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR may have incorrectly formatted Release Notes.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added Core Team Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 29, 2018
… global_ref doesn't call deallocator, in fact it makes the caller responsible for deletion of the reference, while otherwise the reference is released on scope leave.
@dryganets dryganets force-pushed the sergeyd/reference-leaks-fixes branch from 74cd97f to cc26fea Compare August 29, 2018 21:57
@dryganets
Copy link
Contributor Author

I left only fix relevant to the issue. Everything else was correct.

@react-native-bot react-native-bot added ✅Release Notes and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 30, 2018
@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 31, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gengjiawen pushed a commit to gengjiawen/react-native that referenced this pull request Sep 14, 2018
Summary:
release method of local_ref and global_ref doesn't call deallocator, in fact, it leaves the caller responsible for deletion of the reference, while otherwise the reference is released on scope left.

Fixes facebook#18292.
Pull Request resolved: facebook#20913

Differential Revision: D9616237

Pulled By: hramos

fbshipit-source-id: 021aa3e4f039e6b7a98da3e4224c1ee49d5a4921
kelset pushed a commit that referenced this pull request Nov 9, 2018
Summary:
release method of local_ref and global_ref doesn't call deallocator, in fact, it leaves the caller responsible for deletion of the reference, while otherwise the reference is released on scope left.

Fixes #18292.
Pull Request resolved: #20913

Differential Revision: D9616237

Pulled By: hramos

fbshipit-source-id: 021aa3e4f039e6b7a98da3e4224c1ee49d5a4921
@hramos hramos added the Merged This PR has been merged. label Mar 8, 2019
@react-native-bot react-native-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 8, 2019
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
release method of local_ref and global_ref doesn't call deallocator, in fact, it leaves the caller responsible for deletion of the reference, while otherwise the reference is released on scope left.

Fixes facebook#18292.
Pull Request resolved: facebook#20913

Differential Revision: D9616237

Pulled By: hramos

fbshipit-source-id: 021aa3e4f039e6b7a98da3e4224c1ee49d5a4921
@TheSavior TheSavior added p: Microsoft Partner: Microsoft Partner and removed Partner p: Microsoft Partner: Microsoft labels Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[0.54.0][Android] Crash in ReadableNativeArray.getType when size of ReadableNativeArray > 512
6 participants