merlos / iOS-Open-GPX-Tracker

GPS Tracker app for iOS + WatchOS. Log your tracks without limits and share them; Open source GPX tracker app written in Swift
http://www.merlos.org/iOS-Open-GPX-Tracker/
GNU General Public License v3.0
605 stars 149 forks source link

Fix location permissions warning issue (closes #193) #194

Closed ghost closed 2 years ago

ghost commented 3 years ago

Summary

Fixed an issue (#193) where a location permissions warning alert controller was presented in all cases for a fresh install of the app.

Root Cause

The problem was that the app did not check if the location permissions were requested before, so even if the user was never presented with a prompt the warning case would trigger - meaning users would always be shown the permissions denied warning no matter what they chose.

The fix is to rely on the CLLocationManagerDelegate call back of locationManagerDidChangeAuthorization(_ :) which is called both at initialization time of CLLocationManager and when permissions are changed. The check at appDidBecomeActive time has been removed because the aforementioned delegate call accomplishes the same thing.

See below: bug

My first contribution here, please let me know if anything isn't up to snuff :D Especially on the watch side of things - I applied the same changes but am not 100% on the CoreLocation APIs there.

vincentneo commented 3 years ago

Thanks! I will take a look at it later.

It also appears that the iPhone size detection is not working correctly on the iPhone 12 sizes too... Something to fix later for myself

vincentneo commented 3 years ago
Screenshot 2020-12-08 at 11 01 54 PM

Occasionally getting EXC_BAD_ACCESS on enter background, unsure if its preexisting issue, but don't think I've seen this before.

Since didEnterBackground() method does call on locationManager.stopUpdatingLocation(), think it be safer to look more into this.

ghost commented 3 years ago

Oh wow, not sure what code could be causing this. Will look into it over the coming days. Marking this PR as draft till then...

vincentneo commented 3 years ago

Interestingly, I can't make it throw that exception when I have a breakpoint right before the stopUpdatingLocation. Could it be Xcode's issue?

ghost commented 3 years ago

Honestly I'm having a hard time reproducing the issue - I got it a few times but it is not reliable, and I've got no idea what is triggering it. I will keep trying. I think it's too sketchy to merge. Have you had more luck?

In my experience sometimes Core Data threading issues rear their head with EXC_BAD_ACCESS errors, but I think you would've seen issues/gotten crash logs about this before if those were an issue. I did add the launch argument -com.apple.CoreData.ConcurrencyDebug 1 and it did stop the app but I don't really think that's the issue. I could be wrong, but nothing Core Data related happens when the app enters the background.

EDIT: Based on your screenshot and what I've seen it's interesting that the crash happens on a background thread. I think that's a clue.

vincentneo commented 3 years ago
Screenshot 2020-12-22 at 5 02 48 PM

On device, getting this. Metal-related. might not be caused by this pr, but rather maybe a new iOS 14 thing

Also: this when it doesn't crash

viewController:: didEnterBackground
2020-12-22 17:08:06.814686+0800 OpenGpxTracker[419:139392] [Renderer] IconRenderer: Left horizontal padding (18.000000) plus right horizontal padding (18.000000) is larger than the image width (34.000000). Image will now use the center column of pixels to stretch.
2020-12-22 17:08:07.019343+0800 OpenGpxTracker[419:139397] [Renderer] IconRenderer: Left horizontal padding (18.000000) plus right horizontal padding (18.000000) is larger than the image width (34.000000). Image will now use the center column of pixels to stretch.
vincentneo commented 3 years ago

I haven't checked if this is still replicable in more recent iOS 14 builds, but I feel maybe the benefits outweigh the risks? (if we are doing an update anytime soon)

ghost commented 3 years ago

I haven't checked if this is still replicable in more recent iOS 14 builds, but I feel maybe the benefits outweigh the risks? (if we are doing an update anytime soon)

I've just rebased the branch and ran the app a few times - no issues. But I have not tested it to where I can say that it's not crashing anymore. Will check more tomorrow.

It sounds like you're planning a release soon?

ghost commented 3 years ago

Welp - got the crash. I just panned around then swiped for the app to go into the background. Same stuff as before. iPhone 12 mini iOS 14.5.1

MapView: User interaction has ended
viewController:: didEnterBackground
libc++abi: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument
terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument

Will continue to investigate. I just really don't see what code here could be the issue đŸ‘€

vincentneo commented 3 years ago

It sounds like you're planning a release soon?

That’s not really something I can say, but it seems like iOS 14 related bugs are pilling up, and iOS 15 is around the corner. So it’s good to prepare.

I forgot the crash that this originally caused, and have opened #203. Different place, Same error in console. libc++abi: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument

Since it occurs in a different place, this should be more of a app-wide thing then with your patch…

I wonder what causes it though… so random. Users of the App Store published version might be seeing this crash too, as I am receiving feedback about crashes on recently, running iOS 14

vincentneo commented 2 years ago

As with #202, I decided to merge this today.

Thank you @SebastianWild. Sorry for the long wait.