Skip to content

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

Merged
andyfinnell merged 4 commits into
pinterest:masterfrom
vnavarro:vnavarro/BUG-PINProgressiveImageCrash
Nov 6, 2026
Merged

andyfinnell merged 4 commits into
pinterest:masterfrom
vnavarro:vnavarro/BUG-PINProgressiveImageCrash

Conversation

@vnavarro

@vnavarro vnavarro commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes an NSInvalidArgumentException crash in -[PINProgressiveImage data] under memory pressure, with an opt-in, per-manager switch on PINRemoteImageManagerConfiguration.

Why

-data does [self.mutableData copy], which doubles peak memory at the worst moment (the end of a large download). Under memory pressure, Foundation's page allocator raises NSInvalidArgumentException from -copy instead of returning nil, and nothing caught it.

Changes

Crash fix

  • PINRemoteLock.lockWithBlock: now unlocks in @finally. Without this, an exception escaping any of the ~40 lockWithBlock: call sites leaves the mutex permanently held (confirmed: 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 (in PINProgressiveImage+Private.h, not the public header) transfers ownership of the buffer instead of copying it. The completion path in PINRemoteImageDownloadTask no longer allocates a second buffer, which removes the doubling instead of just surviving it. A dataTaken flag stops a late in-flight append from corrupting a downstream incremental decode.
  • New -hasData lets the retry/emptiness check avoid materializing the buffer. On a retry the data is discarded anyway, and the allocation itself can fail.

Opt-in switch (per manager, via configuration)

  • New PINRemoteImageManagerConfiguration.memorySafeProgressiveDataEnabled (getter isMemorySafeProgressiveDataEnabled), default NO.
  • It replaces the earlier process-wide PINRemoteImageManager class property, which is removed from the public header. Each manager copies the value from its configuration at init.
  • PINRemoteImageDownloadTask reads it from its manager and forwards it to the progress image through the new PINProgressiveImage.memorySafeDataEnabled property.
  • Off (default): the completion path and -data behave exactly as before.
  • On: completion uses -hasData and -takeData instead of copying, and -data returns nil instead of raising when the copy can't be allocated.
  • Not gated: the PINRemoteLock @finally fix is always on, because it only changes behavior once an exception has already escaped.
PINRemoteImageManagerConfiguration *config = [[PINRemoteImageManagerConfiguration alloc] init];
config.memorySafeProgressiveDataEnabled = YES;
PINRemoteImageManager *manager = [[PINRemoteImageManager alloc] initWithSessionConfiguration:nil
                                                           alternativeRepresentationProvider:nil
                                                                                  imageCache:nil
                                                                        managerConfiguration:config];

Note: +sharedImageManager uses the default configuration, so it can't enable the switch. Callers who want it on must create their own manager with a configuration.

Precedent: PINDiskCache.m in PINCache already wraps its deserializer in @try/@catch (NSException *) for the same class of problem.

Test Plan

  1. Run PINProgressiveImageTests. It covers hasData/takeData semantics, the dataTaken guard, and a dynamic subclass that forces -copy to throw, to prove -data returns nil when memory-safe is on and still throws when it is off.
  2. Run PINRemoteLockTests. It proves the lock can be reacquired from another thread after a block throws.
  3. Run the full suite: xcodebuild test -workspace PINRemoteImage.xcworkspace -scheme PINRemoteImage -destination <iOS simulator>.
Suite Tests Passed Failed
PINRemoteImage_Tests (existing) 62 62 0
PINProgressiveImageTests (new) 8 8 0
PINRemoteLockTests (new) 2 2 0
Total 72 72 0

vnavarro and others added 3 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>
# Short title (50 chars max)
# More detailed description if needed. Wrap around 72 chars.
#
# - Use bullet points if helpful
#

# Subject line (50 chars) - Hook will add [TICKET-123] automatically
#
# Body: Explain WHAT changed and WHY (not HOW)
# - Use bullet points if needed
# - Wrap at ~72 characters
#
# Reference any related issues at the bottom:
# https://jira.pinadmin.com/browse/@TICKET
@vnavarro

vnavarro commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Regarding the Loremflickr failure

PINRemoteImage_Tests (existing) 62 61 1 (testDiskCacheOnLongURLs, external host 500, same on master)

The site has this statement https://loremflickr.com/

2026-10-21
The flop is real: Flickr decided to block our API use again.

@andyfinnell andyfinnell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only blocking change is to fix the currently failing test.

The rest are suggestions.

Comment thread Source/Classes/PINRemoteImageManager.m Resolved
Comment thread Source/Classes/PINRemoteImageManager.m Resolved
Comment thread Source/Classes/PINRemoteLock.m Resolved
@vnavarro

vnavarro commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Regarding the Loremflickr failure

PINRemoteImage_Tests (existing) 62 61 1 (testDiskCacheOnLongURLs, external host 500, same on master)
The site has this statement https://loremflickr.com/

2026-10-21
The flop is real: Flickr decided to block our API use again.

Fixed the test changing to https://tools.placeholder.com/images

@vnavarro
vnavarro force-pushed the vnavarro/BUG-PINProgressiveImageCrash branch from b569622 to 338f1c0 Compare November 6, 2026 15:14
@vnavarro
vnavarro requested a review from andyfinnell November 6, 2026 15:14
@andyfinnell
andyfinnell merged commit 58f6a43 into pinterest:master Nov 6, 2026
7 checks passed
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.

2 participants