Ghost Object Causing Crash in Java

A Bad Code Style

Java’s GC has made our life easier, but it also makes things more implicit and sometimes easier to make mistakes. Recently I encountered a bad code style which is easy to introduce bugs.

Imagining you have a class with a reference field and a method. The method creates a new object and assign it to the reference. Every time you call the method, a new object is created and assigned to the reference. The code is essentially like below,


class A {
    SomeClass B;
    void initB() {
         B = new SomeClass();
         …… do something with B … …
    }
}

Just call initB many times, this pattern creates a lot of objects without referencing to them. Since we’re not referencing to them, GC will do us the favor of garbage collecting them. However, things can easily go wrong.

When we do something with B after creation of the object, the object can be referred at somewhere else (e.g.: B is a task, and do something adds it to a system task queue). Now we call initB many times, we’ll end up with lots of objects somewhere and only one referred by B. We may not have control over the non-referenced objects any more. This results in garbage.

Things can go worse. If SomeClass defines some callback method, which is triggered later by some signal. We may ends up having callback method from many instances of SomeClass.

An Example in Android

Below is an example in Android.


package com.example.androidtest;

import android.os.Bundle;
import android.os.Handler;
import android.app.Activity;
import android.util.Log;

public class MainActivity extends Activity {

  private static final String TAG = "MainActivity";
  
  private Handler handler;
  private Runnable runnable;
  private Integer value;
  
  @Override
  protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);
    value = 10;
    startRunnable();
    startRunnable();
    removeRunnable();
  }
  
  private void startRunnable() {
    handler = new Handler();
    runnable = new Runnable() {
      @Override
      public void run() {
        Log.d(TAG, value.toString());
      }
    }; 
    handler.postDelayed(runnable, 2000);
  }
  
  private void removeRunnable() {
    handler.removeCallbacks(runnable);
    value = null;
  }
  
}

 

The startRunnable method creates a new Handler and Runnable objects, assigns them to the class reference field. We called this method twice, leaving a Handler and Runnable unreferenced by us.

However, these objects are still referenced by some Android system components. The postDelayed method essentially adds the runnable to a message queue associated with the current thread (The actual details are more complicated than this but you get the idea).

We then call removeRunnable to remove the runnable associated with handler, and assign null to value.

However, this doesn’t remove the ghost runnable associated with the ghost handler. And the run() method of the ghost runnable will be triggered, which will cause a crash when it tries to access value.toString().

The Fix

This coding style is bad and easy to introduce bugs. We essentially only want a single instance of Handler and Runnable at any time. If we have to create a new instance, we should ensure the old one is discarded properly, and leaving them referenced by system components is not.

If the system provides some callback functions that are guaranteed to be called once and we can create the objects inside those functions.

If we need to create objects inside some random methods, we can either reuse the old object, or properly reset the old object before creating new one. The code below shows how to do that to fix the example above.


package com.example.androidtest;

import android.app.Activity;
import android.os.Bundle;
import android.os.Handler;
import android.util.Log;

public class MainActivityNew extends Activity {

  private static final String TAG = "MainActivityNew";
  
  private Handler handler;
  private Runnable runnable;
  private Integer value;
  
  @Override
  protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_main);
    value = 10;
    startRunnable();
    startRunnable();
    removeRunnable();
  }
  
  private void startRunnable() {
    if (handler == null) {
      handler = new Handler();
    } else {
      handler.removeCallbacks(runnable);
    }
    runnable = new Runnable() {
      @Override
      public void run() {
        Log.d(TAG, value.toString());
      }
    }; 
    handler.postDelayed(runnable, 2000);
  }
  
  private void removeRunnable() {
    handler.removeCallbacks(runnable);
    value = null;
  }
  
}

 

In startRunnable method, if a Handler instance is available, we’ll just reuse it; If there’re potentially old Runnable associated with handler, we’ll remove them.