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

Removes retain cycle prevention #106

Closed
wants to merge 2 commits into from

Conversation

steam
Copy link

@steam steam commented Jan 6, 2017

On September 18th, code was added with the aim of preventing a retain cycle. This code inadvertantly creates a crash if the user has denied photo access and the user dismisses the action sheet. It has to do with setting actions = nil in SheetController.

On September 18th, code was added with the aim of preventing a retain cycle. This code inadvertantly creates a crash if the user has denied photo access and the user dismisses the action sheet. It has to do with setting `actions = nil` in SheetController.
@lbrndnr
Copy link
Owner

lbrndnr commented Jan 6, 2017

@steam Looking good! I think it would be better to just set the actions to nil directly, don't you think? removeAllActions shouldn't be a user facing method so it's kind of redundant, no?

The underlying issue turns out to be related to a well documented behavior in PHCachingImageManager - http://stackoverflow.com/questions/33266346/photos-framework-crash-this-application-is-not-allowed-to-access-photo-data. TL;DR - apps crash when a PHCachingImageManager is deallocated if it was created and access to the user’s photos are denied. Nice find @colinta 🤘.
@@ -31,13 +31,19 @@ public enum ImagePickerMediaType {

}

fileprivate let imageManager = PHCachingImageManager()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lbrndnr This is the crux of the problem/bug-fix. When this object is deallocated, if permission to photos has not been given, apple issues an exception of "permission not given". Thanks apple!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying issue turns out to be related to a well documented behavior in PHCachingImageManager - http://stackoverflow.com/questions/33266346/photos-framework-crash-this-application-is-not-allowed-to-access-photo-data. Nice find @colinta 🤘.

This should prevent the crash for all users. We plan to go a step further and guard against displaying the picker when the user has denied access to the photos library. We'll display a message to the user directing them to make the change in their settings.

@steam steam closed this Sep 15, 2018
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

Successfully merging this pull request may close these issues.

3 participants