ikarus23 / MifareClassicTool

An Android NFC app for reading, writing, analyzing, etc. MIFARE Classic RFID tags.
http://www.icaria.de/mct/
GNU General Public License v3.0
4.69k stars 906 forks source link

Activity memory leak caused by anonymous threads #393

Open cuixiaoyiyi opened 2 years ago

cuixiaoyiyi commented 2 years ago

Possible Memory Leak

An anonymous inner class will hold a reference to the this pointer of the outer class and will not be released until the thread ends. It will hold the Activity and prevent its timely release. Please check the links below.

Occurrences

https://github.com/ikarus23/MifareClassicTool/blob/master/Mifare%20Classic%20Tool/app/src/main/java/de/syss/MifareClassicTool/Activities/WriteTag.java#L1253 https://github.com/ikarus23/MifareClassicTool/blob/master/Mifare%20Classic%20Tool/app/src/main/java/de/syss/MifareClassicTool/Activities/KeyMapCreator.java#L458

Possible Solution

If it is necessary, it can be changed to static class + weak reference to eliminate the reference to the activity, which may cause memory leaks. Further discussion is welcome.

ikarus23 commented 2 years ago

Thanks. Not sure if this is an issue here. I want to keep the Activity while the thread/runner is working. I will try to have a closer look. Hints/tips are welcome. ;)

ikarus23 commented 1 year ago

Any more hints? If I'm not mistaken, the behavior is by design. The anonymous class should be able to control the outer class (Activity). If the inner class was successful it finishes the outer class as well. If the inner class was unsuccessful it just ends his self (and with that it reference to the outer class?!).

I'm still not sure if this is an issue here. But I'm no Java programming professional, just a hobbyist.

cuixiaoyiyi commented 1 year ago

The anonymous class should be able to control the outer class (Activity). This is the exception case for anonymous threads, active (unfinished) thread objects are included in the GC Root, it will not be released until it finishes executing, and the objects it holds during this period cannot be released in time. The life cycle of Activity and anonymous thread in the link above cannot be guaranteed to be consistent. Activity.onDestroy() --> GC can/should recycle the activity The activity referenced by the thread --> GC can not recycle the activity (There is maybe a memory leak).

If the activity has been released by the manager in Android platform, a (WindowManager.BadTokenException) may happen after the Toast.makeText invocation.

cuixiaoyiyi commented 1 year ago
Solution: 
static ThreadA extends thread{  
     WeakReference<Activity>  w = null;
     // or 
     Context c = null; // getApplicationContext()  with long lifecyle
     void  run(){
             try{
                    for/while(...){
                          ....
                          if(isInterrupted()){
                              break;
                          }
                    }
                    Activity a = w.get();
                    if(a != null ){
                        // use a
                    }
                    //   or  
                    Toast.makeText(c,...);  // ApplicationContext is enough; Activity is unnecessary.
               }catch(InterruptedException e){
                     ....
               }  
       }
}
class Activity:
void onDestroy(){
     ....
     if(thread != null && thread.isAlive()){
          thread .interrupt();
     }
}