From eb55b1e3a6733ad99b61b3caebb7284bc911d26f Mon Sep 17 00:00:00 2001 From: Vitor Navarro Date: Thu, 30 Jul 2026 17:03:37 -0300 Subject: [PATCH 1/4] Fix for NSInvalidArgumentException in PINProgressiveImage.m:268 Try fix by removing the doubling memory usage caused by [mutableData copy] which in cases of memory pressure would fault with NSInvalidArgumentException. --- PINRemoteImage.xcodeproj/project.pbxproj | 14 ++ Source/Classes/PINProgressiveImage+Private.h | 31 ++++ Source/Classes/PINProgressiveImage.m | 49 +++++- Source/Classes/PINRemoteImageDownloadTask.m | 22 ++- Source/Classes/PINRemoteLock.m | 11 +- .../PINRemoteImage/PINProgressiveImage.h | 3 + Tests/PINProgressiveImageTests.m | 162 ++++++++++++++++++ Tests/PINRemoteLockTests.m | 63 +++++++ 8 files changed, 347 insertions(+), 8 deletions(-) create mode 100644 Source/Classes/PINProgressiveImage+Private.h create mode 100644 Tests/PINProgressiveImageTests.m create mode 100644 Tests/PINRemoteLockTests.m diff --git a/PINRemoteImage.xcodeproj/project.pbxproj b/PINRemoteImage.xcodeproj/project.pbxproj index cac0ba98..9c45470a 100644 --- a/PINRemoteImage.xcodeproj/project.pbxproj +++ b/PINRemoteImage.xcodeproj/project.pbxproj @@ -44,6 +44,8 @@ 139D50B01F672BBF00DE64E0 /* PINRemoteImageDownloadQueue.h in Headers */ = {isa = PBXBuildFile; fileRef = 68B1F27F1E679D7A00ED87C4 /* PINRemoteImageDownloadQueue.h */; }; 139D50B11F672BBF00DE64E0 /* PINResume.h in Headers */ = {isa = PBXBuildFile; fileRef = 68B7E3B01E736C73000FC887 /* PINResume.h */; }; 139D50B21F672BBF00DE64E0 /* PINSpeedRecorder.h in Headers */ = {isa = PBXBuildFile; fileRef = 6860CB061F578287005E886E /* PINSpeedRecorder.h */; }; + 15E2A2683A71757D64EADA89 /* PINProgressiveImage+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */; }; + 2035B2727792667AFEFDC0F2 /* PINRemoteLockTests.m in Sources */ = {isa = PBXBuildFile; fileRef = E9278EF02C46E2F93F5DBB8E /* PINRemoteLockTests.m */; }; 440872302BFFC55700C053C2 /* PINRemoteImageDataConvertible.h in Headers */ = {isa = PBXBuildFile; fileRef = 4408722F2BFFC4AF00C053C2 /* PINRemoteImageDataConvertible.h */; settings = {ATTRIBUTES = (Public, ); }; }; 440872312BFFC55B00C053C2 /* PINRemoteImageDataConvertible.h in Headers */ = {isa = PBXBuildFile; fileRef = 4408722F2BFFC4AF00C053C2 /* PINRemoteImageDataConvertible.h */; settings = {ATTRIBUTES = (Public, ); }; }; 445C57202BF26B6A0005D70F /* NSData+ImageDetectors.h in Headers */ = {isa = PBXBuildFile; fileRef = 445C570A2BF26B6A0005D70F /* NSData+ImageDetectors.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -153,6 +155,8 @@ B5BB4024274DB69F0042AE1D /* Media.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = B5BB4022274DB69F0042AE1D /* Media.xcassets */; }; D93A340224182D46005EB15E /* TestAnimatedImage.m in Sources */ = {isa = PBXBuildFile; fileRef = D93A340124182D46005EB15E /* TestAnimatedImage.m */; }; D93A340324182D46005EB15E /* TestAnimatedImage.m in Sources */ = {isa = PBXBuildFile; fileRef = D93A340124182D46005EB15E /* TestAnimatedImage.m */; }; + EAFC80CEC3D80C1A6CABE195 /* PINProgressiveImageTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 1386DBFC97AF2485ED332AF1 /* PINProgressiveImageTests.m */; }; + F17058DB8820DB9B406074E0 /* PINProgressiveImage+Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */; }; F1B918FF1BCF23C900710963 /* PINRemoteImageCategoryManager.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918DC1BCF23C800710963 /* PINRemoteImageCategoryManager.m */; }; F1B919011BCF23C900710963 /* NSData+ImageDetectors.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918DF1BCF23C800710963 /* NSData+ImageDetectors.m */; }; F1B9190F1BCF23C900710963 /* PINProgressiveImage.m in Sources */ = {isa = PBXBuildFile; fileRef = F1B918EE1BCF23C800710963 /* PINProgressiveImage.m */; }; @@ -201,8 +205,10 @@ /* Begin PBXFileReference section */ 0CF0743524093139008F126B /* PINAPNGAnimatedImage.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PINAPNGAnimatedImage.m; sourceTree = ""; }; 0E71BE2522E94C7200FC5B99 /* fireworks.gif */ = {isa = PBXFileReference; lastKnownFileType = image.gif; path = fireworks.gif; sourceTree = ""; }; + 1386DBFC97AF2485ED332AF1 /* PINProgressiveImageTests.m */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.objc; path = PINProgressiveImageTests.m; sourceTree = ""; }; 139D3A5A1F67284400F82935 /* PINRemoteImage.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = PINRemoteImage.framework; sourceTree = BUILT_PRODUCTS_DIR; }; 139D3A621F67284400F82935 /* PINRemoteImage-tvOSTests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = "PINRemoteImage-tvOSTests.xctest"; sourceTree = BUILT_PRODUCTS_DIR; }; + 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = "PINProgressiveImage+Private.h"; sourceTree = ""; }; 4408722F2BFFC4AF00C053C2 /* PINRemoteImageDataConvertible.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = PINRemoteImageDataConvertible.h; sourceTree = ""; }; 4419469E2BF3AAF100E7A59C /* WebP.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = WebP.xcframework; path = webp/WebP.xcframework; sourceTree = ""; }; 441946A12BF3AB6500E7A59C /* WebPDemux.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = WebPDemux.xcframework; path = webp/WebPDemux.xcframework; sourceTree = ""; }; @@ -283,6 +289,7 @@ B5BB4022274DB69F0042AE1D /* Media.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Media.xcassets; sourceTree = ""; }; D93A340024182D46005EB15E /* TestAnimatedImage.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = TestAnimatedImage.h; sourceTree = ""; }; D93A340124182D46005EB15E /* TestAnimatedImage.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = TestAnimatedImage.m; sourceTree = ""; }; + E9278EF02C46E2F93F5DBB8E /* PINRemoteLockTests.m */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.objc; path = PINRemoteLockTests.m; sourceTree = ""; }; F1B918D11BCF239200710963 /* PINRemoteImage.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = PINRemoteImage.framework; sourceTree = BUILT_PRODUCTS_DIR; }; F1B918D61BCF239200710963 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; F1B918DC1BCF23C800710963 /* PINRemoteImageCategoryManager.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PINRemoteImageCategoryManager.m; sourceTree = ""; }; @@ -409,6 +416,8 @@ ACD288C6E6B13E6DA226D252 /* NSDate+PINCacheTests.h */, ACD28D963D79EEC14EE071CE /* NSDate+PINCacheTests.m */, B5BB4022274DB69F0042AE1D /* Media.xcassets */, + 1386DBFC97AF2485ED332AF1 /* PINProgressiveImageTests.m */, + E9278EF02C46E2F93F5DBB8E /* PINRemoteLockTests.m */, ); path = Tests; sourceTree = ""; @@ -508,6 +517,7 @@ 68B7E3B11E736C73000FC887 /* PINResume.m */, 6860CB061F578287005E886E /* PINSpeedRecorder.h */, 6860CB071F578287005E886E /* PINSpeedRecorder.m */, + 14326ABB868F40FB6CE7328E /* PINProgressiveImage+Private.h */, ); name = Project; path = Classes; @@ -621,6 +631,7 @@ 139D50B21F672BBF00DE64E0 /* PINSpeedRecorder.h in Headers */, 68912D87208FDB4900F5FE0E /* PINRemoteWeakProxy.h in Headers */, 689613E5208FD90B00D2095C /* PINDisplayLink.h in Headers */, + 15E2A2683A71757D64EADA89 /* PINProgressiveImage+Private.h in Headers */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -667,6 +678,7 @@ 445C572C2BF26B6A0005D70F /* PINRemoteImage.h in Headers */, 445C57202BF26B6A0005D70F /* NSData+ImageDetectors.h in Headers */, 445C57222BF26B6A0005D70F /* PINAnimatedImage.h in Headers */, + F17058DB8820DB9B406074E0 /* PINProgressiveImage+Private.h in Headers */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -879,6 +891,8 @@ 683128F51F95045200D5B4A8 /* PINAnimatedImage+PINAnimatedImageTesting.m in Sources */, D93A340224182D46005EB15E /* TestAnimatedImage.m in Sources */, ACD28AB87FABF6BA3B9BF4E4 /* NSDate+PINCacheTests.m in Sources */, + EAFC80CEC3D80C1A6CABE195 /* PINProgressiveImageTests.m in Sources */, + 2035B2727792667AFEFDC0F2 /* PINRemoteLockTests.m in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/Source/Classes/PINProgressiveImage+Private.h b/Source/Classes/PINProgressiveImage+Private.h new file mode 100644 index 00000000..c4580ffc --- /dev/null +++ b/Source/Classes/PINProgressiveImage+Private.h @@ -0,0 +1,31 @@ +// +// PINProgressiveImage+Private.h +// PINRemoteImage +// +// Created by Garrett Moon on 2/9/15. +// +// + +#import + +NS_ASSUME_NONNULL_BEGIN + +@interface PINProgressiveImage (Private) + +/** + Returns the current data and relinquishes ownership of it, avoiding a copy. + + This is destructive and deliberately kept out of the public header: it must only be + called once no further data can be appended for this task -- i.e. from the URL + session completion handler, after every -didReceiveData: for the task has already + been delivered synchronously on its serial delegate queue. Calling it from any other + context (e.g. mid-flight, via KVC, or from a debugger) can silently truncate the + buffer fed to an in-progress incremental image decode. + + @return NSData The data accumulated so far. Returns nil on a second call. + */ +- (nullable NSData *)takeData; + +@end + +NS_ASSUME_NONNULL_END diff --git a/Source/Classes/PINProgressiveImage.m b/Source/Classes/PINProgressiveImage.m index ff37e8b9..80baec1c 100644 --- a/Source/Classes/PINProgressiveImage.m +++ b/Source/Classes/PINProgressiveImage.m @@ -28,6 +28,10 @@ @interface PINProgressiveImage () @property (nonatomic, assign) NSUInteger scannedByte; @property (nonatomic, assign) NSInteger sosCount; @property (nonatomic, strong) PINRemoteLock *lock; +// Set once -takeData has handed off mutableData. Overloads mutableData == nil (which +// otherwise means "nothing received yet") with a second meaning: "already handed off, +// this object is terminal." See the guard at the top of -updateProgressiveImageWithData:. +@property (nonatomic, assign) BOOL dataTaken; #if DEBUG @property (nonatomic, assign) CFTimeInterval scanTime; #endif @@ -133,6 +137,13 @@ - (CFTimeInterval)l_estimatedRemainingTime - (void)updateProgressiveImageWithData:(nonnull NSData *)data expectedNumberOfBytes:(int64_t)expectedNumberOfBytes isResume:(BOOL)isResume { [self.lock lock]; + if (self.dataTaken) { + // Data was already handed off to the completion path (-takeData); this object + // is terminal. Without this guard, a late append would allocate a fresh, short + // buffer here and silently corrupt the incremental image source it fed earlier. + [self.lock unlock]; + return; + } if (isResume) { NSAssert(self.mutableData == nil, @"If we're resuming, data shouldn't be setup yet."); self.startingBytes = data.length; @@ -265,7 +276,43 @@ - (PINImage *)currentImageBlurred:(BOOL)blurred maxProgressiveRenderSize:(CGSize - (NSData *)data { [self.lock lock]; - NSData *data = [self.mutableData copy]; + NSData *data = nil; + @try { + data = [self.mutableData copy]; + } @catch (NSException *exception) { + // Foundation's page allocator raises NSInvalidArgumentException rather than + // returning nil when it cannot satisfy a large allocation. Callers already + // treat nil as "no data" (see PINRemoteImageDownloadTask). + data = nil; + } + [self.lock unlock]; + return data; +} + +// Exactly equivalent to `[self data] == nil` today, without materializing a copy. +// NOT the same as `data.length == 0` -- a fresh-but-allocated zero-length buffer makes +// `-data` return non-nil (see PINRemoteImageDownloadTask's retry/emptiness check). +- (BOOL)hasData +{ + [self.lock lock]; + BOOL hasData = (self.mutableData != nil); + [self.lock unlock]; + return hasData; +} + +// Transfers ownership of the accumulated buffer to the caller without copying. +// ONLY safe once no further data can arrive for this task -- i.e. from the URL +// session completion handler, whose per-task delegate queue is serial and has +// already delivered every -didReceiveData: synchronously. +// Returns a dynamically-mutable NSMutableData upcast to NSData: it is uniquely owned +// (we drop our reference) and nothing in-repo mutates it. Foundation offers no way to +// obtain a truly immutable NSData from an NSMutableData without copying. +- (NSData *)takeData +{ + [self.lock lock]; + NSData *data = self.mutableData; + self.mutableData = nil; + self.dataTaken = YES; [self.lock unlock]; return data; } diff --git a/Source/Classes/PINRemoteImageDownloadTask.m b/Source/Classes/PINRemoteImageDownloadTask.m index 3c062b7d..0c589bba 100644 --- a/Source/Classes/PINRemoteImageDownloadTask.m +++ b/Source/Classes/PINRemoteImageDownloadTask.m @@ -14,6 +14,7 @@ #import "PINRemoteImageCallbacks.h" #import "PINRemoteLock.h" #import "PINSpeedRecorder.h" +#import "PINProgressiveImage+Private.h" @interface PINRemoteImageDownloadTask () { @@ -311,9 +312,11 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request #endif if (error.code != NSURLErrorCancelled) { - NSData *data = self.progressImage.data; - - if (error == nil && data == nil) { + // Decide emptiness and retry WITHOUT materializing the buffer -- on a retry + // the copy was discarded, and the allocation itself can fail under memory + // pressure. -hasData preserves today's semantics exactly: + // an allocated-but-zero-length buffer is not "empty" (see PINProgressiveImage). + if (error == nil && self.progressImage.hasData == NO) { error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain code:PINRemoteImageManagerErrorImageEmpty userInfo:nil]; @@ -338,6 +341,19 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request return; } + // Terminal: no further data can arrive, so hand off the buffer instead of + // copying it. -takeData allocates nothing, so it cannot throw under memory + // pressure -- this is what actually removes the doubling. + NSData *data = [self.progressImage takeData]; + if (error == nil && data == nil) { + // Load-bearing: with -hasData gating the retry decision above, this only + // fires if -progressImage itself is nil, keeping completionHandler from + // ever seeing (nil, response, nil). + error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain + code:PINRemoteImageManagerErrorImageEmpty + userInfo:nil]; + } + completionHandler(data, response, error); } }]; diff --git a/Source/Classes/PINRemoteLock.m b/Source/Classes/PINRemoteLock.m index 6cf74d62..830649c6 100644 --- a/Source/Classes/PINRemoteLock.m +++ b/Source/Classes/PINRemoteLock.m @@ -80,13 +80,16 @@ - (void)lockWithBlock:(dispatch_block_t)block #else pthread_mutex_lock(&_lock); #endif - block(); + @try { + block(); + } @finally { #if PINREMOTELOCK_DEBUG - [_lock unlock]; - [_recursiveLock unlock]; + [_lock unlock]; + [_recursiveLock unlock]; #else - pthread_mutex_unlock(&_lock); + pthread_mutex_unlock(&_lock); #endif + } } - (void)lock diff --git a/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h b/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h index e5e57b46..da3e38c9 100644 --- a/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h +++ b/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h @@ -49,4 +49,7 @@ */ - (nullable NSData *)data; +/// YES if any data has been received. Equivalent to `[self data] != nil`, without copying. +@property (nonatomic, readonly) BOOL hasData; + @end diff --git a/Tests/PINProgressiveImageTests.m b/Tests/PINProgressiveImageTests.m new file mode 100644 index 00000000..a5b20f01 --- /dev/null +++ b/Tests/PINProgressiveImageTests.m @@ -0,0 +1,162 @@ +// +// PINProgressiveImageTests.m +// PINRemoteImage Tests +// +// Tests for the memory-safety fix to PINProgressiveImage's data hand-off: +// - `-data` must survive an allocation failure by returning nil instead of +// letting NSInvalidArgumentException escape (Bugsnag 67fad806301606795e34c849). +// - `-hasData` must answer without copying the buffer. +// - `-takeData` must transfer ownership without allocating, and must be +// guarded so a late append after hand-off cannot silently corrupt state. +// + +#import +#import + +#import +#import "PINProgressiveImage+Private.h" + +/// Dynamically subclasses `object`'s class so that `-copy`/`-copyWithZone:` throw, +/// mimicking Foundation's page allocator raising NSInvalidArgumentException instead +/// of returning nil for a large allocation it can't satisfy. Scoped to the single +/// instance via isa-swizzling so it can't affect any other NSMutableData in the process. +static void PINTestMakeCopyThrowOnInstance(NSObject *object) +{ + Class originalClass = object_getClass(object); + NSString *name = [NSString stringWithFormat:@"PINTestThrowingCopy_%@_%p", NSStringFromClass(originalClass), object]; + Class throwingClass = objc_allocateClassPair(originalClass, name.UTF8String, 0); + + IMP throwingCopyWithZoneIMP = imp_implementationWithBlock(^id(id _self, NSZone *zone) { + @throw [NSException exceptionWithName:NSInvalidArgumentException + reason:@"*** NSAllocateMemoryPages(N) failed" + userInfo:nil]; + }); + IMP throwingCopyIMP = imp_implementationWithBlock(^id(id _self) { + @throw [NSException exceptionWithName:NSInvalidArgumentException + reason:@"*** NSAllocateMemoryPages(N) failed" + userInfo:nil]; + }); + class_addMethod(throwingClass, @selector(copyWithZone:), throwingCopyWithZoneIMP, "@@:^{_NSZone=}"); + class_addMethod(throwingClass, @selector(copy), throwingCopyIMP, "@@:"); + + objc_registerClassPair(throwingClass); + object_setClass(object, throwingClass); +} + +@interface PINProgressiveImageTests : XCTestCase + +@end + +@implementation PINProgressiveImageTests + +- (PINProgressiveImage *)freshProgressiveImage +{ + NSURLSessionDataTask *dataTask = [[NSURLSession sharedSession] dataTaskWithURL:[NSURL URLWithString:@"https://example.com/image.jpg"]]; + return [[PINProgressiveImage alloc] initWithDataTask:dataTask]; +} + +#pragma mark - hasData + +- (void)testHasDataIsFalseBeforeAnyDataArrives +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + XCTAssertFalse(progressiveImage.hasData); + XCTAssertNil(progressiveImage.data); + XCTAssertEqual(progressiveImage.data.length, (NSUInteger)0); +} + +- (void)testHasDataIsTrueAfterDataArrives +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"hello" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + XCTAssertTrue(progressiveImage.hasData); + XCTAssertEqualObjects(progressiveImage.data, chunk); + XCTAssertEqual(progressiveImage.data.length, chunk.length); +} + +// Regression guard: an allocated-but-empty buffer must NOT be treated as "no data". +// This is the one case where `data.length == 0` and `hasData == NO` disagree; the +// retry/emptiness check in PINRemoteImageDownloadTask must use -hasData, not +// data.length, or a zero-byte `didReceiveData:` would newly become ErrorImageEmpty. +- (void)testHasDataIsTrueForAllocatedZeroLengthBuffer +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + [progressiveImage updateProgressiveImageWithData:[NSData data] expectedNumberOfBytes:0 isResume:NO]; + + XCTAssertTrue(progressiveImage.hasData, @"An allocated (even if zero-length) buffer must count as having data, matching today's [mutableData copy] != nil behavior."); + XCTAssertEqual(progressiveImage.data.length, (NSUInteger)0); + XCTAssertNotNil(progressiveImage.data); +} + +#pragma mark - takeData + +- (void)testTakeDataReturnsFullBufferOnceThenNil +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"the quick brown fox" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + NSData *taken = [progressiveImage takeData]; + XCTAssertEqualObjects(taken, chunk); + + XCTAssertNil([progressiveImage takeData], @"A second -takeData call must return nil, not the same buffer again."); + XCTAssertFalse(progressiveImage.hasData, @"After -takeData, the object is terminal: hasData must be NO."); + XCTAssertNil(progressiveImage.data, @"After -takeData, -data must be NO, not re-copy the (now nil'd) buffer."); +} + +- (void)testTakeDataOnEmptyProgressiveImageReturnsNil +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + XCTAssertNil([progressiveImage takeData]); +} + +// The mandatory companion to -takeData: mutableData == nil is an overloaded sentinel +// that means both "nothing received yet" and (post-fix) "already handed off". Without +// the dataTaken guard, a late append after -takeData would allocate a fresh, short +// buffer and silently feed it to CGImageSourceUpdateData, corrupting the decode instead +// of crashing. This test proves the guard makes updateProgressiveImageWithData: a no-op +// once data has been taken. +- (void)testLateAppendAfterTakeDataIsIgnored +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *firstChunk = [@"first" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:firstChunk expectedNumberOfBytes:(int64_t)firstChunk.length isResume:NO]; + + NSData *taken = [progressiveImage takeData]; + XCTAssertEqualObjects(taken, firstChunk); + + // Simulate a late in-flight `didReceiveData:` racing the completion handler. + NSData *lateChunk = [@"late-arriving-bytes" dataUsingEncoding:NSUTF8StringEncoding]; + XCTAssertNoThrow([progressiveImage updateProgressiveImageWithData:lateChunk expectedNumberOfBytes:(int64_t)lateChunk.length isResume:NO]); + + XCTAssertFalse(progressiveImage.hasData, @"A late append after -takeData must be dropped, not silently start a new, short buffer."); + XCTAssertEqual(progressiveImage.data.length, (NSUInteger)0); + XCTAssertNil(progressiveImage.data); +} + +#pragma mark - -data survives allocation failure + +- (void)testDataReturnsNilInsteadOfThrowingWhenCopyFails +{ + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"payload" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + NSMutableData *mutableData = [progressiveImage valueForKey:@"mutableData"]; + XCTAssertNotNil(mutableData); + PINTestMakeCopyThrowOnInstance(mutableData); + + __block NSData *data; + XCTAssertNoThrow(data = progressiveImage.data, @"An allocation failure inside -copy must not escape -data as an exception."); + XCTAssertNil(data); + + // The lock must not be left held: a subsequent call through the same lock must + // not deadlock. (Regression coverage for the mutex-leak fix lives in + // PINRemoteLockTests; this just confirms -data's own explicit lock/unlock pairs + // correctly around the @try/@catch.) + XCTAssertTrue(progressiveImage.hasData, @"hasData does not copy, so it must still reflect the buffer's presence even though -data failed."); +} + +@end diff --git a/Tests/PINRemoteLockTests.m b/Tests/PINRemoteLockTests.m new file mode 100644 index 00000000..16eca9df --- /dev/null +++ b/Tests/PINRemoteLockTests.m @@ -0,0 +1,63 @@ +// +// PINRemoteLockTests.m +// PINRemoteImage Tests +// +// Regression coverage for the mutex leak fixed in PINRemoteLock: before the fix, +// an exception escaping the block passed to -lockWithBlock: skipped the matching +// unlock, leaving the lock permanently held. That single bug meant every one of +// PINProgressiveImage/PINRemoteImageDownloadTask's ~40 `lockWithBlock:` call sites +// was one exception away from a permanent deadlock, not just the OOM copy site. +// + +#import +#import "PINRemoteLock.h" + +@interface PINRemoteLockTests : XCTestCase + +@end + +@implementation PINRemoteLockTests + +- (void)testLockWithBlockReleasesTheLockWhenTheBlockThrows +{ + PINRemoteLock *lock = [[PINRemoteLock alloc] initWithName:@"PINRemoteLockTests"]; + + XCTAssertThrowsSpecificNamed({ + [lock lockWithBlock:^{ + @throw [NSException exceptionWithName:@"PINRemoteLockTestException" reason:@"boom" userInfo:nil]; + }]; + }, NSException, @"PINRemoteLockTestException"); + + // If -lockWithBlock: leaked the lock on the exception above, this second + // acquisition (deliberately from another thread, since the lock is + // non-recursive) would hang forever instead of completing. + XCTestExpectation *reacquired = [self expectationWithDescription:@"lock reacquired after throwing block"]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + [lock lockWithBlock:^{ + [reacquired fulfill]; + }]; + }); + + [self waitForExpectationsWithTimeout:5.0 handler:^(NSError * _Nullable error) { + XCTAssertNil(error, @"lockWithBlock: appears to have leaked the mutex after an exception escaped the block."); + }]; +} + +- (void)testLockWithBlockStillRunsAndUnlocksOnTheHappyPath +{ + PINRemoteLock *lock = [[PINRemoteLock alloc] initWithName:@"PINRemoteLockTests"]; + + __block BOOL ran = NO; + [lock lockWithBlock:^{ + ran = YES; + }]; + XCTAssertTrue(ran); + + XCTestExpectation *reacquired = [self expectationWithDescription:@"lock reacquired on happy path"]; + [lock lockWithBlock:^{ + [reacquired fulfill]; + }]; + [self waitForExpectationsWithTimeout:5.0 handler:nil]; +} + +@end From ecdc6f09d92e4cf2805c997c4c52d3ef59db9789 Mon Sep 17 00:00:00 2001 From: Vitor Navarro Date: Fri, 23 Oct 2026 14:35:13 -0300 Subject: [PATCH 2/4] Gate memory-safe progressive data handling behind an opt-in switch 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 --- Source/Classes/PINProgressiveImage.m | 16 +++++---- Source/Classes/PINRemoteImageDownloadTask.m | 36 +++++++++++-------- Source/Classes/PINRemoteImageManager.m | 12 +++++++ .../PINRemoteImage/PINRemoteImageManager.h | 9 +++++ Tests/PINProgressiveImageTests.m | 34 ++++++++++++++++++ 5 files changed, 86 insertions(+), 21 deletions(-) diff --git a/Source/Classes/PINProgressiveImage.m b/Source/Classes/PINProgressiveImage.m index 80baec1c..9b8cd62e 100644 --- a/Source/Classes/PINProgressiveImage.m +++ b/Source/Classes/PINProgressiveImage.m @@ -277,13 +277,17 @@ - (NSData *)data { [self.lock lock]; NSData *data = nil; - @try { + if (PINRemoteImageManager.isMemorySafeProgressiveDataEnabled) { + @try { + data = [self.mutableData copy]; + } @catch (NSException *exception) { + // Foundation's page allocator raises NSInvalidArgumentException rather than + // returning nil when it cannot satisfy a large allocation. Callers already + // treat nil as "no data" (see PINRemoteImageDownloadTask). + data = nil; + } + } else { data = [self.mutableData copy]; - } @catch (NSException *exception) { - // Foundation's page allocator raises NSInvalidArgumentException rather than - // returning nil when it cannot satisfy a large allocation. Callers already - // treat nil as "no data" (see PINRemoteImageDownloadTask). - data = nil; } [self.lock unlock]; return data; diff --git a/Source/Classes/PINRemoteImageDownloadTask.m b/Source/Classes/PINRemoteImageDownloadTask.m index 0c589bba..a4491f74 100644 --- a/Source/Classes/PINRemoteImageDownloadTask.m +++ b/Source/Classes/PINRemoteImageDownloadTask.m @@ -312,11 +312,15 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request #endif if (error.code != NSURLErrorCancelled) { - // Decide emptiness and retry WITHOUT materializing the buffer -- on a retry - // the copy was discarded, and the allocation itself can fail under memory - // pressure. -hasData preserves today's semantics exactly: + // Read once so this completion stays on a single path even if the switch flips mid-flight. + BOOL memorySafeData = PINRemoteImageManager.isMemorySafeProgressiveDataEnabled; + // When memory-safe, decide emptiness and retry WITHOUT materializing the buffer -- + // on a retry the copy was discarded, and the allocation itself can fail under memory + // pressure. -hasData preserves the copy path's semantics exactly: // an allocated-but-zero-length buffer is not "empty" (see PINProgressiveImage). - if (error == nil && self.progressImage.hasData == NO) { + NSData *data = memorySafeData ? nil : self.progressImage.data; + BOOL isEmpty = memorySafeData ? (self.progressImage.hasData == NO) : (data == nil); + if (error == nil && isEmpty) { error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain code:PINRemoteImageManagerErrorImageEmpty userInfo:nil]; @@ -341,17 +345,19 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request return; } - // Terminal: no further data can arrive, so hand off the buffer instead of - // copying it. -takeData allocates nothing, so it cannot throw under memory - // pressure -- this is what actually removes the doubling. - NSData *data = [self.progressImage takeData]; - if (error == nil && data == nil) { - // Load-bearing: with -hasData gating the retry decision above, this only - // fires if -progressImage itself is nil, keeping completionHandler from - // ever seeing (nil, response, nil). - error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain - code:PINRemoteImageManagerErrorImageEmpty - userInfo:nil]; + if (memorySafeData) { + // Terminal: no further data can arrive, so hand off the buffer instead of + // copying it. -takeData allocates nothing, so it cannot throw under memory + // pressure -- this is what actually removes the doubling. + data = [self.progressImage takeData]; + if (error == nil && data == nil) { + // Load-bearing: with -hasData gating the retry decision above, this only + // fires if -progressImage itself is nil, keeping completionHandler from + // ever seeing (nil, response, nil). + error = [NSError errorWithDomain:PINRemoteImageManagerErrorDomain + code:PINRemoteImageManagerErrorImageEmpty + userInfo:nil]; + } } completionHandler(data, response, error); diff --git a/Source/Classes/PINRemoteImageManager.m b/Source/Classes/PINRemoteImageManager.m index aa48d0da..9a415a86 100644 --- a/Source/Classes/PINRemoteImageManager.m +++ b/Source/Classes/PINRemoteImageManager.m @@ -9,6 +9,7 @@ #import #import +#import #if !__has_include() #import "PINOperation.h" @@ -168,6 +169,17 @@ @implementation PINRemoteImageManager static PINRemoteImageManager *sharedImageManager = nil; static dispatch_once_t sharedDispatchToken; +static atomic_bool memorySafeProgressiveDataEnabled = false; + ++ (BOOL)isMemorySafeProgressiveDataEnabled +{ + return atomic_load(&memorySafeProgressiveDataEnabled); +} + ++ (void)setMemorySafeProgressiveDataEnabled:(BOOL)enabled +{ + atomic_store(&memorySafeProgressiveDataEnabled, enabled); +} + (instancetype)sharedImageManager { diff --git a/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h b/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h index 72853e67..dec391a4 100644 --- a/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h +++ b/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h @@ -224,6 +224,15 @@ typedef void(^PINRemoteImageManagerMetrics)(NSURL * __nonnull url, NSURLSession */ + (void)setSharedImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration; +/** + Opt-in, process-wide switch for memory-safe handling of downloaded image data. Defaults to NO. + + When YES, a finished download hands its accumulated buffer to the completion path instead of copying it, and + -[PINProgressiveImage data] returns nil instead of raising when Foundation fails to allocate the copy under + memory pressure. Set it once, before any downloads start; each download reads it once when it completes. + */ +@property (class, atomic, assign, getter=isMemorySafeProgressiveDataEnabled) BOOL memorySafeProgressiveDataEnabled; + /** The result of this method is assigned to self.cache in init. If you wish to provide a customized cache to the manager you can subclass PINRemoteImageManager and return a custom object, implementing PINRemoteImageCaching protocol from this method. Same effect could be achieved by using initWithSessionConfiguration:alternativeRepresentationProvider:imageCache: initializer. diff --git a/Tests/PINProgressiveImageTests.m b/Tests/PINProgressiveImageTests.m index a5b20f01..6db078b0 100644 --- a/Tests/PINProgressiveImageTests.m +++ b/Tests/PINProgressiveImageTests.m @@ -14,6 +14,7 @@ #import #import +#import #import "PINProgressiveImage+Private.h" /// Dynamically subclasses `object`'s class so that `-copy`/`-copyWithZone:` throw, @@ -45,10 +46,25 @@ static void PINTestMakeCopyThrowOnInstance(NSObject *object) @interface PINProgressiveImageTests : XCTestCase +@property (nonatomic, assign) BOOL originalMemorySafeProgressiveDataEnabled; + @end @implementation PINProgressiveImageTests +- (void)setUp +{ + [super setUp]; + self.originalMemorySafeProgressiveDataEnabled = PINRemoteImageManager.isMemorySafeProgressiveDataEnabled; + PINRemoteImageManager.memorySafeProgressiveDataEnabled = YES; +} + +- (void)tearDown +{ + PINRemoteImageManager.memorySafeProgressiveDataEnabled = self.originalMemorySafeProgressiveDataEnabled; + [super tearDown]; +} + - (PINProgressiveImage *)freshProgressiveImage { NSURLSessionDataTask *dataTask = [[NSURLSession sharedSession] dataTaskWithURL:[NSURL URLWithString:@"https://example.com/image.jpg"]]; @@ -159,4 +175,22 @@ - (void)testDataReturnsNilInsteadOfThrowingWhenCopyFails XCTAssertTrue(progressiveImage.hasData, @"hasData does not copy, so it must still reflect the buffer's presence even though -data failed."); } +- (void)testDataStillThrowsWhenCopyFailsAndMemorySafeDataIsDisabled +{ + PINRemoteImageManager.memorySafeProgressiveDataEnabled = NO; + + PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + NSData *chunk = [@"payload" dataUsingEncoding:NSUTF8StringEncoding]; + [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; + + NSMutableData *mutableData = [progressiveImage valueForKey:@"mutableData"]; + XCTAssertNotNil(mutableData); + PINTestMakeCopyThrowOnInstance(mutableData); + + // The legacy path leaves -data's lock held after the throw, and -dealloc takes that lock, + // so the object is intentionally leaked to keep the test from deadlocking. + CFBridgingRetain(progressiveImage); + XCTAssertThrowsSpecificNamed(progressiveImage.data, NSException, NSInvalidArgumentException); +} + @end From c2da45115cc52e155b2304c09ae0a02164d19441 Mon Sep 17 00:00:00 2001 From: Vitor Navarro Date: Fri, 6 Nov 2026 10:42:09 -0300 Subject: [PATCH 3/4] Switch to use PINREmoteImageManagerConfiguration # 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 --- Source/Classes/PINProgressiveImage.m | 2 +- Source/Classes/PINRemoteImageDownloadTask.m | 3 ++- .../Classes/PINRemoteImageManager+Private.h | 1 + Source/Classes/PINRemoteImageManager.m | 14 ++---------- .../PINRemoteImageManagerConfiguration.m | 1 + .../PINRemoteImage/PINProgressiveImage.h | 2 ++ .../PINRemoteImage/PINRemoteImageManager.h | 10 --------- .../PINRemoteImageManagerConfiguration.h | 9 ++++++++ Tests/PINProgressiveImageTests.m | 22 ++++--------------- 9 files changed, 22 insertions(+), 42 deletions(-) diff --git a/Source/Classes/PINProgressiveImage.m b/Source/Classes/PINProgressiveImage.m index 9b8cd62e..5e147d8e 100644 --- a/Source/Classes/PINProgressiveImage.m +++ b/Source/Classes/PINProgressiveImage.m @@ -277,7 +277,7 @@ - (NSData *)data { [self.lock lock]; NSData *data = nil; - if (PINRemoteImageManager.isMemorySafeProgressiveDataEnabled) { + if (self.memorySafeDataEnabled) { @try { data = [self.mutableData copy]; } @catch (NSException *exception) { diff --git a/Source/Classes/PINRemoteImageDownloadTask.m b/Source/Classes/PINRemoteImageDownloadTask.m index a4491f74..f1b6419a 100644 --- a/Source/Classes/PINRemoteImageDownloadTask.m +++ b/Source/Classes/PINRemoteImageDownloadTask.m @@ -313,7 +313,7 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request if (error.code != NSURLErrorCancelled) { // Read once so this completion stays on a single path even if the switch flips mid-flight. - BOOL memorySafeData = PINRemoteImageManager.isMemorySafeProgressiveDataEnabled; + BOOL memorySafeData = self.manager.memorySafeProgressiveDataEnabled; // When memory-safe, decide emptiness and retry WITHOUT materializing the buffer -- // on a retry the copy was discarded, and the allocation itself can fail under memory // pressure. -hasData preserves the copy path's semantics exactly: @@ -364,6 +364,7 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request } }]; }]]; + self->_progressImage.memorySafeDataEnabled = self.manager.memorySafeProgressiveDataEnabled; }]; } diff --git a/Source/Classes/PINRemoteImageManager+Private.h b/Source/Classes/PINRemoteImageManager+Private.h index 39550e32..120eb6d7 100644 --- a/Source/Classes/PINRemoteImageManager+Private.h +++ b/Source/Classes/PINRemoteImageManager+Private.h @@ -23,6 +23,7 @@ typedef void (^PINRemoteImageManagerDataCompletion)(NSData *data, NSURLResponse @property (nonatomic, readonly) NSArray *progressThresholds; @property (nonatomic, readonly) NSTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, readonly) BOOL shouldBlurProgressive; +@property (nonatomic, readonly) BOOL memorySafeProgressiveDataEnabled; @property (nonatomic, readonly) CGSize maxProgressiveRenderSize; @end diff --git a/Source/Classes/PINRemoteImageManager.m b/Source/Classes/PINRemoteImageManager.m index 9a415a86..5d5e597a 100644 --- a/Source/Classes/PINRemoteImageManager.m +++ b/Source/Classes/PINRemoteImageManager.m @@ -141,6 +141,7 @@ @interface PINRemoteImageManager () @property (nonatomic, strong) NSMapTable *UUIDToTask; @property (nonatomic, strong) NSArray *progressThresholds; @property (nonatomic, assign) BOOL shouldBlurProgressive; +@property (nonatomic, assign) BOOL memorySafeProgressiveDataEnabled; @property (nonatomic, assign) CGSize maxProgressiveRenderSize; @property (nonatomic, assign) NSTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, strong) dispatch_queue_t callbackQueue; @@ -169,18 +170,6 @@ @implementation PINRemoteImageManager static PINRemoteImageManager *sharedImageManager = nil; static dispatch_once_t sharedDispatchToken; -static atomic_bool memorySafeProgressiveDataEnabled = false; - -+ (BOOL)isMemorySafeProgressiveDataEnabled -{ - return atomic_load(&memorySafeProgressiveDataEnabled); -} - -+ (void)setMemorySafeProgressiveDataEnabled:(BOOL)enabled -{ - atomic_store(&memorySafeProgressiveDataEnabled, enabled); -} - + (instancetype)sharedImageManager { dispatch_once(&sharedDispatchToken, ^{ @@ -274,6 +263,7 @@ -(nonnull instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration * _shouldUpgradeLowQualityImages = configuration.shouldUpgradeLowQualityImages; _shouldBlurProgressive = configuration.shouldBlurProgressive; _maxProgressiveRenderSize = configuration.maxProgressiveRenderSize; + _memorySafeProgressiveDataEnabled = configuration.memorySafeProgressiveDataEnabled; self.tasks = [[NSMutableDictionary alloc] init]; self.canceledTasks = [[NSHashTable alloc] initWithOptions:NSHashTableWeakMemory capacity:5]; self.UUIDToTask = [NSMapTable weakToWeakObjectsMapTable]; diff --git a/Source/Classes/PINRemoteImageManagerConfiguration.m b/Source/Classes/PINRemoteImageManagerConfiguration.m index 88896fd0..6f3e80d3 100644 --- a/Source/Classes/PINRemoteImageManagerConfiguration.m +++ b/Source/Classes/PINRemoteImageManagerConfiguration.m @@ -21,6 +21,7 @@ - (nonnull instancetype)init { _highQualityBPSThreshold = 500000; _lowQualityBPSThreshold = 50000; // approximately edge speed _shouldUpgradeLowQualityImages = NO; + _memorySafeProgressiveDataEnabled = NO; } return self; } diff --git a/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h b/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h index da3e38c9..b9128c83 100644 --- a/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h +++ b/Source/Classes/include/PINRemoteImage/PINProgressiveImage.h @@ -25,6 +25,8 @@ @property (atomic, assign) CFTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, strong, readonly, nonnull) NSURLSessionDataTask * dataTask; @property (nonatomic, readonly) CFTimeInterval estimatedRemainingTime; +/** When YES, -data returns nil instead of raising if copying the buffer fails. Defaults to NO. */ +@property (atomic, assign, getter=isMemorySafeDataEnabled) BOOL memorySafeDataEnabled; - (nonnull instancetype)init NS_UNAVAILABLE; - (nonnull instancetype)initWithDataTask:(nonnull NSURLSessionDataTask *)dataTask; diff --git a/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h b/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h index dec391a4..332e590c 100644 --- a/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h +++ b/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h @@ -224,16 +224,6 @@ typedef void(^PINRemoteImageManagerMetrics)(NSURL * __nonnull url, NSURLSession */ + (void)setSharedImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration; -/** - Opt-in, process-wide switch for memory-safe handling of downloaded image data. Defaults to NO. - - When YES, a finished download hands its accumulated buffer to the completion path instead of copying it, and - -[PINProgressiveImage data] returns nil instead of raising when Foundation fails to allocate the copy under - memory pressure. Set it once, before any downloads start; each download reads it once when it completes. - */ -@property (class, atomic, assign, getter=isMemorySafeProgressiveDataEnabled) BOOL memorySafeProgressiveDataEnabled; - - /** The result of this method is assigned to self.cache in init. If you wish to provide a customized cache to the manager you can subclass PINRemoteImageManager and return a custom object, implementing PINRemoteImageCaching protocol from this method. Same effect could be achieved by using initWithSessionConfiguration:alternativeRepresentationProvider:imageCache: initializer. @deprecated Use the class method +defaultImageCache diff --git a/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h b/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h index 20b0746c..1a824031 100644 --- a/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h +++ b/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h @@ -43,4 +43,13 @@ /** Whether high quality images should be downloaded when a low quality image is cached if network connectivity has improved. */ @property (nonatomic, readwrite, assign) BOOL shouldUpgradeLowQualityImages; +/** + Opt-in switch for memory-safe handling of downloaded image data. Defaults to NO. + + When YES, a finished download hands its accumulated buffer to the completion path instead of copying it, and + -[PINProgressiveImage data] returns nil instead of raising when Foundation fails to allocate the copy under + memory pressure. + */ +@property (nonatomic, readwrite, assign, getter=isMemorySafeProgressiveDataEnabled) BOOL memorySafeProgressiveDataEnabled; + @end diff --git a/Tests/PINProgressiveImageTests.m b/Tests/PINProgressiveImageTests.m index 6db078b0..93729c4d 100644 --- a/Tests/PINProgressiveImageTests.m +++ b/Tests/PINProgressiveImageTests.m @@ -46,29 +46,16 @@ static void PINTestMakeCopyThrowOnInstance(NSObject *object) @interface PINProgressiveImageTests : XCTestCase -@property (nonatomic, assign) BOOL originalMemorySafeProgressiveDataEnabled; - @end @implementation PINProgressiveImageTests -- (void)setUp -{ - [super setUp]; - self.originalMemorySafeProgressiveDataEnabled = PINRemoteImageManager.isMemorySafeProgressiveDataEnabled; - PINRemoteImageManager.memorySafeProgressiveDataEnabled = YES; -} - -- (void)tearDown -{ - PINRemoteImageManager.memorySafeProgressiveDataEnabled = self.originalMemorySafeProgressiveDataEnabled; - [super tearDown]; -} - - (PINProgressiveImage *)freshProgressiveImage { NSURLSessionDataTask *dataTask = [[NSURLSession sharedSession] dataTaskWithURL:[NSURL URLWithString:@"https://example.com/image.jpg"]]; - return [[PINProgressiveImage alloc] initWithDataTask:dataTask]; + PINProgressiveImage *progressiveImage = [[PINProgressiveImage alloc] initWithDataTask:dataTask]; + progressiveImage.memorySafeDataEnabled = YES; + return progressiveImage; } #pragma mark - hasData @@ -177,9 +164,8 @@ - (void)testDataReturnsNilInsteadOfThrowingWhenCopyFails - (void)testDataStillThrowsWhenCopyFailsAndMemorySafeDataIsDisabled { - PINRemoteImageManager.memorySafeProgressiveDataEnabled = NO; - PINProgressiveImage *progressiveImage = [self freshProgressiveImage]; + progressiveImage.memorySafeDataEnabled = NO; NSData *chunk = [@"payload" dataUsingEncoding:NSUTF8StringEncoding]; [progressiveImage updateProgressiveImageWithData:chunk expectedNumberOfBytes:(int64_t)chunk.length isResume:NO]; From 338f1c00fa96cd2bc71a65d1a3b7f0ed9309fe4b Mon Sep 17 00:00:00 2001 From: Vitor Navarro Date: Fri, 6 Nov 2026 12:01:57 -0300 Subject: [PATCH 4/4] Address Andy review --- Source/Classes/PINProgressiveImage+Private.h | 2 +- Source/Classes/PINRemoteImageDownloadTask.m | 4 ++-- Source/Classes/PINRemoteImageManager+Private.h | 2 +- Source/Classes/PINRemoteImageManager.m | 6 +++--- Source/Classes/PINRemoteImageManagerConfiguration.m | 2 +- .../Classes/include/PINRemoteImage/PINRemoteImageManager.h | 1 + .../PINRemoteImage/PINRemoteImageManagerConfiguration.h | 2 +- Tests/PINRemoteImageTests.m | 2 +- 8 files changed, 11 insertions(+), 10 deletions(-) diff --git a/Source/Classes/PINProgressiveImage+Private.h b/Source/Classes/PINProgressiveImage+Private.h index c4580ffc..b56ba1d8 100644 --- a/Source/Classes/PINProgressiveImage+Private.h +++ b/Source/Classes/PINProgressiveImage+Private.h @@ -2,7 +2,7 @@ // PINProgressiveImage+Private.h // PINRemoteImage // -// Created by Garrett Moon on 2/9/15. +// Created by Vitor Navarro on 30/7/26. // // diff --git a/Source/Classes/PINRemoteImageDownloadTask.m b/Source/Classes/PINRemoteImageDownloadTask.m index f1b6419a..1ae9bb22 100644 --- a/Source/Classes/PINRemoteImageDownloadTask.m +++ b/Source/Classes/PINRemoteImageDownloadTask.m @@ -313,7 +313,7 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request if (error.code != NSURLErrorCancelled) { // Read once so this completion stays on a single path even if the switch flips mid-flight. - BOOL memorySafeData = self.manager.memorySafeProgressiveDataEnabled; + BOOL memorySafeData = self.manager.isMemorySafeProgressiveDataEnabled; // When memory-safe, decide emptiness and retry WITHOUT materializing the buffer -- // on a retry the copy was discarded, and the allocation itself can fail under memory // pressure. -hasData preserves the copy path's semantics exactly: @@ -364,7 +364,7 @@ - (void)scheduleDownloadWithRequest:(NSURLRequest *)request } }]; }]]; - self->_progressImage.memorySafeDataEnabled = self.manager.memorySafeProgressiveDataEnabled; + self->_progressImage.memorySafeDataEnabled = self.manager.isMemorySafeProgressiveDataEnabled; }]; } diff --git a/Source/Classes/PINRemoteImageManager+Private.h b/Source/Classes/PINRemoteImageManager+Private.h index 120eb6d7..0cd7b911 100644 --- a/Source/Classes/PINRemoteImageManager+Private.h +++ b/Source/Classes/PINRemoteImageManager+Private.h @@ -23,7 +23,7 @@ typedef void (^PINRemoteImageManagerDataCompletion)(NSData *data, NSURLResponse @property (nonatomic, readonly) NSArray *progressThresholds; @property (nonatomic, readonly) NSTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, readonly) BOOL shouldBlurProgressive; -@property (nonatomic, readonly) BOOL memorySafeProgressiveDataEnabled; +@property (nonatomic, readonly) BOOL isMemorySafeProgressiveDataEnabled; @property (nonatomic, readonly) CGSize maxProgressiveRenderSize; @end diff --git a/Source/Classes/PINRemoteImageManager.m b/Source/Classes/PINRemoteImageManager.m index 5d5e597a..13e7abca 100644 --- a/Source/Classes/PINRemoteImageManager.m +++ b/Source/Classes/PINRemoteImageManager.m @@ -9,7 +9,6 @@ #import #import -#import #if !__has_include() #import "PINOperation.h" @@ -141,7 +140,7 @@ @interface PINRemoteImageManager () @property (nonatomic, strong) NSMapTable *UUIDToTask; @property (nonatomic, strong) NSArray *progressThresholds; @property (nonatomic, assign) BOOL shouldBlurProgressive; -@property (nonatomic, assign) BOOL memorySafeProgressiveDataEnabled; +@property (nonatomic, assign) BOOL isMemorySafeProgressiveDataEnabled; @property (nonatomic, assign) CGSize maxProgressiveRenderSize; @property (nonatomic, assign) NSTimeInterval estimatedRemainingTimeThreshold; @property (nonatomic, strong) dispatch_queue_t callbackQueue; @@ -170,6 +169,7 @@ @implementation PINRemoteImageManager static PINRemoteImageManager *sharedImageManager = nil; static dispatch_once_t sharedDispatchToken; + + (instancetype)sharedImageManager { dispatch_once(&sharedDispatchToken, ^{ @@ -263,7 +263,7 @@ -(nonnull instancetype)initWithSessionConfiguration:(NSURLSessionConfiguration * _shouldUpgradeLowQualityImages = configuration.shouldUpgradeLowQualityImages; _shouldBlurProgressive = configuration.shouldBlurProgressive; _maxProgressiveRenderSize = configuration.maxProgressiveRenderSize; - _memorySafeProgressiveDataEnabled = configuration.memorySafeProgressiveDataEnabled; + _isMemorySafeProgressiveDataEnabled = configuration.isMemorySafeProgressiveDataEnabled; self.tasks = [[NSMutableDictionary alloc] init]; self.canceledTasks = [[NSHashTable alloc] initWithOptions:NSHashTableWeakMemory capacity:5]; self.UUIDToTask = [NSMapTable weakToWeakObjectsMapTable]; diff --git a/Source/Classes/PINRemoteImageManagerConfiguration.m b/Source/Classes/PINRemoteImageManagerConfiguration.m index 6f3e80d3..5289add1 100644 --- a/Source/Classes/PINRemoteImageManagerConfiguration.m +++ b/Source/Classes/PINRemoteImageManagerConfiguration.m @@ -21,7 +21,7 @@ - (nonnull instancetype)init { _highQualityBPSThreshold = 500000; _lowQualityBPSThreshold = 50000; // approximately edge speed _shouldUpgradeLowQualityImages = NO; - _memorySafeProgressiveDataEnabled = NO; + _isMemorySafeProgressiveDataEnabled = NO; } return self; } diff --git a/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h b/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h index 332e590c..72853e67 100644 --- a/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h +++ b/Source/Classes/include/PINRemoteImage/PINRemoteImageManager.h @@ -224,6 +224,7 @@ typedef void(^PINRemoteImageManagerMetrics)(NSURL * __nonnull url, NSURLSession */ + (void)setSharedImageManagerWithConfiguration:(nullable NSURLSessionConfiguration *)configuration; + /** The result of this method is assigned to self.cache in init. If you wish to provide a customized cache to the manager you can subclass PINRemoteImageManager and return a custom object, implementing PINRemoteImageCaching protocol from this method. Same effect could be achieved by using initWithSessionConfiguration:alternativeRepresentationProvider:imageCache: initializer. @deprecated Use the class method +defaultImageCache diff --git a/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h b/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h index 1a824031..528bbec1 100644 --- a/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h +++ b/Source/Classes/include/PINRemoteImage/PINRemoteImageManagerConfiguration.h @@ -50,6 +50,6 @@ -[PINProgressiveImage data] returns nil instead of raising when Foundation fails to allocate the copy under memory pressure. */ -@property (nonatomic, readwrite, assign, getter=isMemorySafeProgressiveDataEnabled) BOOL memorySafeProgressiveDataEnabled; +@property (nonatomic, readwrite, assign) BOOL isMemorySafeProgressiveDataEnabled; @end diff --git a/Tests/PINRemoteImageTests.m b/Tests/PINRemoteImageTests.m index b90494cd..1818fdc6 100644 --- a/Tests/PINRemoteImageTests.m +++ b/Tests/PINRemoteImageTests.m @@ -205,7 +205,7 @@ - (NSURL *)grayscalePNGURL - (NSURL *)veryLongURL { - return [NSURL URLWithString:@"https://loremflickr.com/200/301?longarg=helloMomHowAreYouDoing.IamFineJustMovedToLiveWithANiceChapWeTravelTogetherInHisBlueBoxThroughSpaceAndTimeMaybeYouveMetHimAlready.YesterdayWeMetACultureOfPeopleWithTentaclesWhoSingWithAVeryCelestialVoice.SoGood.SeeYouSoon.MaybeYesterday.WhoKnows.XOXO"]; + return [NSURL URLWithString:@"https://via.placeholder.com/200x301?text=helloMomHowAreYouDoing.IamFineJustMovedToLiveWithANiceChapWeTravelTogetherInHisBlueBoxThroughSpaceAndTimeMaybeYouveMetHimAlready.YesterdayWeMetACultureOfPeopleWithTentaclesWhoSingWithAVeryCelestialVoice.SoGood.SeeYouSoon.MaybeYesterday.WhoKnows.XOXO"]; } - (NSURL *)progressiveURL