fix: double-resume of checked continuation in ScreenTextExtractor.extractText(from:) crashes app on tiny OCR screenshots#698
Conversation
…ractText(from:) crashes app on tiny OCR screenshots Fixes FuJacob#502
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private final class OCRContinuationResumer { | ||
| private let lock = NSLock() | ||
| private var hasResumed = false | ||
|
|
||
| func resume(_ action: () -> Void) { | ||
| lock.lock() | ||
| let shouldResume = !hasResumed | ||
| if shouldResume { | ||
| hasResumed = true | ||
| } | ||
| lock.unlock() | ||
|
|
||
| guard shouldResume else { return } | ||
| action() | ||
| } | ||
| } |
There was a problem hiding this comment.
OCRContinuationResumer missing @unchecked Sendable
OCRContinuationResumer is captured by reference inside a DispatchQueue.global(qos:).async closure, which requires its captured values to be Sendable in Swift 5.7+ with -strict-concurrency. The class uses NSLock to protect all mutable state, so it is safe for concurrent access, but without declaring @unchecked Sendable it may produce a warning (or error under stricter project settings) like "capture of 'resumer' with non-sendable type 'OCRContinuationResumer' in an isolated closure". Adding the conformance makes the intent explicit and future-proofs the code.
| for dimension in [1, 2] { | ||
| let image = try makeSolidImage(width: dimension, height: dimension) | ||
| await assertNoRecognizedText(from: image, extractor: extractor) | ||
| } |
There was a problem hiding this comment.
The guard threshold is >= 4, which means 1, 2, and 3 px all short-circuit to .noRecognizedText, but the loop only iterates over [1, 2]. A 3px input is the edge case closest to the threshold and is currently untested. Adding 3 to the array would cover all values immediately below the minimum without meaningfully increasing test runtime.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Fixes the
SWIFT TASK CONTINUATION MISUSEcrash inScreenTextExtractor.extractText(from:). Vision can report a failing request through theVNRecognizeTextRequestcompletion handler and then rethrow the same failure fromhandler.perform(_:), so the checked continuation could resume twice. Resumption is now single-shot through a lock-guarded resumer shared by both paths, and near-zero-sized screenshots (under 4px per side, the known trigger from #502) short-circuit to the existing.noRecognizedTextpath before a Vision request is built.Validation
CotabbyTests/ScreenTextExtractorTests.swiftwith deterministic syntheticCGImagecases: 1x1 and 2x2 inputs must throw aScreenTextExtractionErrorwithout a continuation misuse, an empty normal-size image resumes exactly once with.noRecognizedText, and the downsampling path is unchanged.perform(_:)into the dual error-reporting path the crash log in [Bug] Crashes when OCR handles tiny screenshot during visual context generation #502 shows, and the guarded flow resumes exactly once (callbackCount=1, no crash).xcodebuild testrun did not complete in this environment (SPM package resolution stalled); the new test file is registered inproject.pbxproj(regenerated via XcodeGen) so CI picks it up.Linked issues
Fixes #502
Risk / rollout notes
.noRecognizedTextimmediately instead of reaching Vision. These inputs previously either crashed or produced no text, so the observable change is crash removal.project.pbxprojwas regenerated to register the new test file; no app-target settings changed.Greptile Summary
Fixes the
SWIFT TASK CONTINUATION MISUSEcrash (#502) where Vision's dual error-reporting path (completion handler +handler.performrethrow) could resume aCheckedContinuationtwice. A lock-guardedOCRContinuationResumergate ensures only the first resume call executes, and a 4px minimum-dimension guard short-circuits degenerate screenshots before a Vision request is even built.OCRContinuationResumer: newfinal classusingNSLockto make the "resume once" contract explicit and safe across the callback/async boundary; all threecontinuation.resumecall sites are routed through it..noRecognizedTextimmediately after downsampling, keeping them on the same unavailable-context path as blank screenshots.ScreenTextExtractorTests.swiftcover the 1px/2px short-circuit paths and the blank normal-size image path;project.pbxprojis updated to register the new test file.Confidence Score: 4/5
Safe to merge — the crash fix is correctly scoped and the minimum-dimension guard preserves existing behavior for all images that previously reached Vision.
The lock-and-gate pattern in OCRContinuationResumer is correct: the check-and-set is atomic and the action is executed outside the lock to avoid re-entrancy issues. The one gap is that OCRContinuationResumer is not declared @unchecked Sendable despite crossing a concurrency boundary, which may produce compiler warnings under stricter settings. The 3px boundary case is also untested, but this has no runtime impact.
ScreenTextExtractor.swift — the OCRContinuationResumer class should declare @unchecked Sendable; everything else in the file is straightforward.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Caller participant E as extractText(from:) participant R as OCRContinuationResumer participant V as VNImageRequestHandler participant H as Completion Handler C->>E: extractText(from: image) E->>E: downsampledImageIfNeeded() alt "preparedImage < 4px side" E-->>C: throw .noRecognizedText (short-circuit) else image is large enough E->>E: withCheckedThrowingContinuation E->>R: create resumer E->>V: DispatchQueue.async → handler.perform([request]) V->>H: completion(request, error?) alt error in callback H->>R: "resumer.resume { continuation.resume(throwing: .ocrFailed) }" R-->>H: "hasResumed = true, action fired" else success in callback H->>R: "resumer.resume { continuation.resume(returning: text) }" R-->>H: "hasResumed = true, action fired" end alt handler.perform also throws (double-error path) V->>R: "resumer.resume { continuation.resume(throwing: .ocrFailed) }" R-->>V: hasResumed already true, no-op (crash prevented) end E-->>C: ExtractedScreenText or throws endReviews (1): Last reviewed commit: "fix: double-resume of checked continuati..." | Re-trigger Greptile