onevcat / Kingfisher

A lightweight, pure-Swift library for downloading and caching images from the web.
MIT License
22.99k stars 2.63k forks source link

app crashs on iOS 14 when use Kingfisher in List #1508

Closed hassfers closed 3 years ago

hassfers commented 3 years ago

Hello

yes I know iOS 14 is still in beta but maybe it helps you ;-)

I use kingfisher in a SwiftUI List to load pictures from urls. The initial state is shown as expected, but when I start scrolling the app crashs with a Bad Access- Error.

If I delete the KFImage everything working well.

If I use the same url for every cell it also works fine.

Maybe there is some problem when SwiftUI tries to reuse the Cell and can't load the image somehow.

Does anybody has an idea what to do ?

onevcat commented 3 years ago

Hi,

Thanks for letting me know this.

But I tried the SwiftUI List demo in the project and it seems working well and no crash happens.

Is it only on real devices or also on simulators? Do you have a sample view or some code snippet that we can use to reproduce it?

Thank you!

hassfers commented 3 years ago

Hello and thanks for the fast reply.

After some more investigations I found out, that it only occurs on device, and only if the KFImage is in a NavigationLink. So it could also a bug with SwiftUI itself. Or Im doing something wrong.

I updated the SwiftUI List demo and got the same Bad Access Error.


import class Kingfisher.ImageCache
import KingfisherSwiftUI
import SwiftUI

struct SwiftUIList : View {

    let index = 1 ..< 700

    var body: some View {
        NavigationView{
            List(index) { i in
                ListCell(index: i)
            }.navigationBarTitle(Text("SwiftUI List"), displayMode: .inline)
        }
    }

    struct ListCell: View {
        let index: Int
        var body: some View {
            NavigationLink(destination: Text("\(index)")) {
                ListCellImage(index: index)
                    .scaledToFit()
                    .frame(maxWidth: 100, maxHeight: 100)
            }
        }
    }

    struct ListCellImage: View {

        @State var done = false

        var alreadyCached: Bool {
            ImageCache.default.isCached(forKey: url.absoluteString)
        }

        let index: Int
        var url: URL {
            URL(string: "https://github.com/onevcat/Flower-Data-Set/raw/master/rose/rose-\(index).jpg")!
        }

        var body: some View {
            HStack(alignment: .center) {
                Spacer()
                KFImage(url, isLoaded: $done)
                    .resizable()
                    .onSuccess { r in
                        print("Success: \(self.index) - \(r.cacheType)")
                    }
                    .onFailure { e in
                        print("Error \(self.index): \(e)")
                    }
                    .onProgress { downloaded, total in
                        print("\(downloaded) / \(total))")
                    }
                    .placeholder {
                        HStack {
                            Image(systemName: "arrow.2.circlepath.circle")
                                .resizable()
                                .frame(width: 50, height: 50)
                                .padding(10)
                            Text("Loading...").font(.title)
                        }
                        .foregroundColor(.gray)
                    }
                    .cancelOnDisappear(true)
                    .aspectRatio(contentMode: .fit)
                    .cornerRadius(20)
                    .frame(width: 300, height: 300)
                    .opacity(done || alreadyCached ? 1.0 : 0.3)
                    .animation(.linear(duration: 0.4))

                Spacer()
            }.padding(.vertical, 12)
        }

    }
}

#if DEBUG
struct SwiftUIList_Previews : PreviewProvider {
    static var previews: some View {
        SwiftUIList()
    }
}
#endif
TeslaCrest commented 3 years ago

Hi ! I'm facing the exact same problem in my app when using Kingfisher in List that are embedded inside NavigationLink. That crash happens in older version of my app (already live on the AppStore) & in the iOS 14 ready version I'm working on. The newer version uses the latest Kingfisher build (5.15.0). I can give you more details if needed.

Also, it's worth to mention that in some cases it's not crashing immediately but only after few rows are loaded.

onevcat commented 3 years ago

@hassfers @RosayGaspard

Thanks for this information.

I tried the sample above and it did give me a crash when building with Xcode 12 and run on iOS 14. But if I build it using Xcode 11, or build with Xcode 12 but run on iOS 13, the code does not give a crash. To me, it seems that this issue only happens on iOS 14.

Am I missing something or do you have the same situation?

@RosayGaspard Thank you for your kindly report.

That crash happens in older version of my app (already live on the AppStore)

Oh, it is already on AppStore, so I guess it is also happening on iOS 13? The crash stack I can get when running on iOS 14 indicates an internal layout error under UIKit. Maybe it is related to something like cell reuse or frame calculation. Can you paste something like a symbol stack for the crashes on both older iOS 13 devices and new iOS 14? I want to track if we are having the same issue on both system versions.

Thank you again!

TeslaCrest commented 3 years ago

