Skip to content
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

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

Closed
hassfers opened this issue Sep 14, 2020 · 26 comments
Closed

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

hassfers opened this issue Sep 14, 2020 · 26 comments

Comments

@hassfers
Copy link

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
Copy link
Owner

onevcat commented Sep 14, 2020

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
Copy link
Author

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

@CyberBison
Copy link

CyberBison commented Sep 15, 2020

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
Copy link
Owner

onevcat commented Sep 15, 2020

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

@CyberBison
Copy link

CyberBison commented Sep 15, 2020

@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
Copy link
Owner

onevcat commented Sep 15, 2020

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.

@CyberBison
Copy link

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
Copy link
Owner

onevcat commented Sep 15, 2020

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
Copy link
Author

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
Copy link
Owner

onevcat commented Sep 15, 2020

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
Copy link
Owner

onevcat commented Sep 16, 2020

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
Copy link
Owner

onevcat commented Sep 16, 2020

@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
Copy link

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

@CyberBison
Copy link

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

@onevcat
Copy link
Owner

onevcat commented Sep 16, 2020

5.15.1 was released for this.

@onevcat onevcat closed this as completed Sep 16, 2020
@lehelmedves
Copy link

@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
Copy link

Am I doing something wrong?

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

@lehelmedves
Copy link

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

@onevcat
Copy link
Owner

onevcat commented Sep 17, 2020

@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
Copy link

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

@onevcat
Copy link
Owner

onevcat commented Sep 17, 2020

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

@lehelmedves
Copy link

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

@lehelmedves
Copy link

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
Copy link
Owner

onevcat commented Sep 19, 2020

Fixed and released as 5.15.2 for this issue.

@onevcat
Copy link
Owner

onevcat commented Sep 19, 2020

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
Copy link

Sounds great! Thank you for the info!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants