nextcloud / android

📱 Nextcloud Android app
https://play.google.com/store/apps/details?id=com.nextcloud.client
GNU General Public License v2.0
4.25k stars 1.76k forks source link

Static Context #2042

Open tobiasKaminsky opened 6 years ago

tobiasKaminsky commented 6 years ago

https://github.com/nextcloud/android/blob/f04f500669b1ea3c9fc85eb022b10f4345ae464e/src/main/java/com/owncloud/android/MainApp.java#L101-L101

Do not place Android context classes in static fields; this is a memory leak (and also breaks Instant Run) less... A static field will leak contexts. Non-static inner classes have an implicit reference to their outer class. If that outer class is for example a Fragment or Activity, then this reference means that the long-running handler/loader/task will hold a reference to the activity which prevents it from getting garbage collected. Similarly, direct field references to activities and fragments from these longer running instances can cause leaks. ViewModel classes should never point to Views or non-application Contexts.

Maybe we can avoid this somehow?

przybylski commented 6 years ago

So many things depends on getting application context that it might be very hard to release this dependency. One simplification might be to move some parts of logic to ViewModel and bind it with Activity with LiveData.

tobiasKaminsky commented 6 years ago

getApplicationContext() from activity will still be there, but not the hard bound context in MainApp. I am unsure if this is enough...

przybylski commented 6 years ago

From Activity yes, but a lot of utils and helpers are using MainApp context. It is reasonable to move that away from them, but nothing that could be done all at once.

przybylski commented 6 years ago

I had a deeper look now and the whole static context is so deep into the system that untangling this will be a bit problematic. A lot of static fields in other parts of the code depends on this so hard that those parts become unusable (eg. ProviderMeta)

tobiasKaminsky commented 6 years ago

If we really want to do this, then we would have to initiate those (e.g. ProviderMeta) with a context reference. But then we cannot statically access it...