1223132 - Startup crash when restoring screenshots

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Brian Nicholson (:bnicholson)]

STR:
1) Create a bunch of tabs (I have 30). Make sure most of them have screenshots in the tabs tray.
2) Close/reopen the browser a few times.

[BrowserViewController.swift:375] dequeueQueuedTabs() > Queue. Count: 0.
Client(19225,0x700000635000) malloc: *** error for object 0x7fdf0ae03a30: double free
*** set a breakpoint in malloc_error_break to debug

It points to this line: https://github.com/mozilla/firefox-ios/blob/af523b3c03f01b1b87022a23b46d9be6c76ad807/Storage/DiskImageStore.swift#L50

The first thing I noticed is that we're creating this UIImage on a background thread, but UIKit isn't thread-safe. Postponing the UIImage creation to the main thread seems to fix the crash.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8685074 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

UIImage *is* thread safe. But stuff that uses it might not be. So I think we can be more precise here.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8685074 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

Looks like the docs may be wrong -- see comments!

Comment 4

[Brian Nicholson (:bnicholson)]

Looks like this might be a simulator-only bug. I can't repro on my device, and the issue reported at [1] is also simulator-specific.

[1] https://github.com/Haneke/HanekeSwift/issues/115

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8685074 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

Thanks for the CGImage suggestion. Creating a CGImage from NSData is pretty nasty, but I found yet another image API -- CIImage -- that UIImage accepts as a constructor. Better yet, CIImages can be created directly from NSData.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8685074 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

If this works, great.

Something to think about, perf-wise: the image itself will be decoded as needed, almost certainly on the main thread. (That doesn't change with this patch.)

CGImage gives us some control over when image decoding and caching happens: look up kCGImageSourceShouldCache and kCGImageSourceShouldCacheImmediately. If these have improved since iOS 7, this might allow us to force the UIImage to decode the source JPG or PNG on the background thread, buying us some responsiveness.

If you still have a hacky CGImage-based branch around, maybe give this a shot for perf experimentation?

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

First measure, then change code :-) Add some logging around image decoding to find out how long it takes and on what thread it happens pls.

Comment 8

[Brian Nicholson (:bnicholson)]

There is indeed a noticeable delay (~500ms) introduced this patch applied, when opening the tabs tray with 15 tabs open. Interestingly, this delay happens every time I open the tabs tray and not just the first time -- at worst, I was expecting CIImage to lazily decode the image just once on the main thread, caching the result for later.

I never had a working CGImage implementation since I found CIImage before I got anywhere. I'll give it another go now.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8685074 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

CGImage looks like a dead end since, among other things, it requires providing the image width and height when we initialize it. Let's just go with the lock-based approach for now; it should be fine perf-wise since we'll be synchronizing access off the main thread. This also gives us a generic fix that we can use to replace all instances of UIImage(data: NSData).

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8685074 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1235

If it works, LGTM.

Comment 11

[Brian Nicholson (:bnicholson)]

https://github.com/mozilla/firefox-ios/commit/5ac33f075b03887135bf8cd3efccd97f043112aa