[TIMOB-13392] Android: fix TabGroup fragment memory leaks, fix maps in tabs including when acti... #6008

mokesmokes · 2014-09-02T21:03:46Z

This fixes and improves the initial commit using the ViewPager for the TabGroup.
Issues solved:

Huge memory leak when the tab group activity was destroyed and later recreated (i.e. as in https://jira.appcelerator.org/browse/TIMOB-17016). The issue was that ever since 3.3.0 we have been keeping references to the fragments in our own data structures, and also never detaching and destroying fragment. So if this cycle happens a few times - memory explodes. This was masked by the crash we had until now.... The main fix was to remove ALL fragment references in our code.
The tab group needed to restore data structures coming back (same situation as in 17016), so that we can point to the correct fragments when coming back, and not blindly create new ones (the leak). So for this we had no choice but update TiLifecycle, and add onSaveInstanceState and onRestoreInstanceState. onSaveInstanceState is now used in the tab group.
We needed the restore state as early as possible (i.e. activity creation), so I had to add the restored Bundle to the windowCreated() calls. The tab group must use this, and it's a good thing - hopefully other window modules and can make use of this stuff and be more intelligent in their handling.
These are the key fixes. The rest of the changes are to accommodate these infrastructure changes.

In general, this is very well tested, since it built on the testing done for the previous commit which was already heavy. Specifically tested: removing and adding tabs, using a map in the tabs, disabling and enabling tabs, doing all the previous then opening a new window (destroying the tab group activity), and then coming back to the tab group - all works splendidly and no memory leak.
Please merge ASAP since this fixes serious issues in the previous commit. Thanks!

ingo · 2014-09-02T21:06:17Z

Thank you. Is this part of https://jira.appcelerator.org/browse/TIMOB-13392, or is it part of a new ticket?

mokesmokes · 2014-09-02T21:07:03Z

This is a bunch of fixes for that issue. I'm currently writing up the comments for it, hopefully all will be clear in 5 minutes :)

mokesmokes · 2014-09-02T21:22:51Z

android/modules/android/src/java/ti/modules/titanium/android/TiJSActivity.java

 	{
 		// Set the layout proxy here since it's not ready when we indirectly call it inside contextCreated()
 		setLayoutProxy(window);

 		// The UIWindow needs to be created before we run the script
 		activityWindow = new TiUIActivityWindow((TiActivityWindowProxy)window, this, getLayout());
-		super.windowCreated();
+		super.windowCreated(savedInstanceState);
 	}

 	@Override


We have changes like this in several files to enable activities to save and restore arbitrary data structures. This was absolutely required for the tab group, which needs to store/retrieve the IDs of the fragments in the FragmentManager in order to prevent leaks.

mokesmokes · 2014-09-03T05:41:45Z

See comments in the code for explanation of what was done.

hieupham007 · 2014-09-03T18:44:43Z

android/modules/ui/src/java/ti/modules/titanium/ui/widget/tabgroup/TiUIActionBarTabGroup.java

-			return POSITION_NONE;
+			while (fragmentTags.size() <= position) {
+				fragmentTags.add(null);
+            }


indentation is a bit off here

hieupham007 · 2014-09-04T00:14:39Z

Hey Mark, I'm running your updated test case in the ticket as I'm trying to reproduce the memory leak problem. I have developer option 'dont keep activities' on, so the activities are destroyed every time i switch tabs. I'm not seeing any significant movement of the heap via DDMS after scrolling through all 5 tabs ~20-30 times.

mokesmokes · 2014-09-04T15:59:50Z

@hieupham007 put the following code in android/modules/ui/src/java/ti/modules/titanium/ui/widget/tabgroup/TiUIActionBarTab.java in the TabFragment class, open and close window 2 several times (with don't keep activities on) and watch over time how many fragments are created (in the 3.5.0 master).... This is what happens when we create fragments every time, instead of reusing the fragments that are already in the FragmentManager.
I'm also assuming there is a performance hit as we recreate more and more zombie fragments every time we return to the tabgroup.

@Override
public void onCreate(Bundle savedInstanceState) {
  Log.e(TAG, "fragment onCreate called savedInstanceState == null: " + (savedInstanceState == null));
  super.onCreate(savedInstanceState);
}

mokesmokes · 2014-09-05T20:14:36Z

android/titanium/src/java/org/appcelerator/titanium/view/TiUIFragment.java

-		} else {
-			transaction = manager.beginTransaction();
-		}
+		FragmentTransaction transaction = manager.beginTransaction();


This check was not necessary and in fact was crashing the app on occasion due to https://code.google.com/p/android/issues/detail?id=42601 . The crash would occur because tabFragment.getChildFragmentManager() would sometimes be called prior to tabFragment.onActivityCreated occurring (although getSupportFragmentManager works fine at that point). The reason the 3.5.0 master works is that it does not set the ID "tabcontent", so we never called getChildFragmentManager... However testing shows that we don't need it, and the sample app works perfectly without it. If we ever want getChildFragmentManager to be used, then it can only be called after fragment.onActivityCreated (unless Google fixes their bug). Nevertheless, it's not needed at this point.

mokesmokes · 2014-09-05T20:17:05Z

Modified per comments, documented smoothScrollOnTabClick, fixed a map issue - ready to go.

mokesmokes · 2014-09-10T05:34:34Z

Any update on the review for this? All comments taken care of and no issues found with any test I tried, with latest fixes.

pingwang2011 · 2014-09-10T17:11:31Z

apidoc/Titanium/UI/TabGroup.yml

+    description: |
+        If true, when clicking the tab the page transition is animated, false otherwise.
+    platforms: [android]
+    since: 3.4.0


This should be since 3.5.0 .

mokesmokes · 2014-09-10T18:09:25Z

@pingwang2011 fixed the doc

hieupham007 · 2014-09-15T22:18:22Z

Functionally tested with KitchenSink and test cases in the ticket. Looks good to me.

mokesmokes · 2014-09-29T17:47:45Z

Can this be merged please, if there are no outstanding issues? Thanks.

mokesmokes · 2014-10-14T09:23:43Z

@ingo @hieupham007 @pingwang2011
Guys, this is a large PR that is currently tested and mergeable on 3.5.0. If unmerged soon it will surely break over time and more work will be required to modify and re-test. This PR fixes changes from my previous PR: #5651 as well as adds functionality requested by other developers (smoothScrollOnTabClick). Please merge..... thanks :)

yomybaby · 2014-10-14T11:49:41Z

👍 Great work!

…ctivity restored, etc

…licked

…fixes per review

hieupham007 · 2014-12-03T01:10:10Z

I've created a new PR to resolve conflicts: #6414

mokesmokes reviewed Sep 2, 2014
View reviewed changes

hieupham007 reviewed Sep 3, 2014
View reviewed changes

mokesmokes reviewed Sep 5, 2014
View reviewed changes

mokesmokes mentioned this pull request Sep 10, 2014

Timob 17016 new #6020

Closed

pingwang2011 reviewed Sep 10, 2014
View reviewed changes

mokesmokes mentioned this pull request Sep 30, 2014

[TIMOB-15443] Android: Enable proxies to get window/tabgroup activity lifecycle events #6179

Closed

Mark Mokryn added 4 commits October 28, 2014 11:17

fix TabGroup fragment memory leaks, fix maps in tabs including when a…

403510d

…ctivity restored, etc

add explanations and a sanity check error message that never gets called

d446f4f

change tab immediately without a scrolling transition if the tab is c…

aa4418e

…licked

add TabGroup property smoothScrollOnTabClick, default value true

2e8836f

Mark Mokryn added 3 commits October 28, 2014 11:22

indentation fix

0b26d94

fix map fragment handling, document smoothScrollOnTabClick, cosmetic …

f1925ac

…fixes per review

change doc to 3.5.0, comment typo fix

80604a4

mokesmokes force-pushed the TIMOB-13392 branch from 0a98412 to 80604a4 Compare October 28, 2014 09:31

ingo changed the title ~~fix TabGroup fragment memory leaks, fix maps in tabs including when acti...~~ [TIMOB-13392] Android: fix TabGroup fragment memory leaks, fix maps in tabs including when acti... Nov 18, 2014

hieupham007 mentioned this pull request Dec 3, 2014

[Timob-13392]: Android: fix TabGroup fragment memory leaks, fix maps in tabs... #6414

Merged

hieupham007 closed this Dec 3, 2014

manumaticx mentioned this pull request Dec 17, 2014

MapView V2 MeasureChina/Ti.DrawerLayout#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIMOB-13392] Android: fix TabGroup fragment memory leaks, fix maps in tabs including when acti... #6008

[TIMOB-13392] Android: fix TabGroup fragment memory leaks, fix maps in tabs including when acti... #6008

mokesmokes commented Sep 2, 2014

ingo commented Sep 2, 2014

mokesmokes commented Sep 2, 2014

mokesmokes Sep 2, 2014

mokesmokes commented Sep 3, 2014

hieupham007 Sep 3, 2014

hieupham007 commented Sep 4, 2014

mokesmokes commented Sep 4, 2014

mokesmokes Sep 5, 2014

mokesmokes commented Sep 5, 2014

mokesmokes commented Sep 10, 2014

pingwang2011 Sep 10, 2014

mokesmokes commented Sep 10, 2014

hieupham007 commented Sep 15, 2014

mokesmokes commented Sep 29, 2014

mokesmokes commented Oct 14, 2014

yomybaby commented Oct 14, 2014

hieupham007 commented Dec 3, 2014

[TIMOB-13392] Android: fix TabGroup fragment memory leaks, fix maps in tabs including when acti... #6008

[TIMOB-13392] Android: fix TabGroup fragment memory leaks, fix maps in tabs including when acti... #6008

Conversation

mokesmokes commented Sep 2, 2014

ingo commented Sep 2, 2014

mokesmokes commented Sep 2, 2014

mokesmokes Sep 2, 2014

Choose a reason for hiding this comment

mokesmokes commented Sep 3, 2014

hieupham007 Sep 3, 2014

Choose a reason for hiding this comment

hieupham007 commented Sep 4, 2014

mokesmokes commented Sep 4, 2014

mokesmokes Sep 5, 2014

Choose a reason for hiding this comment

mokesmokes commented Sep 5, 2014

mokesmokes commented Sep 10, 2014

pingwang2011 Sep 10, 2014

Choose a reason for hiding this comment

mokesmokes commented Sep 10, 2014

hieupham007 commented Sep 15, 2014

mokesmokes commented Sep 29, 2014

mokesmokes commented Oct 14, 2014

yomybaby commented Oct 14, 2014

hieupham007 commented Dec 3, 2014