-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
issue#939, fix multiple-instance memory leak #1130
Conversation
@gpeal when you get a chance, can you review this? |
paused = true; | ||
synchronized (AirMapView.this) { | ||
AirMapView.this.onPause(); | ||
paused = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move setting paused to true and false into onPause and onResume? The boolean logic being here breaks now that you call onPause separately.
context.removeLifecycleEventListener(lifecycleListener); | ||
lifecycleListener = null; | ||
} | ||
if(!paused) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would destroy get called without pause?
} | ||
if (!destroyed) { | ||
onDestroy(); | ||
destroyed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just set destroyed to true at the top of the method. This will prevent future logic errors if you ever return early or anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also start the method with:
if (destroyed) {
return;
}
destroyed = true;
} | ||
if (!destroyed) { | ||
onDestroy(); | ||
destroyed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also start the method with:
if (destroyed) {
return;
}
destroyed = true;
onDestroy is final method so I can't override it. | ||
*/ | ||
public synchronized void doDestroy() { | ||
if (lifecycleListener != null && context != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this gets called while the map is initializing before onMapReady is called? Can you cancel the initialization? Maybe return early from onMapReady if destroyed?
Hi @lelandrichardson, thanks. @gpeal 's comments make sense to me (but I haven't got a chance to change :( ). If your followup PR contains some changes requiring some real case testing, please let me know, I will apply it to our app and let the testers have a try. |
Address PR feedback from #1130
* commit '3469a224d2e38c5e6b81e03344bc211d8fbb28f1': Address PR feedback from react-native-maps#1130 Add android only note to showsIndoorLevelPicker Conflicts: lib/android/googlemap/src/main/java/com/airbnb/android/react/maps/googlemap/AirGoogleMapView.java lib/android/src/main/AndroidManifest.xml
This is to revert part of PR#694 to fix the memory leak problem on android.
It also has the patch from IjzerenHein@14ca6f7 to avoid the crash on pause -- which is mentioned in #414