@onevcat Sorry I realise my post was a bit confusing. The older version of the app, that is live in the store, works well on iOS 13 devices, it crashes only on iOS14 devices. The problem for me is that iOS14 will certainly be released this week (if not today after the Apple event), so all my users who will update to it will have crashes within the app :(

But I think I might have some more information for you:

Some of my views also embed Kingfisher elements inside a NavigationLink but only have a few rows (10 to 15 to be exact) and in that case, the views don't crash at all.

onevcat commented 3 years ago

That makes sense.

I dig into it a bit deeper, and now I can have a minimal reproducible example as below:

struct ContentView: View {
    let indexes = 1 ..< 100
    var body: some View {
        NavigationView {
            List(indexes) { i in
                NavigationLink(
                    destination: Text("Index \(i)"),
                    label: {
                        HStack { Cell(index: i) }
                    })
            }
        }
    }
}

struct Cell: View {

    @State var loaded = false
    let index: Int

    var body: some View {
        Group {
            if loaded {
                Text("Loaded: \(index)")
            } else {
                Text("Loading: \(index)")
            }
        }.onAppear {
            loaded = true
        }
    }
}

This crash happens when embedding any view that can change in an HStack and around by NavigationLink. Obviously, it is not an issue in Kingfisher (as the example above has nothing to do with Kingfisher or any other complicate logic). It is a regression of SwiftUI shipped on iOS 14 beta.

I will try to see if I can give a workaround for it now. But I guess it is more important to report it through feedback/radar (I will do it later today) and expect Apple has a fix on it in the final release.

TeslaCrest commented 3 years ago

I see, thank you for the quick answer ! I thing this might be a regression in the latest beta, because I never noticed this problem with earlier beta of iOS 14.

Hope they'll fix it before the release.

onevcat commented 3 years ago

An obvious, but not always working solution, is not using an HStack to layout inside the NavigationLink, but move it out, say, instead of this:

List(indexes) { i in
    NavigationLink(
        destination: Text("Index \(i)"),
        label: {
            HStack { 
                Text("Hello ")
                Cell(index: i) 
            }
        })
}

Do this:

List(indexes) { i in
    HStack {  // <-- Move HStack out!
        Text("Hello")
        NavigationLink(
            destination: Text("Index \(i)"),
            label: {
                Cell(index: i)
            })
    }
}

(Of course, remember to replace either Text("Hello") or Cell(index: i) to KFImage!)

Until now, I didn't find a (bad) side effect for this workaround, but maybe sometimes there is no way to implement your layout by this. However, it would be better than a crash. Let's see what Apple can do for it in a few days!

hassfers commented 3 years ago

Many many thanks for your deep investigation and your solutions and I'm very sorry for finger pointing 😇 I had a look and had exakt this structure 🤦‍♂️ Never thought there is such a great problem with this very basic blocks Lets see, but I hope Apple fix this until the release of iOS 14

onevcat commented 3 years ago

I've sent feedback to Apple for this issue. I will leave it open for a while at least until the GM version comes out. Then we can check if there is a real solution for it.

onevcat commented 3 years ago

Unfortunately, it was not fixed in today's Xcode 12 GM. As indicated above, to workaround it, stop using a stack (HStack, VStack or ZStack) inside a NavigationLink. As long as an empty NavigationLink existing, the push navigation seems to be working fine (but the disclosure indicator will be hidden):

List(indexes) { i in
    HStack { 
        Text("Hello ")
        Cell(index: i) 
    }
    NavigationLink(
        destination: Text("Index \(i)"),
        label: {
        })
}

If you want to see the indicator, using a Text("") inside label would do the trick:

List(indexes) { i in
    HStack { 
        Text("Hello ")
        Cell(index: i) 
    }
    NavigationLink(
        destination: Text("Index \(i)"),
        label: {
            Text("")  // Force to show the disclosure indicator
        })
}
onevcat commented 3 years ago

@hassfers @RosayGaspard

I tried some fix (#1510) inside Kingfisher and hopefully it can solve this issue. Can you try to switch to the master branch temporarily to check if it works? If everything goes fine, I will give it a tag then.

tatealive commented 3 years ago

I also experience this same issue and would love for it to be fixed!

TeslaCrest commented 3 years ago

@onevcat MANY THANKS ! Your fix from the master branch is working well in my app !

onevcat commented 3 years ago

5.15.1 was released for this.

lehelmedves commented 3 years ago

@onevcat thank you for the quick implementation. Have you tested 5.15.1 in Xcode 11? It seems to throw some errors. I really need the fix in 5.15.1, but I also still need to build my project with Xcode 11. Is there any way you can make it work?

lehelmedves commented 3 years ago

Am I doing something wrong?

Screen Shot 2020-09-17 at 10 25 56 AM

lehelmedves commented 3 years ago

By the way, I've tried and the issue persists even with Buttons, not just NavigationLinks.

onevcat commented 3 years ago

@lehelmedves I created a fix in this branch https://github.com/onevcat/Kingfisher/tree/fix/swiftui-build-xcode11

However, I am on my vacation now and do not have a Mac at hand to verify it. Please try if that works and if everything goes fine I will tag it in a few days when I return.

lehelmedves commented 3 years ago

You're fast! Actually, it works only if you put the following for both of your modified lines: [weak binder = self.binder]

onevcat commented 3 years ago

Oh. I guess it is exactly what I have done?

lehelmedves commented 3 years ago

Sorry, I misread the file comparison in Git, indeed, exactly what you have done, I missed the naming part.

lehelmedves commented 3 years ago

Quick follow-up question @onevcat: how does caching work with KFImage? Do I have to do anything special to enable caching? Will caching work and get triggered if I'm using URL's like https://getimage.com?id=25 ?

onevcat commented 3 years ago

Fixed and released as 5.15.2 for this issue.

onevcat commented 3 years ago

KFImage uses the same ImageCache under-hood, so it follows everything (maybe you are already familiar) as Kingfisher does. You do not need to do anything to enable cache and it just works by default. If you want to tweak it, follow the cache related section in the Cheat Sheet in Wiki.

lehelmedves commented 3 years ago

Sounds great! Thank you for the info!