Crash when activity is destroyed

[michaelrhughes]

TLDR; Glide should just log and cancel the load instead of crashing when the activity is destoryed since there is no point in loading anything. The user of the library shouldn't have to explicitly handle this situation.

---

Glide Version/Integration library (if any): 'com.github.bumptech.glide:glide:3.6.1' Device/Android Version: Any/Android 5.0+ Issue details/Repro steps/Use case background:

This issue is related to issue #138. When making a call on glide if the activity has been destroyed it will throw an IllegalArgumentException. The following code in the RequestManagerRetriever.java class is responsible:

@TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1)
private static void assertNotDestroyed(Activity activity) {
    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1 && activity.isDestroyed()) {
        throw new IllegalArgumentException("You cannot start a load for a destroyed activity");
    }
}

This is causing a crash in my crashlytics console for a small percentage of my users. There is likely some kind of timing issue where the user hits home as they are scrolling a big list and the activity is cleaned up which results in this crash. I could fix this issue by wrapping glide and running this code myself and simply not calling glide in this situation. However I believe this is the wrong approach, the library shouldn't be crashing in this situation and instead should log and do nothing.

If the activity is destroyed then the view that is being displayed which the image is being displayed is not visible and as a result no exception should be thrown. Instead glide should log and simply not show the image. Users of the library should not have to explicitly handle situations like these, it makes more sense to just handle it in the library.

Glide load line:

Glide.with(context).load(url).bitmapTransform(new GrayscaleTransformation(getContext())).into(imageView);

Layout XML: not relevant

Stack trace / LogCat:

Fatal Exception: java.lang.IllegalArgumentException: You cannot start a load for a destroyed activity
       at com.bumptech.glide.manager.RequestManagerRetriever.assertNotDestroyed(RequestManagerRetriever.java:134)
       at com.bumptech.glide.manager.RequestManagerRetriever.get(RequestManagerRetriever.java:102)
       at com.bumptech.glide.manager.RequestManagerRetriever.get(RequestManagerRetriever.java:87)
       at com.bumptech.glide.Glide.with(Glide.java:620)
       at com.myapp.myapp$24.run(MyAppClass.java:977)
       at android.os.Handler.handleCallback(Handler.java:739)
       at android.os.Handler.dispatchMessage(Handler.java:95)
       at android.os.Looper.loop(Looper.java:135)
       at android.app.ActivityThread.main(ActivityThread.java:5431)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:914)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:707)

[TWiStErRob]

Thanks very much for the detailed report!

What does com.myapp.myapp$24 do? What triggers it to be scheduled for later execution?

[michaelrhughes]

It sounds like what you're getting at is that I can fix the issue on my end instead of making a change in the library. However I do not believe this is the best solution since people use a simplified library like Glide because they don't have to worry about corner cases like this. In my opinion Glide should handle this directly. Obviously I'm no expert on Glide's code so I could be off base here.

To answer your question a network call comes in which then posts to run the code on the UI thread. Since I am in a view there I can do:

onAttachedToWindow
    isAttached = true;

onDetachedFromWindow
    isAttached = false;

// in my method i can call
if (isAttached) {
    //Glide call
}

What do you guys think?

[TWiStErRob]

You're right. I kind of agree with you, it would be simpler if Glide ignored this, but at the same time I don't, here's why. I think in general it's just good to know what's going on in your code. Glide ignoring this would hide a potential bug in your code, that you would consider a feature, but at the same time for someone else it may be a good thing that Glide brings it up. The problem is that there isn't really a good mid-way between log and a crash. Most people ignore logging, especially because Glide doesn't log much unless you turn it on explciitly. On the other hand if your app crashes, you're forced to find out why and fix the bug. Even if you could get away with not doing so.

I think in general if you're going from back->UI thread it's a good thing to check if the UI is still alive. Why would you spend any time updating something that doesn't exist, or not visible and never will be visible. Especially taking away from the precious render time and battery life. In this simple case you may not be doing much, but it's really easy to think of similar use cases which are resource heavy. So regarding your implementation idea, I think you can just ignore the whole Runnable ($24) if your activity.isFinishing(); or even better (together with isFinishing) try to cancel the network request if the activity dies.

By the way, to give a little background why that check is in place: Glide attaches a fragment to the activity/fragment if you're using the corresponding with(...). This fragment is used to subscribe to lifecycle events so requests can be automatically paused/resumed and cancelled. See http://stackoverflow.com/a/32887693/253468. Oh, and for this fragment to be added, as a normal one, the activity has to be in good condition. Follow the Glide.with call chain in code to see more.

