New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when activity is destroyed #803

Closed
michaelrhughes opened this Issue Dec 9, 2015 · 43 comments

Comments

Projects
None yet
@michaelrhughes
Copy link

michaelrhughes commented Dec 9, 2015

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

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Dec 9, 2015

Thanks very much for the detailed report!

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

@michaelrhughes

This comment has been minimized.

Copy link
Author

michaelrhughes commented Dec 9, 2015

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

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Dec 9, 2015

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

This comment has been minimized.

Copy link
Author

michaelrhughes commented Dec 9, 2015

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

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Dec 9, 2015

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

This comment has been minimized.

Copy link
Author

michaelrhughes commented Dec 9, 2015

Thanks!

@Guang1234567

This comment has been minimized.

Copy link

Guang1234567 commented Dec 10, 2015

@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

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Dec 10, 2015

@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

This comment has been minimized.

Copy link

Guang1234567 commented Dec 11, 2015

@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

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Dec 11, 2015

@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

This comment has been minimized.

Copy link
Member

sjudd commented Dec 12, 2015

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.

@TWiStErRob TWiStErRob added the wontfix label Dec 14, 2015

@Guang1234567

This comment has been minimized.

Copy link

Guang1234567 commented Dec 17, 2015

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

@TWiStErRob

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Dec 17, 2015

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

@renaudcerrato

This comment has been minimized.

Copy link

renaudcerrato commented Aug 26, 2016

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

image

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

@TWiStErRob

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Aug 26, 2016

@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

This comment has been minimized.

Copy link

renaudcerrato commented Aug 27, 2016

@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

This comment has been minimized.

Copy link
Contributor

ygnessin commented Jan 5, 2017

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

This comment has been minimized.

Copy link

nadsikan commented Feb 27, 2017

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

This comment has been minimized.

Copy link

nadsikan commented Mar 1, 2017

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

This comment has been minimized.

Copy link
Collaborator

TWiStErRob commented Mar 1, 2017

@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

This comment has been minimized.

Copy link

ameyab10 commented Mar 22, 2017

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

This comment has been minimized.

Copy link
Author

michaelrhughes commented Mar 22, 2017

@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.

@ameyab10

This comment has been minimized.

Copy link

ameyab10 commented Mar 27, 2017

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

This comment has been minimized.

Copy link

RagingRiver commented Mar 31, 2017

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

This comment has been minimized.

Copy link

HyowonHwang commented Jun 16, 2017

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-pro

This comment has been minimized.

Copy link

hooman-pro commented Aug 16, 2017

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

This comment has been minimized.

Copy link

AllanWang commented Aug 21, 2017

@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

This comment has been minimized.

Copy link

rajileshthayath commented Sep 8, 2017

@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

This comment has been minimized.

Copy link

tutanck commented Oct 23, 2017

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

@AllanWang

This comment has been minimized.

Copy link

AllanWang commented Oct 23, 2017

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

@tutanck

This comment has been minimized.

Copy link

tutanck commented Oct 23, 2017

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

@AllanWang

This comment has been minimized.

Copy link

AllanWang commented Oct 23, 2017

@tutanck sorry, it should be "initialize it"

@prsolucoes

This comment has been minimized.

Copy link

prsolucoes commented Nov 9, 2017

Hi,

It was solved in 4.3.1 version?

Thanks.

@AllanWang

This comment has been minimized.

Copy link

AllanWang commented Nov 9, 2017

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

This comment has been minimized.

Copy link

blennerSilva commented Nov 9, 2017

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

@AllanWang

This comment has been minimized.

Copy link

AllanWang commented Nov 9, 2017

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

This comment has been minimized.

Copy link

jt-gilkeson commented Nov 21, 2017

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

This comment has been minimized.

Copy link

tangyiwu commented Jan 30, 2018

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

This comment has been minimized.

Copy link

AllanWang commented Jan 30, 2018

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

This comment has been minimized.

Copy link

farhan commented Feb 14, 2018

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

This comment has been minimized.

Copy link

garywzh commented Apr 4, 2018

definitely should not crash it, a log is enough

@toby78

This comment has been minimized.

Copy link

toby78 commented Jan 11, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment