Fix navigation view memory leak - some live data objects from navigat… #2051

Lebedinsky · 2019-09-17T14:10:46Z

Description

Fixes NavigationView memory leak.

Fixes #2012

I have added any issue links
I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
I have added the appropriate milestone and project boards

Goal

Some LiveData objects from NavigationViewModel were never released as part of the view onDestroy() causing a memory leak. The goal here is to avoid that leak.

Implementation

This PR prevents that leak unsubscribing NavigationViewModel LiveData objects previously added by removing the observers of the LifecycleOwner adding unsubscribe methods to NavigationViewSubscriber, InstructionView and SummaryBottomSheet that are called automatically when called onDestroy() method for lifecycle owner of this views. Here Lifecycle owner is NavigationView by adding to it LifecycleRegistry.

Testing

Please describe the manual tests that you ran to verify your changes

I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
Tested in https://github.com/william-reed/mapbox-navui-leak and the leak is not reported anymore
I have tested via a test drive, or a simulation/mock location app
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have updated the CHANGELOG including this PR

…ion view model were never released as part of the view on destroy.

codecov-io · 2019-09-17T14:34:26Z

Codecov Report

Merging #2051 into master will increase coverage by 0.04%.
The diff coverage is 34.69%.

@@             Coverage Diff              @@
##             master    #2051      +/-   ##
============================================
+ Coverage      41.5%   41.54%   +0.04%     
- Complexity     1348     1351       +3     
============================================
  Files           286      286              
  Lines          9127     9156      +29     
  Branches        684      684              
============================================
+ Hits           3788     3804      +16     
- Misses         5020     5033      +13     
  Partials        319      319

andrlee · 2019-09-18T05:18:57Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java


  private NavigationPresenter navigationPresenter;
+  private NavigationViewModel navigationViewModel;


could be declared final

andrlee · 2019-09-18T05:19:21Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java

-  NavigationViewSubscriber(NavigationPresenter navigationPresenter) {
+  NavigationViewSubscriber(final LifecycleOwner owner,
+                           final NavigationViewModel navigationViewModel,
+                           final NavigationPresenter navigationPresenter) {
    this.navigationPresenter = navigationPresenter;


could be declared final

andrlee · 2019-09-18T05:19:55Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java


  private NavigationPresenter navigationPresenter;
+  private NavigationViewModel navigationViewModel;
+  private LifecycleOwner lifecycleOwner;


could be declared final

andrlee · 2019-09-18T05:30:47Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

@@ -65,7 +66,7 @@
 *
 * @since 0.7.0
 */
-public class NavigationView extends CoordinatorLayout implements LifecycleObserver, OnMapReadyCallback,
+public class NavigationView extends CoordinatorLayout implements LifecycleOwner, OnMapReadyCallback,


since every Activity and Fragment implements LifecycleOwner i'm not sure if this is a legit fix, considering that we already expose on* callbacks would it be reasonable to expect users to wire them up to activity or fragment lifecycle callbacks?

That is exactly why I did it. When we implement LifecycleOwner in NavigationView - we remove the obligation from user to pass LifecycleOwner into this view by smth like setLifecycleOwner(). In our documentation we describes how to operate with NavigationView, and there only need to call on* callbacks. All other stuff regarding lifecycle in NavigationView we made internally and we shouldn't ask users to do any other movements. So implementing NavigationView as LifecycleOwner (for it "child" views, like IntructionView, SummaryBottomSheet and so on) allow us to minimize additional user interactions with our NavigationView for it correct work.
I mean NavigationView is smth like "parent" or container for other views (IntructionView, SummaryBottomSheet, etc.), so in this nested views we just subscribe for observing lifecycle of its "parent" (NavigationView).

so afaik view has much simpler lifecycle, looking at the change are we essentially forcing activity/fragment lifecycle onto view? what's the benefit of doing it? should we just simply migrate over to Fragment?
cc: @Guardiola31337

in my perspective this view has grown beyond the scope of what any view should contain. leading to a fragment would be a logical choice. but then again there are people that use views for everything and completely avoid fragments and activities so this seems like a personal choice more than anything

Guardiola31337

This is looking good so far @Lebedinsky Great work!

I left some comments.

Guardiola31337 · 2019-09-19T16:48:39Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

@@ -206,6 +214,12 @@ public void onStop() {
    }
  }

+  @NonNull


Could we add Javadoc for public API?

No need here, it is just overrided method for LifecycleOwner

Guardiola31337 · 2019-09-19T16:56:01Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

@@ -176,18 +181,21 @@ public void onFeedbackDismissed() {
   * @param navigationViewModel to which this View is subscribing
   * @since 0.6.2
   */
-  public void subscribe(NavigationViewModel navigationViewModel) {
+  public void subscribe(LifecycleOwner owner, NavigationViewModel navigationViewModel) {


Let's make sure we update the release notes and the android docs site with these changes (and add the SEMVER label here 😅), as we are breaking subscribe API here 👍

Guardiola31337 · 2019-09-19T16:56:55Z

...i/src/main/java/com/mapbox/services/android/navigation/ui/v5/summary/SummaryBottomSheet.java

@@ -69,8 +76,13 @@ protected void onFinishInflate() {
    bind();
  }

-  public void subscribe(NavigationViewModel navigationViewModel) {
-    navigationViewModel.summaryModel.observe((LifecycleOwner) getContext(), new Observer<SummaryModel>() {
+  public void subscribe(LifecycleOwner owner, NavigationViewModel navigationViewModel) {


Same as before re: SemVer

Guardiola31337

I've left some nit-pickings @Lebedinsky

Guardiola31337 · 2019-09-19T17:37:22Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

@@ -90,6 +91,8 @@
  private boolean isMapInitialized;
  private boolean isSubscribed;

+  private LifecycleRegistry lifecycleRegistry;


NIT No need for new line

Guardiola31337 · 2019-09-19T17:39:28Z

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java

@@ -590,7 +604,7 @@ private void updateInstructionMutedState(boolean isMuted) {
    int paddingBuffer = (int) resources.getDimension(R.dimen.route_overview_buffer_padding);
    int instructionHeight = (int) (resources.getDimension(R.dimen.instruction_layout_height) + paddingBuffer);
    int summaryHeight = (int) resources.getDimension(R.dimen.summary_bottomsheet_height);
-    return new int[] {leftRightPadding, instructionHeight, leftRightPadding, summaryHeight};
+    return new int[]{leftRightPadding, instructionHeight, leftRightPadding, summaryHeight};


NIT 👀 how to set up Mapbox checkstyle here https://github.com/mapbox/mapbox-gl-native/wiki/Setting-up-Mapbox-checkstyle

Guardiola31337 · 2019-09-19T17:40:24Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java


-  NavigationViewSubscriber(NavigationPresenter navigationPresenter) {
+  NavigationViewSubscriber(final LifecycleOwner owner,
+                           final NavigationViewModel navigationViewModel,


NIT no need for new lines, also could we rearrange the properties to match constructor for better readability?

I'm agree with you, Done

Guardiola31337 · 2019-09-19T17:41:54Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

@@ -103,6 +106,8 @@
  private SoundButton soundButton;
  private FeedbackButton feedbackButton;

+  private LifecycleOwner lifecycleOwner;


NIT no need for new line

Guardiola31337 · 2019-09-19T17:42:40Z

...i/src/main/java/com/mapbox/services/android/navigation/ui/v5/summary/SummaryBottomSheet.java

+  public void subscribe(LifecycleOwner owner, NavigationViewModel navigationViewModel) {
+    lifecycleOwner = owner;
+    lifecycleOwner.getLifecycle().addObserver(this);
+


NIT no need for new line

Guardiola31337 · 2019-09-19T17:51:00Z

cc @william-reed for visibility Please feel free to share your feedback and if this approach aligns with what you were thinking. Thanks a lot!

Guardiola31337 · 2019-09-19T21:52:42Z

One thing that we missed here was:

I have updated the CHANGELOG including this PR

Please add the entry to the CHANGELOG @Lebedinsky when you have a change as a separate PR 🙏

Guardiola31337

A couple of comments around the tests.

Guardiola31337 · 2019-09-20T14:44:00Z

...src/test/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriberTest.java

+  private NavigationViewSubscriber theNavigationViewSubscriber;
+
+  @Before
+  public void setup() {


👀 #2016 (comment)

Guardiola31337 · 2019-09-20T14:46:15Z

...src/test/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriberTest.java

+
+    theNavigationViewSubscriber.unsubscribe();
+
+    verify(navigationViewModel.retrieveRoute()).removeObservers(eq(lifecycleOwner));


Generally is preferable having only 1 assertion per test 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=57

The problem of having multiple assertions are:

It's annoying to have to look at the stacktrace to know what's failing (which assertion failed?)

You don't have information about the remaining assertions (because when an assertion fails it breaks the execution of the test)

That's why is preferable to have only 1 assertion per test and one test per scenario. Basically, if one of them fails you'll know at a glance what in the code might have caused that failure and in which part (class, method, etc.).

Fix code style for PR #2051. Add changes to Changelog.

william-reed · 2019-09-23T00:45:54Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewSubscriber.java

+
+  @OnLifecycleEvent(Lifecycle.Event.ON_DESTROY)
+  void unsubscribe() {
+    navigationViewModel.retrieveRoute().removeObservers(lifecycleOwner);


i dont think this is necessary considering the lifecycle is tied to this view anyway

william-reed · 2019-09-23T00:47:08Z

Thanks for doing this, this was exactly how I had envisioned it with a custom LifecycleOwner. Appreciate it

Fix navigation view memory leak - some live data objects from navigat…

b46b5e3

…ion view model were never released as part of the view on destroy.

Lebedinsky added bug Defect to be fixed. navigation-ui labels Sep 17, 2019

Lebedinsky added this to the v0.42.0 milestone Sep 17, 2019

Remove unused import.

8e3a5ff

Lebedinsky requested review from Guardiola31337 and andrlee September 17, 2019 14:16

Lebedinsky marked this pull request as ready for review September 17, 2019 14:17

Lebedinsky assigned Guardiola31337 Sep 17, 2019

andrlee reviewed Sep 18, 2019

View reviewed changes

Make NavigationViewSubscriber fields final

11027bf

Lebedinsky requested a review from andrlee September 18, 2019 10:08

andrlee approved these changes Sep 19, 2019

View reviewed changes

Guardiola31337 reviewed Sep 19, 2019

View reviewed changes

andrlee merged commit 9fde437 into master Sep 19, 2019

Guardiola31337 reviewed Sep 19, 2019

View reviewed changes

Guardiola31337 mentioned this pull request Sep 19, 2019

Fix NavigationView memory leak #2016

Closed

11 tasks

Guardiola31337 added the backwards incompatible Requires a SEMVER major version change. label Sep 19, 2019

Lebedinsky pushed a commit that referenced this pull request Sep 20, 2019

Fix code style for PR #2051. Add changes to Changelog.

d98831a

Lebedinsky mentioned this pull request Sep 20, 2019

Fix code style for PR #2051. Add changes to Changelog. #2055

Merged

9 tasks

Guardiola31337 reviewed Sep 20, 2019

View reviewed changes

Lebedinsky pushed a commit that referenced this pull request Sep 20, 2019

Merge pull request #2055 from mapbox/rl-2012-navigation-view-leak

96f75b8

Fix code style for PR #2051. Add changes to Changelog.

Guardiola31337 mentioned this pull request Sep 20, 2019

Release v0.42.0 #2057

Closed

12 tasks

william-reed reviewed Sep 23, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix navigation view memory leak - some live data objects from navigat… #2051

Fix navigation view memory leak - some live data objects from navigat… #2051

Lebedinsky commented Sep 17, 2019 •

edited

Loading

codecov-io commented Sep 17, 2019

andrlee Sep 18, 2019

andrlee Sep 18, 2019

Lebedinsky Sep 18, 2019

andrlee Sep 18, 2019

andrlee Sep 18, 2019

Lebedinsky Sep 18, 2019

andrlee Sep 18, 2019 •

edited

Loading

william-reed Sep 23, 2019

Guardiola31337 left a comment

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 left a comment

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 Sep 19, 2019

Lebedinsky Sep 20, 2019

Guardiola31337 commented Sep 19, 2019

Guardiola31337 commented Sep 19, 2019

Guardiola31337 left a comment

Guardiola31337 Sep 20, 2019

Guardiola31337 Sep 20, 2019

william-reed Sep 23, 2019

william-reed commented Sep 23, 2019


		private NavigationPresenter navigationPresenter;
		private NavigationViewModel navigationViewModel;

@@ @@ -206,6 +214,12 @@ public void onStop() { @@
                   }
                 }
+                @NonNull


		theNavigationViewSubscriber.unsubscribe();

		verify(navigationViewModel.retrieveRoute()).removeObservers(eq(lifecycleOwner));

Fix navigation view memory leak - some live data objects from navigat… #2051

Fix navigation view memory leak - some live data objects from navigat… #2051

Conversation

Lebedinsky commented Sep 17, 2019 • edited Loading

Description

Goal

Implementation

Testing

Checklist

codecov-io commented Sep 17, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrlee Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 commented Sep 19, 2019

Guardiola31337 commented Sep 19, 2019

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-reed commented Sep 23, 2019

Lebedinsky commented Sep 17, 2019 •

edited

Loading

andrlee Sep 18, 2019 •

edited

Loading