[michaelrhughes]

Thanks for the detailed reply and spending time to look into this, I really appreciate it. It's amazing when devs support their own libraries with this level of support.

Glide is really powerful because, like you are saying, it can respect the lifecycle of activities and fragment. An activity being destroyed is a lifecycle event. Therefore I would argue it should respect and properly handle when your activity is destroyed.

I get where you are coming from but if Glide were to silently fail here the only additional overhead (in my app) would be a single post request which is negligible. This is a very small corner case in my code (13 crashes in 15000 sessions), I wouldn't have found it without crashlytics. Realistically adding ~6 lines of code (the activity destroyed check) isn't a big deal to work around it but I felt that Glide would be better as a library if it handled this situation.

[TWiStErRob]

It's amazing when devs support their own libraries with this level of support.

I'm not even a full dev on Glide, I'm just having fun here on Glide issues having my daily dose of brain training. Sam's is the sole owner and main maintainer of Glide. I commit only minor things sometimes.

Considering what I just said, let's see what @sjudd's opinion is on ignoring a load on a destroyed activity with a warn log.

[michaelrhughes]

Thanks!

[Guang1234567]

@michaelrhughes @TWiStErRob

Maybe you can solve this problem instead of using "activity.isFinishing()".

//In Activity
 RequestManager mRequestManager
  public void onCreate(){
    RequestManager mRequestManager= Glide.with(this);
     ....

   Runnable r = new Runnable(){
        mRequestManager.load(glideUrl).into(imageview).
   }

 //TODO:  post Runnable after activity is destoryed.
 }

//In Adapter
RecyclerAdapter(Context context, List<MediaStoreData> data, RequestManager requestManager) {
    this.data = data;
    mRequestManager= requestManager
}

[TWiStErRob]

@LiHanGuang hmm, interesting idea. I'm not sure how it helps though. I mean I don't know whether this will:

1. ignore any loads,
2. go through with them unnecessarily,
3. or just delay the crash for a later time.

These last two (2-3) sound bad, and my guess is that Glide will do 2. when called after destroy.

[Guang1234567]

@TWiStErRob "Crash when activity is destroyed" means @michaelrhughes call Glide.with(this) after "activity is destoryed", so you can call Glide.with(this) at the certain moment that activity is not destoryed. In my solution above, "the certain moment" is onCreate()

Q1: ignore any loads. Answer: Donot worry about that. Because i cache the RequestManager instance in onCreate(), then use this instance all the time . You can see the glide demo("com.bumptech.glide.samples.gallery.RecyclerAdapter") .

See RequestManagerFragment.onStart/onStop which calls ActivityFragmentLifecycle which propagates to RequestManager.onStart/onStop.

Q2: go through with them unnecessarily Answer: "go through with them" means you donnot need to lazy get the instance of RequestManager that attached at the current Context( Activity). So i think "go through with them" in some case is necessarily. For example: in a adapter ("com.bumptech.glide.samples.gallery.RecyclerAdapter")

//In Adapter
RecyclerAdapter(Context context, List<MediaStoreData> data, RequestManager requestManager) {
    this.data = data;
    mRequestManager= requestManager
}

@Override
    public void onBindViewHolder(final RecyclerView.ViewHolder holder, final int position) {
      mRequestManager.load(glideurl).into(imageview);
}

Q3: or just delay the crash for a later time. Answer: no delay crash, because see Q1 answer

[TWiStErRob]

@LiHanGuang so it won't do any of what I said, simply because the RequestManager is paused when activity is destroyed, that's a good one :) It also eagerly initializes the magic fragment which prevents this and possible other weirdnesses.

I wonder if @sjudd did this in the samples knowingly. My guess would be that it just made sense to cache it and some APIs need to get it passed: for example the adapter you quoted benefits because it can be used in Activity/Fragment/or even with AppContext by removing the Glide.with dependency from inside adapter.bind and just passing the actual request manager.

@michaelrhughes did you try this?

[sjudd]

Passing the RequestManager in rather than constantly calling Glide.with does a number of things:

1. It makes the code testable (you can mock RequestManager)
2. It allows you to start loads with the correct context for Views/Adapters created in Fragments (otherwise you end up with the View Context which points to the Activity, not the Fragment)
3. It avoids the (relatively minimal) overhead of the with call itself

