From 492d0d7ad61312e0bf9cfeef1835f34185174b6a Mon Sep 17 00:00:00 2001 From: Wendy Date: Thu, 28 Apr 2016 14:52:31 -0700 Subject: [PATCH 1/2] [ASNetworkImageNode] Fix threading issue in current image quality Summary: Because we trampoline to main when setting _currentImageQuality, there would be situations where displayWillStart was called, we get a cached image and set currentImageQuality to 1.0, and then a background thread calling setDefaultImage would enqueue an operation that sets currentImageQuality to 0 and overwrites the correct value --- AsyncDisplayKit/ASNetworkImageNode.mm | 28 ++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/AsyncDisplayKit/ASNetworkImageNode.mm b/AsyncDisplayKit/ASNetworkImageNode.mm index fa42a3c3cc..8ff2be7cf1 100755 --- a/AsyncDisplayKit/ASNetworkImageNode.mm +++ b/AsyncDisplayKit/ASNetworkImageNode.mm @@ -124,9 +124,15 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; if (reset || _URL == nil) { self.image = _defaultImage; - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 1.0; - }); + if (_URL == nil) { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 1.0; + }); + } else { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 0.0; + }); + } } if (self.interfaceState & ASInterfaceStateFetchData) { @@ -151,9 +157,15 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _defaultImage = defaultImage; if (!_imageLoaded) { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 0.0; - }); + if (_URL == nil) { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 1.0; + }); + } else { + ASPerformBlockOnMainThread(^{ + self.currentImageQuality = 0.0; + }); + } _lock.unlock(); // Locking: it is important to release _lock before entering setImage:, as it needs to release the lock before -invalidateCalculatedLayout. // If we continue to hold the lock here, it will still be locked until the next unlock() call, causing a possible deadlock with @@ -229,7 +241,9 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; if (result) { self.image = result; _imageLoaded = YES; - _currentImageQuality = 1.0; + dispatch_async(dispatch_get_main_queue(), ^{ + _currentImageQuality = 1.0; + }); } } } From fd4d18259ea55f0c07d5acd17b7a60b12ae9efa8 Mon Sep 17 00:00:00 2001 From: Wendy Date: Fri, 29 Apr 2016 22:30:56 -0700 Subject: [PATCH 2/2] Use dispatch_async for setting currentImageQuality to ensure current order --- AsyncDisplayKit/ASNetworkImageNode.mm | 51 ++++++++++++++------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/AsyncDisplayKit/ASNetworkImageNode.mm b/AsyncDisplayKit/ASNetworkImageNode.mm index 8ff2be7cf1..27a677fe61 100755 --- a/AsyncDisplayKit/ASNetworkImageNode.mm +++ b/AsyncDisplayKit/ASNetworkImageNode.mm @@ -122,17 +122,15 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _URL = URL; - if (reset || _URL == nil) { + BOOL hasURL = _URL == nil; + if (reset || hasURL) { self.image = _defaultImage; - if (_URL == nil) { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 1.0; - }); - } else { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 0.0; - }); - } + /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, + so always use a dispatch_async to ensure that we queue the operations in the correct order. + (see comment in displayDidFinish) */ + dispatch_async(dispatch_get_main_queue(), ^{ + self.currentImageQuality = hasURL ? 0.0 : 1.0; + }); } if (self.interfaceState & ASInterfaceStateFetchData) { @@ -157,15 +155,13 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; _defaultImage = defaultImage; if (!_imageLoaded) { - if (_URL == nil) { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 1.0; - }); - } else { - ASPerformBlockOnMainThread(^{ - self.currentImageQuality = 0.0; - }); - } + BOOL hasURL = _URL == nil; + /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, + so always use a dispatch_async to ensure that we queue the operations in the correct order. + (see comment in displayDidFinish) */ + dispatch_async(dispatch_get_main_queue(), ^{ + self.currentImageQuality = hasURL ? 0.0 : 1.0; + }); _lock.unlock(); // Locking: it is important to release _lock before entering setImage:, as it needs to release the lock before -invalidateCalculatedLayout. // If we continue to hold the lock here, it will still be locked until the next unlock() call, causing a possible deadlock with @@ -336,7 +332,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; return; } strongSelf.image = progressImage; - ASPerformBlockOnMainThread(^{ + dispatch_async(dispatch_get_main_queue(), ^{ strongSelf->_currentImageQuality = progress; }); }; @@ -361,7 +357,7 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; self.animatedImage = nil; self.image = _defaultImage; _imageLoaded = NO; - ASPerformBlockOnMainThread(^{ + dispatch_async(dispatch_get_main_queue(), ^{ self.currentImageQuality = 0.0; }); } @@ -445,9 +441,14 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } } } - + _imageLoaded = YES; - self.currentImageQuality = 1.0; + /* We want to maintain the order that currentImageQuality is set regardless of the calling thread, + so always use a dispatch_async to ensure that we queue the operations in the correct order. + (see comment in displayDidFinish) */ + dispatch_async(dispatch_get_main_queue(), ^{ + self.currentImageQuality = 1.0; + }); [_delegate imageNode:self didLoadImage:self.image]; }); } @@ -473,7 +474,9 @@ static const CGSize kMinReleaseImageOnBackgroundSize = {20.0, 20.0}; } else { strongSelf.image = [imageContainer asdk_image]; } - strongSelf->_currentImageQuality = 1.0; + dispatch_async(dispatch_get_main_queue(), ^{ + strongSelf->_currentImageQuality = 1.0; + }); } strongSelf->_downloadIdentifier = nil;