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

fix memory leak #14145

Closed
wants to merge 4 commits into from
Closed

fix memory leak #14145

wants to merge 4 commits into from

Conversation

yk3372
Copy link
Contributor

@yk3372 yk3372 commented May 24, 2017

Signed-off-by: yk3372 [email protected]

Motivation (required)

What existing problem does the pull request solve?

ViewManagersPropertyCache, ViewManagerPropertyUpdater, MessageQueueThreadRegistry static field not release when ReactInstanceManager called destroy.

Test Plan (required)

I use this url to integrate RN:
http://facebook.github.io/react-native/docs/integration-with-existing-apps.html
and in Activity's onDestroy function add the follow code to release RN:

mReactInstanceManager.destroy();
mReactInstanceManager = null;

and then when I exit activity, find the static field not release.
the follow is screen shot:
before:
2017-05-23 17 41 16
after:
2017-05-23 17 38 49

Signed-off-by: yk3372 <[email protected]>
@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 24, 2017
@yk3372
Copy link
Contributor Author

yk3372 commented May 24, 2017

The MessageQueueThread maybe reference NativeModuleCallExceptionHandler, and NativeModuleCallExceptionHandler is inner class has CatalystInstanceImpl‘s reference.
This resulting in CatalystInstanceImpl can not release.
Except for the above, to release the static reference is no effect when destroy RN.

#14127

@javache

@yk3372
Copy link
Contributor Author

yk3372 commented May 24, 2017

Why the CircleCI test failed? The package exist and can local build success.

@javache
Copy link
Member

javache commented May 24, 2017

I have some changes up for review internally that remove MessageQueueThreadRegistry completely (D5111966), let's land that first and rebase your change on that.

Summary:
English :)
Closes #14138

Differential Revision: D5114337

Pulled By: javache

fbshipit-source-id: 73cfc4d3b31b4a86ac6fec0293a4f1246c911414
@yk3372
Copy link
Contributor Author

yk3372 commented May 24, 2017

ok, Thanks

@javache
Copy link
Member

javache commented May 24, 2017

That has landed as 275ba31 Can you please rebase?

@yk3372 yk3372 closed this May 25, 2017
@yk3372 yk3372 deleted the my branch May 25, 2017 01:06
@yk3372 yk3372 mentioned this pull request May 25, 2017
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants