Detect fragment view leaks #1061

igokoro · 2018-07-23T17:02:55Z

Extension of the fragment memory leak detection to also keep an eye on fragment's view after onDestroyView(). This is important for cases when a fragment is replaced and added to the backstack - it does not possess a view but is also not destroyed.

If a fragment's view includes any memory-heavy resources, like Bitmaps, a memory leak may become rather severe after a couple of such fragments are added to the backstack. In such cases, it's important to allow GC to do its job and release memory from all "destroyed" views so that new fragments can be displayed.

This PR automates the setup of a fragment's view leak detection.

…agment's view after onDestroyView().

pyricau · 2018-07-24T12:23:49Z

Thanks!

Please read the contributing guidelines and sign the CLA.

igokoro · 2018-07-24T15:54:11Z

@pyricau, already did before submitting the PR.

pyricau · 2018-07-25T16:41:57Z

@igokoro did you fill in this form? https://spreadsheets.google.com/spreadsheet/viewform?formkey=dDViT2xzUHAwRkI3X3k5Z0lQM091OGc6MQ&ndplr=1

I'm not finding your GitHub username (@igokoro) in the sheet.

igokoro · 2018-07-25T17:55:45Z

@pyricau, yes that form - it's also referenced from the contributing guidelines page. I think I used "igokoro" as GitHub username. But anyway - just filled the form again, this time with "@igokoro".

pyricau · 2018-07-25T18:32:41Z

Sorry about that, I just found you twice... I guess I didn't search right. Thanks!

pyricau · 2018-07-26T17:26:17Z

...ry-support-fragment/src/androidTest/java/com/squareup/leakcanary/tests/FragmentLeakTest.java

-  public ActivityTestRule<TestActivity> activityRule = new ActivityTestRule<>(
-      TestActivity.class);
+  public ActivityTestRule<FragmentActivity> activityRule =
+      new ActivityTestRule<>(FragmentActivity.class, true, false);


Why did this need to change, and does that relate to adding activityRule.launchActivity in the test below?

That's correct - it does. Instead of introducing a new Test class - I've updated the existing one to serve the needs of the new test case. With such setup, each test case in this class can start its own activity. This can be reverted if you prefer a separate test class for the new test.

pyricau · 2018-07-26T17:27:48Z

...ry-support-fragment/src/androidTest/java/com/squareup/leakcanary/tests/FragmentLeakTest.java

+    activityRule.launchActivity(new Intent(InstrumentationRegistry.getTargetContext(), NastyActivity.class));
+
+    Espresso.onView(ViewMatchers.withId(R.id.add)).perform(click());
+    Espresso.onView(ViewMatchers.withId(R.id.add)).perform(click());


Why is the test adding the fragment twice?

This is a specific setup for fragment's view leak scenario. Imagine the situation: social network app, from your profile page you open your friend's page, then from his page - his friend's friend and so on - virtually infinite stack.
Here, if replace + addToBackStack fragment transaction is executed - the replaced fragment will not be "destroyed", only onDestroyView() will be executed and only the view should be gone from memory.
So, adding the same fragment twice is just the simplest way to reproduce the problem. Technically, we can add any other fragment on top, not necessarily the same - but that is one more class to create for no particular reason...

could you please explain why now after version 1.6.2 I got leak on replace + addToBackStack operations, and how to handle it?

pyricau · 2018-07-26T17:28:30Z

...anary-support-fragment/src/androidTest/java/com/squareup/leakcanary/tests/NastyActivity.java

+
+import com.squareup.leakcanary.support.fragment.R;
+
+public class NastyActivity extends FragmentActivity {


Couldn't we add the add view to TestActivity instead?

Yes, that was my first idea. But then I decided to keep separate test scenarios separated. I do not necessarily understand all the details about the original test case, so didn't want to break it.
Besides, such changes are easier to understand in isolation. Combined together in a single activity, the new test case would need to ignore the static fragment leak from the original test.

pyricau · 2018-07-26T17:32:44Z

...anary-support-fragment/src/androidTest/java/com/squareup/leakcanary/tests/NastyFragment.java

+
+    @Override
+    public void onViewCreated(View view, @Nullable Bundle savedInstanceState) {
+        label = view.findViewById(R.id.label);


That's interesting, I feel like this must be a common mistake. Do you know of Google documentation recommending to not do that or to clear view references in onDestroyView?

Is this something that one should do on all fragments, or just for some special cases?

I do agree - it has to be a common mistake. That's why I decided to submit this PR :)
Honestly, I'm pretty sure I was doing things like this for years. I'm not aware of any documentation that highlights the issues - I didn't check recently though.

I know that such memory leak is possible when replace + addToBackStack fragment transaction is executed. So, given that replace is add + remove - the same issues can happen for any removed fragments that are added to back stack.

This also occurs when you use detach regardless of the backstack.

pyricau · 2018-07-26T17:33:01Z

leakcanary-support-fragment/src/main/res/layout/fragment_nasty.xml

+    android:layout_width="match_parent"
+    android:layout_height="match_parent"
+    android:gravity="center"
+    android:text="I'm real nasty">


Nasty fragment is nasty.

pyricau · 2018-07-26T17:33:58Z

(not necessarily requesting changes, just trying to get a better understanding)

igokoro · 2018-07-27T13:13:48Z

@pyricau, happy to answer your questions and update the code as needed.

igokoro · 2018-08-31T12:00:32Z

@pyricau, any updates? Is there anything that is holding up this PR?

SUPERCILEX · 2018-09-08T23:40:38Z

@pyricau this would be really cool as I've run into similar memory leaks.

pyricau · 2018-10-16T01:34:28Z

Note: CLA signed. I'd like to follow up with a few changes, can you do this? => https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ It should enable me to add commits on top of the PR.

- Factoring test utility code out - Adding latches to avoid flakes - Removing "nasty", using descriptive names

Extension of fragment memory leak detection to also keep an eye on fr…

b625d78

…agment's view after onDestroyView().

pyricau requested changes Jul 26, 2018

View reviewed changes

pyricau approved these changes Oct 16, 2018

View reviewed changes

PR follow up

35ad391

- Factoring test utility code out - Adding latches to avoid flakes - Removing "nasty", using descriptive names

pyricau force-pushed the feature/detect_fragment_view_leaks branch from 3ae7960 to 35ad391 Compare October 16, 2018 23:19

pyricau merged commit 7d7d2d0 into square:master Oct 16, 2018

pyricau added this to the Next release milestone Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect fragment view leaks #1061

Detect fragment view leaks #1061

igokoro commented Jul 23, 2018

pyricau commented Jul 24, 2018

igokoro commented Jul 24, 2018

pyricau commented Jul 25, 2018

igokoro commented Jul 25, 2018

pyricau commented Jul 25, 2018

pyricau Jul 26, 2018

igokoro Jul 27, 2018

pyricau Jul 26, 2018

igokoro Jul 27, 2018

insearching Jan 18, 2019

pyricau Jul 26, 2018

igokoro Jul 27, 2018

pyricau Jul 26, 2018

igokoro Jul 27, 2018

SUPERCILEX Sep 8, 2018

pyricau Jul 26, 2018

pyricau commented Jul 26, 2018

igokoro commented Jul 27, 2018

igokoro commented Aug 31, 2018

SUPERCILEX commented Sep 8, 2018

pyricau commented Oct 16, 2018


		import com.squareup.leakcanary.support.fragment.R;

		public class NastyActivity extends FragmentActivity {

Detect fragment view leaks #1061

Detect fragment view leaks #1061

Conversation

igokoro commented Jul 23, 2018

pyricau commented Jul 24, 2018

igokoro commented Jul 24, 2018

pyricau commented Jul 25, 2018

igokoro commented Jul 25, 2018

pyricau commented Jul 25, 2018

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

pyricau commented Jul 26, 2018

igokoro commented Jul 27, 2018

igokoro commented Aug 31, 2018

SUPERCILEX commented Sep 8, 2018

pyricau commented Oct 16, 2018