NPE in run() method of GifImageView

[ianjoneill]

Just had a crash in our app due to an NPE in the run() method of GifImageView.

Stack trace:

java.lang.NullPointerException: Attempt to invoke virtual method 'int com.felipecsl.gifimageview.library.GifDecoder.getFrameCount()' on a null object reference
    at com.felipecsl.gifimageview.library.GifImageView.void run()(GifImageView.java:130)
    at java.lang.Thread.run(Thread.java:818)

Having a look at the code, I think it's possible that this is a threading problem. I think it's possible to NPE if the following scenario occurs:

1. startAnimation() is called, so run() is called in a new thread.
2. run() finds that shouldClear is false.
3. clear() is called on the main thread, so the cleanupRunnable is run almost immediately, setting gifDecoder to null.
4. The animation thread calls gifDecoder.getFrameCount().

This seems to be a similar issue to #5.

[ianjoneill]

I don't think it will fix the problem, but making the animating and shouldClear fields of the GifImageView class volatile should reduce the chances of NPE's happening, see the java tutorial on Atomic Access.

[jaredsburrows]

@ianjoneill How did you get around this? @felipecsl Any update on this?

[ianjoneill]

I never came up with a reliable test-case (I've only ever seen the crash once), but as I mentioned in my previous comment, I think marking the animating and shouldClear fields as volatile should help.

[jaredsburrows]

@ianjoneill Yes but after applying your fix, have you received this exception again?

[ianjoneill]

Unfortunately I never got around to trying it out.

[mattinger]

I've gotten around this by cleaning up directly in the thread where the run method is executing. There's not a good reason that I can see for the cleanup to happen in the main UI thread. I was able to easily produce this issue, by using gifImageView in a ViewPager, and cycling rapidly forward and backward.

The problem is that since the cleanup happens in another thread, the initial condition:

if (!animating && !renderFrame) {
        break;
      }

does not actually trigger the break, but then the cleanup runs in another thread, and sets gifDecoder to null.

I also remove the "tmpBitmap" variable as well, and replaced it with an inline anonymous runnable, as it removes another race condition:

Bitmap bitmap = gifDecoder.getNextFrame();
        if (frameCallback != null) {
          bitmap = frameCallback.onFrameAvailable(bitmap);
        }
        final Bitmap tmpBitmap = bitmap;
        frameDecodeTime = (System.nanoTime() - before) / 1000000;
        handler.post(new Runnable() {
          @Override
          public void run() {
              if (tmpBitmap != null && !tmpBitmap.isRecycled()) {
                  setImageBitmap(tmpBitmap);
              }
          }
        });