Skip to content

Fix for NSInvalidArgumentException in PINProgressiveImage.m:268 - #657

Open
vnavarro wants to merge 2 commits into
pinterest:masterfrom
vnavarro:vnavarro/BUG-PINProgressiveImageCrash
Open

vnavarro wants to merge 2 commits into
pinterest:masterfrom
vnavarro:vnavarro/BUG-PINProgressiveImageCrash

Conversation

@vnavarro

@vnavarro vnavarro commented Jul 30, 2026 •

Copy link
Copy Markdown

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.

  1. 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.

  2. PINProgressiveImage.data wraps the copy in @try/@catch and returns nil on failure — -data is already nullable and every caller already tolerates nil.

  3. 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.

  4. 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).

Test File Result Duration
testHasDataIsFalseBeforeAnyDataArrives PINProgressiveImageTests.m ✅ Passed 0.000s
testHasDataIsTrueAfterDataArrives PINProgressiveImageTests.m ✅ Passed 0.000s
testHasDataIsTrueForAllocatedZeroLengthBuffer PINProgressiveImageTests.m ✅ Passed 0.000s
testDataLengthDoesNotRequireData PINProgressiveImageTests.m ✅ Passed 0.021s
testTakeDataReturnsFullBufferOnceThenNil PINProgressiveImageTests.m ✅ Passed 0.000s
testTakeDataOnEmptyProgressiveImageReturnsNil PINProgressiveImageTests.m ✅ Passed 0.006s
testLateAppendAfterTakeDataIsIgnored PINProgressiveImageTests.m ✅ Passed 0.000s
testDataReturnsNilInsteadOfThrowingWhenCopyFails PINProgressiveImageTests.m ✅ Passed 0.006s
testLockWithBlockReleasesTheLockWhenTheBlockThrows PINRemoteLockTests.m ✅ Passed 0.005s
testLockWithBlockStillRunsAndUnlocksOnTheHappyPath PINRemoteLockTests.m ✅ Passed 0.008s

Full upstream suite run (xcodebuild test, scheme PINREmoteImage, iPhone 17 sim)

Test suite Tests Passed Failed
PINRemoteImage_Tests (existing) 62 60 2
PINProgressiveImageTests (new) 8 8 0
PINRemoteLockTests (new) 2 2 0
Total 72 70 2

vnavarro and others added 2 commits July 30, 2026 17:03
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>
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.

1 participant