Skip to content

Bug 1223132 - Use thread-safe helper instead of UIImage(data: NSData)#1235

Merged
thebnich merged 3 commits intomozilla-mobile:masterfrom
thebnich:screenshot-crash
Nov 10, 2015
Merged

Bug 1223132 - Use thread-safe helper instead of UIImage(data: NSData)#1235
thebnich merged 3 commits intomozilla-mobile:masterfrom
thebnich:screenshot-crash

Conversation

@thebnich
Copy link
Contributor

@thebnich thebnich commented Nov 9, 2015

Note that we also do off-thread operations with UIImage in put:

         return deferDispatchAsync(queue) { () -> Success in
             let imagePath = (self.filesDir as NSString).stringByAppendingPathComponent(key)
             if let data = UIImageJPEGRepresentation(image, self.quality) {

It's unclear whether this is also unsafe, though moving it to the main thread would be pretty costly (IIRC, this operation took hundreds of milliseconds when I ran some benchmarks). I'm inclined to leave it as is for now until we have evidence that it's a problem.

@thebnich thebnich force-pushed the screenshot-crash branch 2 times, most recently from 5e6f2d6 to 47d7089 Compare November 9, 2015 21:06
@rnewman
Copy link
Contributor

rnewman commented Nov 9, 2015

UIImage is safe to use on any thread. From the docs:

Image objects are immutable, so you cannot change their properties after
creation. This means that you generally specify an image’s properties at
initialization time or rely on the image’s metadata to provide the property
value. It also means that image objects are themselves safe to use from any
thread. The way you change the properties of an existing image object is to use
one of the available convenience methods to create a copy of the image but with
the custom value you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is the only one that needs to be run on the main thread, so I think you should roll back the UIImage to NSData change and see if this still fixes the bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we crash before this -- see line linked in bug. The crash happens on the UIImage(data) init.

@thebnich
Copy link
Contributor Author

thebnich commented Nov 9, 2015

This might be an iOS bug -- after some research, I found the same issue reported in the HanekeSwift repo. If we want, we could use the solution from there (using a fine-grained lock around this initializer).

@thebnich thebnich changed the title Bug 1223132 - Create restored screenshots on the main thread Bug 1223132 - Create an intermediate CIImage for restored screenshots Nov 10, 2015
@rnewman
Copy link
Contributor

rnewman commented Nov 10, 2015

If this fixes things, great.

I have a tiny glimmer of fear that it doesn't: CIImage, as far as I can tell, is pretty lazy; that might leave a gap for the same issue to occur. So please test thoroughly before landing!

@rnewman rnewman added this to the Firefox iOS 1.2 milestone Nov 10, 2015
@thebnich thebnich changed the title Bug 1223132 - Create an intermediate CIImage for restored screenshots Bug 1223132 - Use thread-safe helper instead of UIImage(data: NSData) Nov 10, 2015
thebnich added a commit that referenced this pull request Nov 10, 2015
Bug 1223132 - Use thread-safe helper instead of UIImage(data: NSData)
@thebnich thebnich merged commit 5ac33f0 into mozilla-mobile:master Nov 10, 2015
@thebnich thebnich deleted the screenshot-crash branch November 10, 2015 23:52
isabelrios pushed a commit to isabelrios/firefox-ios that referenced this pull request Feb 19, 2024
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.

2 participants