It's not intended to avoid this particular assertion, though it can be useful in limited cases where the assertion can't be avoided due to bugs in how fragments are handled in the support libraries (typically affects deeply nested Fragment hierarchies and/or fragment pagers).

The assertion exists because, aside from the fragment issues I mentioned, it never makes sense to try to start an image load while an Activity is being destroyed. It often indicates that you're doing something else dangerous, like starting an image load in an AsyncTask callback for a task that may finish after onDestroy is called and that isn't cancelled.

More practically, because Glide.with() returns a RequestManager the caller then uses, we'd have to create a special mock or pass along some state that indicates that the load should actually be ignored all the down to the into() call, which would be hard to maintain.

[Guang1234567]

Can using "RequestManager" to solve @michaelrhughes 's problem?

[TWiStErRob]

@LiHanGuang Yes it was a good idea, though he didn't respond yet.

[renaudcerrato]

I'm getting a few crashes about that also, but the weird thing is that it's happening during Fragment::onActivityCreated:

Putting if(!activity.isDestroyed()) checks inside my own onActivityCreated sounds weird, isn't it? :-)

[TWiStErRob]

@renaudcerrato I think you need to review your fragment transactions. That stack looks like the fragment didn't have a chance to start up properly before the activity is destroyed. I think in your case you can fix this simply (other than fixing your transactions) by replacing Glide.with(getContext()) with Glide.with((Fragment)this), which may require some refactoring with your custom ImageView; related suggested reading: http://stackoverflow.com/a/32887693/253468

Tip: I think you wanted to spell it "announce" (= spread the word).

[renaudcerrato]

@TWiStErRob: I'm unable to reproduce it, I'm just getting a few in my crash console. IMO, that happens in some rare cases if the activity is finished early. Anyway, I wasn't expecting that onActivityCreated may be called after finish() =)

[ygnessin]

I am encountering this issue while running my test suite. Thought I'd add my 2 cents, because as @TWiStErRob mentioned above, there is a fine line between swallowing/logging errors and letting them crash. But what about the case where everything works fine in my app, but only crashes in my test suite? I'm assuming there's some kind of test-only race condition that is triggering this.

EDIT: Nevermind, I discovered that this was indeed a bug in my code around activity lifecycle handling. So I guess +1 for Rob's point about crashes getting devs' attention :)

[nadsikan]

I am encountering this issue as well and have no idea how to go around it. I am displaying a listview using a customadapter with images in one activity and a another activity with one image. Both are loaded using glide. When I add another user, an image is assigned to the user. That is when the issue arises

[nadsikan]

E/AndroidRuntime: FATAL EXCEPTION: main
                  Process: com.example.nahdi.taufir, PID: 10648
                  java.lang.IllegalArgumentException: You cannot start a load for a destroyed activity
                      at com.bumptech.glide.manager.RequestManagerRetriever.assertNotDestroyed(RequestManagerRetriever.java:134)
                      at com.bumptech.glide.manager.RequestManagerRetriever.get(RequestManagerRetriever.java:102)
                      at com.bumptech.glide.manager.RequestManagerRetriever.get(RequestManagerRetriever.java:87)
                      at com.bumptech.glide.Glide.with(Glide.java:629)
                      at com.example.nahdi.taufir.CustomAdapterReduced$1.onChildAdded(CustomAdapterReduced.java:84)
                      at com.google.android.gms.internal.zzblz.zza(Unknown Source)
                      at com.google.android.gms.internal.zzbnz.zzYj(Unknown Source)
                      at com.google.android.gms.internal.zzboc$1.run(Unknown Source)
                      at android.os.Handler.handleCallback(Handler.java:739)
                      at android.os.Handler.dispatchMessage(Handler.java:95)
                      at android.os.Looper.loop(Looper.java:148)
                      at android.app.ActivityThread.main(ActivityThread.java:5417)
                      at java.lang.reflect.Method.invoke(Native Method)
                      at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
                      at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)

Please help me! This error is driving me insane. Cant figure out what to do. @TWiStErRob

[TWiStErRob]

@nadsikan there's not enough details and space here to help you here, please try to ask on StackOverflow or open a new issue with all the details and hopefully a fully working reprodicable example app. This issue spans more than just a simple Glide load, very likely somethings is wrong with your lifecycle handling.

[ameyab10]

Crashing for this seems a little drastic, I feel a error level log on logcat could have done the same job and leave it up-to the devs to do their due diligence.

