Repository navigation
Conversation
Try fix by removing the doubling memory usage caused by [mutableData copy] which in cases of memory pressure would fault with NSInvalidArgumentException.
Add +[PINRemoteImageManager memorySafeProgressiveDataEnabled] (default NO) so consumers can roll out the PINProgressiveImage OOM fix behind an experiment. When enabled, the download completion path uses -hasData/-takeData instead of copying the buffer, and -[PINProgressiveImage data] catches allocation exceptions. When disabled, both paths behave exactly as before. The PINRemoteLock @try/@finally fix stays unconditional. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: NSInvalidArgumentException crash in PINProgressiveImage -data under memory pressure
-data does [self.mutableData copy], which doubles peak memory at exactly the worst moment (end of a large download). Under memory pressure, Foundation's page allocator raises NSInvalidArgumentException from -copy instead of returning nil, and that exception was uncaught.
PINRemoteLock.lockWithBlock: now unlocks in @finally. Without this, an exception escaping any of the ~40 lockWithBlock: call sites in this library leaves the mutex permanently held (confirmed empirically: pthread_mutex_trylock returns EBUSY after the escape). This is a correctness fix independent of the OOM crash.
PINProgressiveImage.data wraps the copy in @try/@catch and returns nil on failure — -data is already nullable and every caller already tolerates nil.
New -takeData transfers ownership of the buffer instead of copying it, so the completion-handler path (PINRemoteImageDownloadTask) no longer needs to allocate a second buffer at all — this is what actually removes the doubling rather than just surviving it. It's gated by a dataTaken flag so a late in-flight append (there's no window for one on this path, but defense in depth) can't silently corrupt a downstream incremental image decode.
New -hasData let the retry/emptiness check in PINRemoteImageDownloadTask avoid materializing the buffer before deciding whether to retry — on a retry the data is discarded anyway, and the allocation itself can fail.
-takeData is intentionally not on the public header (PINProgressiveImage+Private.h instead): it's destructive, and -data continues to back a copy, readonly property, so it must stay idempotent.
Precedent: PINCache's PINDiskCache.m already wraps its deserializer in @try/@catch (NSException *) for the same class of problem.
Opt-in switch
The OOM-safe data handling is behind a process-wide switch that defaults to off, so consumers can roll it out behind an experiment:
PINRemoteImageManager.memorySafeProgressiveDataEnabled = YES; // set once, before any downloads start
Off (default): the completion path and -[PINProgressiveImage data] behave exactly as before.
On: completion uses -hasData and -takeData instead of copying the buffer, and -data returns nil instead of raising when the copy can't be allocated.
Not gated: the PINRemoteLock @try/@finally fix is always on, because it only changes behavior once an exception has already escaped.
Per download: each download reads the switch once, when it completes.
Testing
New unit tests in PINProgressiveImageTests.m (hasData/takeData semantics, the dataTaken guard, and a dynamic-subclass trick that forces -copy to throw to prove -data survives it) and PINRemoteLockTests.m (proves the lock is reacquirable from another thread after a block throws). Full existing suite passes (2 pre-existing, unrelated network flakes reproduce identically on master).
Full upstream suite run (xcodebuild test, scheme PINREmoteImage, iPhone 17 sim)