From 63c0794ad83706eebc91f29192a26183207d67c9 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Wed, 15 May 2019 20:26:39 +0800 Subject: [PATCH 1/3] Fix that SDAnimatedImageView initWithImage will skip the initialize logic and crash --- SDWebImage/SDAnimatedImageView.m | 11 +++++++++++ Tests/Tests/SDAnimatedImageTest.m | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/SDWebImage/SDAnimatedImageView.m b/SDWebImage/SDAnimatedImageView.m index 7e940828..eac7a6fd 100644 --- a/SDWebImage/SDAnimatedImageView.m +++ b/SDWebImage/SDAnimatedImageView.m @@ -62,6 +62,7 @@ static NSUInteger SDDeviceFreeMemory() { #else @property (nonatomic, strong) CADisplayLink *displayLink; #endif +@property (nonatomic, assign) BOOL hasInitialized; @end @@ -123,6 +124,11 @@ static NSUInteger SDDeviceFreeMemory() { - (void)commonInit { + if (self.hasInitialized) { + return; + } + self.hasInitialized = YES; + self.shouldCustomLoopCount = NO; self.shouldIncrementalLoad = YES; self.lock = dispatch_semaphore_create(1); @@ -183,6 +189,11 @@ static NSUInteger SDDeviceFreeMemory() { return; } + // Check has initlized + if (!self.hasInitialized) { + [self commonInit]; + } + // Check Progressive rendering [self updateIsProgressiveWithImage:image]; diff --git a/Tests/Tests/SDAnimatedImageTest.m b/Tests/Tests/SDAnimatedImageTest.m index 1605998c..142546ca 100644 --- a/Tests/Tests/SDAnimatedImageTest.m +++ b/Tests/Tests/SDAnimatedImageTest.m @@ -156,6 +156,20 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun #endif } +- (void)test12AnimatedImageViewInitWithImage { + // Test that -[SDAnimatedImageView initWithImage:] this convenience initializer not crash + SDAnimatedImage *image = [SDAnimatedImage imageWithData:[self testAPNGPData]]; + SDAnimatedImageView *imageView; +#if SD_UIKIT + imageView = [[SDAnimatedImageView alloc] initWithImage:image]; +#else + if (@available(macOS 10.12, *)) { + imageView = [SDAnimatedImageView imageViewWithImage:image]; + } +#endif + expect(imageView.image).equal(image); +} + - (void)test20AnimatedImageViewRendering { XCTestExpectation *expectation = [self expectationWithDescription:@"test SDAnimatedImageView rendering"]; SDAnimatedImageView *imageView = [[SDAnimatedImageView alloc] init]; From 1d5b411f3cdff6b34be7a193fe60154da56be334 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 16 May 2019 00:34:52 +0800 Subject: [PATCH 2/3] Using the lazy property for `lock` and `runLoopMode` to ensure this will not cause crash and logic issue. Without need of checking on `setImage:` --- SDWebImage/SDAnimatedImageView.m | 62 +++++++++++++++++++++---------- Tests/Tests/SDAnimatedImageTest.m | 2 +- 2 files changed, 43 insertions(+), 21 deletions(-) diff --git a/SDWebImage/SDAnimatedImageView.m b/SDWebImage/SDAnimatedImageView.m index eac7a6fd..f81fd8b5 100644 --- a/SDWebImage/SDAnimatedImageView.m +++ b/SDWebImage/SDAnimatedImageView.m @@ -40,7 +40,10 @@ static NSUInteger SDDeviceFreeMemory() { return vm_stat.free_count * page_size; } -@interface SDAnimatedImageView () +@interface SDAnimatedImageView () { + NSRunLoopMode _runLoopMode; + BOOL _initFinished; // Extra flag to mark the `commonInit` is called +} @property (nonatomic, strong, readwrite) UIImage *currentFrame; @property (nonatomic, assign, readwrite) NSUInteger currentFrameIndex; @@ -62,7 +65,6 @@ static NSUInteger SDDeviceFreeMemory() { #else @property (nonatomic, strong) CADisplayLink *displayLink; #endif -@property (nonatomic, assign) BOOL hasInitialized; @end @@ -124,14 +126,10 @@ static NSUInteger SDDeviceFreeMemory() { - (void)commonInit { - if (self.hasInitialized) { - return; - } - self.hasInitialized = YES; - + // Pay attention that UIKit's `initWithImage:` will trigger a `setImage:` during initialization before this `commonInit`. + // So the properties which rely on this order, should using lazy-evaluation or do extra check in `setImage:`. self.shouldCustomLoopCount = NO; self.shouldIncrementalLoad = YES; - self.lock = dispatch_semaphore_create(1); #if SD_MAC self.wantsLayer = YES; // Default value from `NSImageView` @@ -140,9 +138,10 @@ static NSUInteger SDDeviceFreeMemory() { self.imageAlignment = NSImageAlignCenter; #endif #if SD_UIKIT - self.runLoopMode = [[self class] defaultRunLoopMode]; [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didReceiveMemoryWarning:) name:UIApplicationDidReceiveMemoryWarningNotification object:nil]; #endif + // Mark commonInit finished + _initFinished = YES; } - (void)resetAnimatedImage @@ -189,11 +188,6 @@ static NSUInteger SDDeviceFreeMemory() { return; } - // Check has initlized - if (!self.hasInitialized) { - [self commonInit]; - } - // Check Progressive rendering [self updateIsProgressiveWithImage:image]; @@ -251,17 +245,38 @@ static NSUInteger SDDeviceFreeMemory() { } #if SD_UIKIT -- (void)setRunLoopMode:(NSString *)runLoopMode +- (void)setRunLoopMode:(NSRunLoopMode)runLoopMode { - if (![@[NSDefaultRunLoopMode, NSRunLoopCommonModes] containsObject:runLoopMode]) { - NSAssert(NO, @"Invalid run loop mode: %@", runLoopMode); - _runLoopMode = [[self class] defaultRunLoopMode]; - } else { - _runLoopMode = runLoopMode; + if ([_runLoopMode isEqualToString:runLoopMode]) { + return; } + if (_displayLink) { + if (_runLoopMode) { + [_displayLink removeFromRunLoop:[NSRunLoop mainRunLoop] forMode:_runLoopMode]; + } + if (runLoopMode) { + [_displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:runLoopMode]; + } + } + _runLoopMode = [runLoopMode copy]; +} + +- (NSRunLoopMode)runLoopMode +{ + if (!_runLoopMode) { + _runLoopMode = [[self class] defaultRunLoopMode]; + } + return _runLoopMode; } #endif +- (BOOL)shouldIncrementalLoad { + if (!_initFinished) { + return YES; // Defaults to YES + } + return _initFinished; +} + #pragma mark - Private - (NSOperationQueue *)fetchQueue { @@ -280,6 +295,13 @@ static NSUInteger SDDeviceFreeMemory() { return _frameBuffer; } +- (dispatch_semaphore_t)lock { + if (!_lock) { + _lock = dispatch_semaphore_create(1); + } + return _lock; +} + #if SD_MAC - (CVDisplayLinkRef)displayLink { diff --git a/Tests/Tests/SDAnimatedImageTest.m b/Tests/Tests/SDAnimatedImageTest.m index 142546ca..94f8d240 100644 --- a/Tests/Tests/SDAnimatedImageTest.m +++ b/Tests/Tests/SDAnimatedImageTest.m @@ -156,7 +156,7 @@ static const NSUInteger kTestGIFFrameCount = 5; // local TestImage.gif loop coun #endif } -- (void)test12AnimatedImageViewInitWithImage { +- (void)test13AnimatedImageViewInitWithImage { // Test that -[SDAnimatedImageView initWithImage:] this convenience initializer not crash SDAnimatedImage *image = [SDAnimatedImage imageWithData:[self testAPNGPData]]; SDAnimatedImageView *imageView; From dbc412baf43c65462b5b40daf8bd6b4847547f88 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 16 May 2019 12:52:16 +0800 Subject: [PATCH 3/3] A little optimization code for runLooMode setter --- SDWebImage/SDAnimatedImageView.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/SDWebImage/SDAnimatedImageView.m b/SDWebImage/SDAnimatedImageView.m index f81fd8b5..37c977da 100644 --- a/SDWebImage/SDAnimatedImageView.m +++ b/SDWebImage/SDAnimatedImageView.m @@ -247,14 +247,14 @@ static NSUInteger SDDeviceFreeMemory() { #if SD_UIKIT - (void)setRunLoopMode:(NSRunLoopMode)runLoopMode { - if ([_runLoopMode isEqualToString:runLoopMode]) { + if ([_runLoopMode isEqual:runLoopMode]) { return; } if (_displayLink) { if (_runLoopMode) { [_displayLink removeFromRunLoop:[NSRunLoop mainRunLoop] forMode:_runLoopMode]; } - if (runLoopMode) { + if (runLoopMode.length > 0) { [_displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:runLoopMode]; } }