[michaelrhughes]

@ameyab10 It looks like in your onChildAdded method you are making a Glide.with()..load() call. The context passed into the with method is that of an activity that has already been destroyed. You can do what I suggested above:

onAttachedToWindow
    isAttached = true;

onDetachedFromWindow
    isAttached = false;

// in my method i can call
if (isAttached) {
    //Glide call
}

However I still do think this is something that the library would ideally manage since putting this code in every fragment/activity is quite cumbersome.

[Emeritus-DarranKelinske]

Has anyone found a good fix for this issue?

[ameyab10]

So I am actually super constrained in things, as in I am implementing a interface for loading an image from for a library I use and don't get passed the view, anyways for now I am falling back to simply passing Glide the ApplicationContext as opposed to the activity context.

[RagingRiver]

I am entering an Activity and leaving it with the HomeAsUp button in the Toolbar. The first time there is no problem, but the second time if I quickly execute the Glide.with(Context)... during a FAB button click I get the same error. I have looked at what could be causing the issue on my end and came up with nothing. It may be with the use of cache in the Glide operation. Regardless, I have resorted to the @ameyab10 temporary solution of using the ApplicationContext which eliminates the crash.

[HyowonHwang]

I have read all comments for this, but still wonder if there is not good idea for this issue. I heard glide control life cycle by empty fragment and guess that's why this issue happen. Anyone can find good solution for this, please? Sometimes, it is a quite difficult to add activities and fragment's instance to custom view.

[hooman-mirghasemi]

this issue is reported on commented on Dec 9, 2015 but now I have this problem with version 4 of glide!!!! do you have any plane to solve this problem: ""You cannot start a load for a destroyed activity""??

[AllanWang]

@hooman-pro where are you initializing glide? Doing so during onCreate still seems to work fine.

@TWiStErRob Given that there seems to be race conditions if we use Glide with a ViewHolder's context, would it be advisable to create a RequestManager during the Activity's onCreate and pass it to every subsequent ViewHolder? I'd like to keep context references to a minimum, but this seems like the only way to reliably load and unload images without the error.

Relevant adapter item code

[rajileshthayath]

@hooman-pro @override public void destroy(View viewHolder) {

if (mContext != null && !mContext.isFinishing()) { GlideApp.with(viewHolder.getRootView().getContext()).clear(view); } } !mContext.isFinishing() this solved my crash.

[tutanck]

I faced the same issue .. i fixed it like that Glide.with(mContext.getApplicationContext()) //activity.getApplicationContext() it seems to work

[AllanWang]

@tutanck I think that removes the whole life cycle handling. Best initialize it with the activity context on creation or validate beforehand

[tutanck]

@AllanWang What do you mean by "I italien it" ?

[AllanWang]

@tutanck sorry, it should be "initialize it"

[paulocoutinhox]

Hi,

It was solved in 4.3.1 version?

Thanks.

[AllanWang]

From the changelog, no.

This also isn't a bug. It's just the way Glide was designed so that issues like these don't go unnoticed. Creating a requestmanager on start works just fine to avoid this

[blennerSilva]

what do you mean by Creating a requestManager on start.? can you provide some example plz?

[AllanWang]

See above

Just create the requestmanager (Glide.with... or GlideApp.with...) when you know the context must exist and then use it later on

[jt-gilkeson]

Just as a heads up for people who run into this (perhaps on older versions of the library and aren't going to upgrade right away) - some adapters may be saving the layoutInflater (or use some other "optimization") which after rotation can inflate a viewHolder with the wrong context (which despite being incorrect - works for the most part). If you then call something like imageView.getContext() and use it with Glide you can run into this issue.

[tangyiwu]

I think the way is that we pause request before activity destroyed.

@Override
+    public boolean onKeyDown(int keyCode, KeyEvent event) {
+        if (keyCode == KeyEvent.KEYCODE_BACK) {
+            Glide.with(this).pauseRequests();
+        }
+        return super.onKeyDown(keyCode, event);
+    }
+
+    @Override
+    public void finish() {
+        Glide.with(this).pauseRequests();
+        super.finish();
+    }

[AllanWang]

The crashes come from calling Glide.with(this) on an invalid context. Doing what you mentioned above will pause requests that are no longer necessary, but it doesn't solve the crash issue.

[farhan]

I am using following util function before loading any image

final Context  context = getContext();
if (isValidContextForGlide(context) {
      // Load image via Glide lib using context
}

 public static boolean isValidContextForGlide(final Context context) {
        if (context == null) {
            return false;
        }
        if (context instanceof Activity) {
            final Activity activity = (Activity) context;
            if (activity.isDestroyed() || activity.isFinishing()) {
                return false;
            }
        }
        return true;
    }

[garywzh]

definitely should not crash it, a log is enough

[toby78]

Why don't you have a config variable for Glide in the app to tell which behaviour it should have?

e.g.

a) default setting - the simple version for people who want a single code line to load an image without thinking about all other potential issues in more complex situations

b) current behaviour, loading of image is not stopped automatically and 20 additional code lines are required at some place in your own code

