Fix a crash that can sometimes happen when cancelling (#189)

Gifski/Gifski.swift

@@ -47,9 +47,9 @@ final class Gifski {
 		var loopGif: Bool = true
 	}
 
-	// TODO: `NSMutableData` is not thread-safe. We should use a lock when writing to it.
 	private var gifData = NSMutableData()
 	private var progress: Progress!
+	private var gifski: GifskiWrapper?
 
 	// TODO: Split this method up into smaller methods. It's too large.
 	/**
@@ -68,6 +68,10 @@ final class Gifski {
 		progress = Progress(parent: .current())
 
 		let completionHandlerOnce = Once().wrap { [weak self] (_ result: Result<Data, Error>) -> Void in
+			// Ensure libgifski finishes no matter what.
+			try? self?.gifski?.finish()
+			self?.gifski?.release()
+
 			DispatchQueue.main.async {
 				guard
 					let self = self,
@@ -89,27 +93,29 @@ final class Gifski {
 			fast: false
 		)
 
-		guard let gifski = GifskiWrapper(settings: settings) else {
+		self.gifski = GifskiWrapper(settings: settings)
+
+		guard let gifski = gifski else {
 			completionHandlerOnce(.failure(.invalidSettings))
 			return
 		}
 
-		gifski.setProgressCallback(context: &progress) { context in
-			let progress = context!.assumingMemoryBound(to: Progress.self).pointee
-			progress.completedUnitCount += 1
-			return progress.isCancelled ? 0 : 1
+		gifski.setProgressCallback { [weak self] in
+			guard let self = self else {
+				return 1
+			}
+
+			self.progress.completedUnitCount += 1
+
+			return self.progress.isCancelled ? 0 : 1
 		}
 
-		gifski.setWriteCallback(context: &gifData) { bufferLength, bufferPointer, context in
-			guard
-				bufferLength > 0,
-				let bufferPointer = bufferPointer
-			else {
+		gifski.setWriteCallback { [weak self] bufferLength, bufferPointer in
+			guard let self = self else {
 				return 0
 			}
 
-			let data = context!.assumingMemoryBound(to: NSMutableData.self).pointee
-			data.append(bufferPointer, length: bufferLength)
+			self.gifData.append(bufferPointer, length: bufferLength)
 
 			return 0
 		}

Gifski/GifskiWrapper.swift

@@ -47,8 +47,11 @@ enum GifskiWrapperError: UInt32, LocalizedError {
 	}
 }
 
+/// - Important: Don't forget to call `.release()` when done, whether it succeeded or failed.
 final class GifskiWrapper {
 	private let pointer: OpaquePointer
+	private var unmanagedSelf: Unmanaged<GifskiWrapper>!
+	private var hasFinished = false
 
 	init?(settings: GifskiSettings) {
 		var settings = settings
@@ -58,6 +61,9 @@ final class GifskiWrapper {
 		}
 
 		self.pointer = pointer
+
+		// We need to keep a strong reference to self so we can ensure it's not deallocated before libgifski finishes writing.
+		self.unmanagedSelf = Unmanaged.passRetained(self)
 	}
 
 	private func wrap(_ fn: () -> GifskiError) throws {
@@ -68,18 +74,53 @@ final class GifskiWrapper {
 		}
 	}
 
-	func setProgressCallback(
-		context: UnsafeMutableRawPointer,
-		callback: @escaping (@convention(c) (UnsafeMutableRawPointer?) -> Int32)
-	) {
-		gifski_set_progress_callback(pointer, callback, context)
+	typealias ProgressCallback = () -> Int
+
+	private var progressCallback: ProgressCallback!
+
+	func setProgressCallback(_ callback: @escaping ProgressCallback) {
+		guard !hasFinished else {
+			return
+		}
+
+		progressCallback = callback
+
+		gifski_set_progress_callback(
+			pointer,
+			{ context in // swiftlint:disable:this opening_brace
+				let this = Unmanaged<GifskiWrapper>.fromOpaque(context!).takeUnretainedValue()
+				return Int32(this.progressCallback())
+			},
+			unmanagedSelf.toOpaque()
+		)
 	}
 
-	func setWriteCallback(
-		context: UnsafeMutableRawPointer,
-		callback: (@convention(c) (Int, UnsafePointer<UInt8>?, UnsafeMutableRawPointer?) -> Int32)?
-	) {
-		gifski_set_write_callback(pointer, callback, context)
+	typealias WriteCallback = (Int, UnsafePointer<UInt8>) -> Int
+
+	private var writeCallback: WriteCallback!
+
+	func setWriteCallback(_ callback: @escaping WriteCallback) {
+		guard !hasFinished else {
+			return
+		}
+
+		writeCallback = callback
+
+		gifski_set_write_callback(
+			pointer,
+			{ bufferLength, bufferPointer, context in // swiftlint:disable:this opening_brace
+				guard
+					bufferLength > 0,
+					let bufferPointer = bufferPointer
+				else {
+					return 0
+				}
+
+				let this = Unmanaged<GifskiWrapper>.fromOpaque(context!).takeUnretainedValue()
+				return Int32(this.writeCallback(bufferLength, bufferPointer))
+			},
+			unmanagedSelf.toOpaque()
+		)
 	}
 
 	// swiftlint:disable:next function_parameter_count
@@ -91,6 +132,10 @@ final class GifskiWrapper {
 		pixels: UnsafePointer<UInt8>,
 		presentationTimestamp: Double
 	) throws {
+		guard !hasFinished else {
+			return
+		}
+
 		try wrap {
 			gifski_add_frame_argb(
 				pointer,
@@ -105,6 +150,16 @@ final class GifskiWrapper {
 	}
 
 	func finish() throws {
+		guard !hasFinished else {
+			return
+		}
+
+		hasFinished = true
+
 		try wrap { gifski_finish(pointer) }
 	}
+
+	func release() {
+		unmanagedSelf.release()
+	}
 }