[ntpanchal]

It sounds like what you're getting at is that I can fix the issue on my end instead of making a change in the library. However I do not believe this is the best solution since people use a simplified library like Glide because they don't have to worry about corner cases like this. In my opinion Glide should handle this directly. Obviously I'm no expert on Glide's code so I could be off base here.

To answer your question a network call comes in which then posts to run the code on the UI thread. Since I am in a view there I can do:

onAttachedToWindow
    isAttached = true;

onDetachedFromWindow
    isAttached = false;

// in my method i can call
if (isAttached) {
    //Glide call
}

What do you guys think?

Thank you so much. saved my time. :+1:

[ArcherEmiya05]

So until today there is still no proper fix on Glide library side? I also still getting a lot of crash report with this even handling it side by side. The context may not be null but if the activity gets destroyed then boom app will still close. If I were a user of the app that would be unpleasant experience because not all user will comprehend what happened. We do understand the reason of throwing exception and its good, but Android platform is too fragmented where each device behaves differently depending on what modification on OS manufacturer does. At least add configuration like cancelOnDestroy(true) or just log.wtf it because in one app there will be hundreds scenario of needing to load an images and no one would really not want to do try catch on each of those request hahaha that would be a huge boilerplate. Try catch is said to be use for unexpected behavior where we do not have any control over that situation like slow internet connection but also expensive as its redirect the system flow or behavior.

[oakkitten]

how about crashing only in debug builds?

yes, this problem is often an error on the part of the programmer, but as evidenced by the numerous issues this can, if rarely, happen even when your code is correct.

it is not reasonable for programmers to expect that Glide will crash in the case when an activity is destroyed. it is also not reasonable for programmers to be aware of all the corner cases where their code calling Glide is called after the destruction of an activity. in other words, a good programmer can have their good code crash in production because Glide thinks that it is reasonable to crash in expected circumstances.

[dharmishthawallis2020]

I have solved my problem using below method. May it help someone. I m using Glide to load image in Runnable Using Handler.

private boolean isPause = false, isDestroy = false;

 @Override
    public void onPause() {
        super.onPause();
        isPause = true;
        handler.removeCallbacksAndMessages(null);
    }

@Override
    public void onDestroy() {
        super.onDestroy();
        isDestroy = true;
        handler.removeCallbacksAndMessages(null);
    }

@Override
    public void onViewCreated(@NonNull View rootview, @Nullable Bundle savedInstanceState) {
        super.onViewCreated(rootview, savedInstanceState);
        ....
        if (!isPause && !isDestroy) {
                            Glide.with(rootview)
                                    .load("https://dummyimage.com/150x150/000000/ffffff.png&text=This+is+Image")
                                    .fitCenter()
                                    .placeholder(R.mipmap.ic_launcher)
                                    .error(R.mipmap.ic_launcher)
                                    .into(ivAdImage);
                        }

[guidovezzoni]

I'm using this context extension before loading:

/**
 * Returns true when [Context] is unavailable or is about to become unavailable
 */
fun Context?.isDoomed(): Boolean = when (this) {
    null -> true
    is Application -> false
    is Activity -> (this.isDestroyed or this.isFinishing)
    else -> false
}

[sahandnayebaziz]

This crash is affecting my app where...

• The app is mainly Jetpack Compose • The app preloads ~100 small images when it is first launched with the following code:

allImageUrls.forEach {
  Glide.with(activity)
    .load(it)
    .priority(Priority.LOW)
    .diskCacheStrategy(DiskCacheStrategy.ALL)
    .preload()
}

How can I catch this exception so the app doesn't crash? It seems like, even if I switch this to do them one-by-one, it can still crash after Glide.with(activity) evaluates successfully but it is still in the loading / caching stage?