From 637c4f3a9ff52eb59623592ce5b20e7c48ee0c25 Mon Sep 17 00:00:00 2001 From: ricky Date: Thu, 7 Jul 2016 09:23:37 -0700 Subject: [PATCH] [ASTraitCollection] Remove traitCollectionContext from ASTraitCollection; add containerWindowSize Passing around a pointer was leading to crashes as the ASVC was the sole owner of the context. There are cases where the VC would dealloc while its subnodes were laying out. This could lead to the subnodes accessing a garbage pointer. --- AsyncDisplayKit/ASViewController.h | 12 ------ AsyncDisplayKit/ASViewController.mm | 39 +++---------------- AsyncDisplayKit/Details/ASEnvironment.h | 25 ++---------- AsyncDisplayKit/Details/ASEnvironment.mm | 16 +------- AsyncDisplayKit/Details/ASTraitCollection.h | 16 +------- AsyncDisplayKit/Details/ASTraitCollection.m | 30 +++++++------- .../Private/ASEnvironmentInternal.mm | 2 +- 7 files changed, 28 insertions(+), 112 deletions(-) diff --git a/AsyncDisplayKit/ASViewController.h b/AsyncDisplayKit/ASViewController.h index 6222af8140..8ef9bc8029 100644 --- a/AsyncDisplayKit/ASViewController.h +++ b/AsyncDisplayKit/ASViewController.h @@ -27,18 +27,6 @@ typedef ASTraitCollection * _Nonnull (^ASDisplayTraitsForTraitWindowSizeBlock)(C @property (nonatomic, strong, readonly) DisplayNodeType node; -/** - * An optional context to pass along with an ASTraitCollection. - * This can be used to pass any internal state to all subnodes via the ASTraitCollection that is not - * included in UITraitCollection. This could range from more fine-tuned size classes to a class of - * constants that is based upon the new trait collection. - * - * Be aware that internally this context is held by a C struct which cannot retain the pointer. Therefore - * ASVC keeps a strong reference to the context to make sure that it stays alive. If you change this value - * it will propagate the change to the subnodes. - */ -@property (nonatomic, strong) id _Nullable traitCollectionContext; - /** * Set this block to customize the ASDisplayTraits returned when the VC transitions to the given traitCollection. */ diff --git a/AsyncDisplayKit/ASViewController.mm b/AsyncDisplayKit/ASViewController.mm index 86417d08aa..deffec60f5 100644 --- a/AsyncDisplayKit/ASViewController.mm +++ b/AsyncDisplayKit/ASViewController.mm @@ -58,17 +58,6 @@ return self; } -- (void)dealloc -{ - if (_traitCollectionContext != nil) { - // The setter will iterate through the VC's subnodes and replace the traitCollectionContext in their ASEnvironmentTraitCollection with nil. - // Since the VC holds the only strong reference to this context and we are in the process of destroying - // the VC, all the references in the subnodes will be unsafe unless we nil them out. More than likely all the subnodes will be dealloc'ed - // as part of the VC being dealloc'ed, but this is just to make extra sure. - self.traitCollectionContext = nil; - } -} - - (void)loadView { ASDisplayNodeAssertTrue(!_node.layerBacked); @@ -199,27 +188,15 @@ ASVisibilityDepthImplementation; #pragma mark - ASEnvironmentTraitCollection -- (void)setTraitCollectionContext:(id)traitCollectionContext -{ - if (_traitCollectionContext != traitCollectionContext) { - // nil out the displayContext in the subnodes so they aren't hanging around with a dealloc'ed pointer don't set - // the new context yet as this will cause ASEnvironmentTraitCollectionIsEqualToASEnvironmentTraitCollection to fail - ASEnvironmentTraitCollectionUpdateDisplayContext(self.node, nil); - - _traitCollectionContext = traitCollectionContext; - } -} - - (ASEnvironmentTraitCollection)environmentTraitCollectionForUITraitCollection:(UITraitCollection *)traitCollection { if (self.overrideDisplayTraitsWithTraitCollection) { ASTraitCollection *asyncTraitCollection = self.overrideDisplayTraitsWithTraitCollection(traitCollection); - self.traitCollectionContext = asyncTraitCollection.traitCollectionContext; return [asyncTraitCollection environmentTraitCollection]; } ASEnvironmentTraitCollection asyncTraitCollection = ASEnvironmentTraitCollectionFromUITraitCollection(traitCollection); - asyncTraitCollection.displayContext = self.traitCollectionContext; + asyncTraitCollection.containerWindowSize = self.view.frame.size; return asyncTraitCollection; } @@ -227,9 +204,9 @@ ASVisibilityDepthImplementation; { if (self.overrideDisplayTraitsWithWindowSize) { ASTraitCollection *traitCollection = self.overrideDisplayTraitsWithWindowSize(windowSize); - self.traitCollectionContext = traitCollection.traitCollectionContext; return [traitCollection environmentTraitCollection]; } + self.node.environmentTraitCollection.containerWindowSize = windowSize; return self.node.environmentTraitCollection; } @@ -255,17 +232,13 @@ ASVisibilityDepthImplementation; [super traitCollectionDidChange:previousTraitCollection]; ASEnvironmentTraitCollection environmentTraitCollection = [self environmentTraitCollectionForUITraitCollection:self.traitCollection]; + environmentTraitCollection.containerWindowSize = self.view.bounds.size; [self progagateNewEnvironmentTraitCollection:environmentTraitCollection]; } -- (void)willTransitionToTraitCollection:(UITraitCollection *)newCollection withTransitionCoordinator:(id)coordinator -{ - [super willTransitionToTraitCollection:newCollection withTransitionCoordinator:coordinator]; - - ASEnvironmentTraitCollection environmentTraitCollection = [self environmentTraitCollectionForUITraitCollection:newCollection]; - [self progagateNewEnvironmentTraitCollection:environmentTraitCollection]; -} - +// Note: We don't override willTransitionToTraitCollection:withTransitionCoordinator: because viewWillTransitionToSize:withTransitionCoordinator: will also called +// called in all these cases. However, there are cases where viewWillTransitionToSize:withTransitionCoordinator: but willTransitionToTraitCollection:withTransitionCoordinator: +// is not. - (void)viewWillTransitionToSize:(CGSize)size withTransitionCoordinator:(id)coordinator { [super viewWillTransitionToSize:size withTransitionCoordinator:coordinator]; diff --git a/AsyncDisplayKit/Details/ASEnvironment.h b/AsyncDisplayKit/Details/ASEnvironment.h index dd95c2e107..de9902a68a 100644 --- a/AsyncDisplayKit/Details/ASEnvironment.h +++ b/AsyncDisplayKit/Details/ASEnvironment.h @@ -69,25 +69,9 @@ typedef struct ASEnvironmentTraitCollection { UIUserInterfaceIdiom userInterfaceIdiom; UIUserInterfaceSizeClass verticalSizeClass; UIForceTouchCapability forceTouchCapability; - - // WARNING: - // This pointer is in a C struct and therefore not managed by ARC. It is - // an unsafe unretained pointer, so when you dereference it you better be - // sure that it is valid. - // - // Use displayContext when you wish to pass view context specific data along with the - // display traits to subnodes. This should be a piece of data owned by an - // ASViewController, which will ensure that the data is still valid when laying out - // its subviews. When the VC is dealloc'ed, the displayContext it created will also - // be dealloced but any subnodes that are hanging around (why would they be?) will now - // have a displayContext that points to a bad pointer. - // - // As an added precaution ASDisplayTraitsClearDisplayContext is called from ASVC's desctructor - // which will propagate a nil displayContext to its subnodes. - id __unsafe_unretained displayContext; -} ASEnvironmentTraitCollection; -extern void ASEnvironmentTraitCollectionUpdateDisplayContext(id rootEnvironment, id _Nullable context); + CGSize containerWindowSize; +} ASEnvironmentTraitCollection; extern ASEnvironmentTraitCollection ASEnvironmentTraitCollectionFromUITraitCollection(UITraitCollection *traitCollection); extern BOOL ASEnvironmentTraitCollectionIsEqualToASEnvironmentTraitCollection(ASEnvironmentTraitCollection lhs, ASEnvironmentTraitCollection rhs); @@ -165,14 +149,11 @@ ASDISPLAYNODE_EXTERN_C_END if (ASEnvironmentTraitCollectionIsEqualToASEnvironmentTraitCollection(currentTraits, oldTraits) == NO) {\ /* Must dispatch to main for self.view && [self.view.dataController completedNodes]*/ \ ASPerformBlockOnMainThread(^{\ - BOOL needsLayout = (oldTraits.displayContext == currentTraits.displayContext) || currentTraits.displayContext != nil;\ NSArray *> *completedNodes = [self.view.dataController completedNodes];\ for (NSArray *sectionArray in completedNodes) {\ for (ASCellNode *cellNode in sectionArray) {\ ASEnvironmentStatePropagateDown(cellNode, currentTraits);\ - if (needsLayout) {\ - [cellNode setNeedsLayout];\ - }\ + [cellNode setNeedsLayout];\ }\ }\ });\ diff --git a/AsyncDisplayKit/Details/ASEnvironment.mm b/AsyncDisplayKit/Details/ASEnvironment.mm index 77ffca7a0f..1f7526d7b9 100644 --- a/AsyncDisplayKit/Details/ASEnvironment.mm +++ b/AsyncDisplayKit/Details/ASEnvironment.mm @@ -26,23 +26,11 @@ ASEnvironmentHierarchyState _ASEnvironmentHierarchyStateMakeDefault() }; } -extern void ASEnvironmentTraitCollectionUpdateDisplayContext(id rootEnvironment, id context) -{ - ASEnvironmentState envState = [rootEnvironment environmentState]; - ASEnvironmentTraitCollection environmentTraitCollection = envState.environmentTraitCollection; - environmentTraitCollection.displayContext = context; - envState.environmentTraitCollection = environmentTraitCollection; - [rootEnvironment setEnvironmentState:envState]; - - for (id child in [rootEnvironment children]) { - ASEnvironmentStatePropagateDown(child, environmentTraitCollection); - } -} - ASEnvironmentTraitCollection _ASEnvironmentTraitCollectionMakeDefault() { return (ASEnvironmentTraitCollection) { // Default values can be defined in here + .containerWindowSize = CGSizeZero, }; } @@ -69,7 +57,7 @@ BOOL ASEnvironmentTraitCollectionIsEqualToASEnvironmentTraitCollection(ASEnviron lhs.displayScale == rhs.displayScale && lhs.userInterfaceIdiom == rhs.userInterfaceIdiom && lhs.forceTouchCapability == rhs.forceTouchCapability && - lhs.displayContext == rhs.displayContext; + CGSizeEqualToSize(lhs.containerWindowSize, rhs.containerWindowSize); } ASEnvironmentState ASEnvironmentStateMakeDefault() diff --git a/AsyncDisplayKit/Details/ASTraitCollection.h b/AsyncDisplayKit/Details/ASTraitCollection.h index 9524886d0f..c737b5d874 100644 --- a/AsyncDisplayKit/Details/ASTraitCollection.h +++ b/AsyncDisplayKit/Details/ASTraitCollection.h @@ -18,21 +18,7 @@ @property (nonatomic, assign, readonly) UIUserInterfaceIdiom userInterfaceIdiom; @property (nonatomic, assign, readonly) UIUserInterfaceSizeClass verticalSizeClass; @property (nonatomic, assign, readonly) UIForceTouchCapability forceTouchCapability; - -/** - * An optional context to pass along with an ASTraitCollection. - * This can be used to pass any internal state to all subnodes via the ASTraitCollection that is not - * included in UITraitCollection. This could range from more fine-tuned size classes to a class of - * constants that is based upon the new trait collection. - * - * Be aware that internally this context is held by a C struct which cannot retain the pointer. - * ASTraitCollection is generally a very short-lived class, existing only to provide a non-struct API - * to trait collections. When an ASTraitCollection is returned via one of ASViewController's 2 - * custom trait collection creation blocks, traitCollectionContext is assigned to the VC's traitCollectionContext. - * This makes sure that the VC is the owner of the context and ASEnvironmentTraitCollections will not - * have a reference to a dangling pointer. - */ -@property (nonatomic, strong, readonly) id traitCollectionContext; +@property (nonatomic, assign, readonly) CGSize containerWindowSize; + (ASTraitCollection *)traitCollectionWithASEnvironmentTraitCollection:(ASEnvironmentTraitCollection)traits; diff --git a/AsyncDisplayKit/Details/ASTraitCollection.m b/AsyncDisplayKit/Details/ASTraitCollection.m index 49fb94c780..f823fde8d6 100644 --- a/AsyncDisplayKit/Details/ASTraitCollection.m +++ b/AsyncDisplayKit/Details/ASTraitCollection.m @@ -21,7 +21,7 @@ horizontalSizeClass:(UIUserInterfaceSizeClass)horizontalSizeClass verticalSizeClass:(UIUserInterfaceSizeClass)verticalSizeClass forceTouchCapability:(UIForceTouchCapability)forceTouchCapability - traitCollectionContext:(id)traitCollectionContext + containerWindowSize:(CGSize)windowSize { self = [super init]; if (self) { @@ -30,7 +30,7 @@ _horizontalSizeClass = horizontalSizeClass; _verticalSizeClass = verticalSizeClass; _forceTouchCapability = forceTouchCapability; - _traitCollectionContext = traitCollectionContext; + _containerWindowSize = windowSize; } return self; } @@ -40,29 +40,29 @@ horizontalSizeClass:(UIUserInterfaceSizeClass)horizontalSizeClass verticalSizeClass:(UIUserInterfaceSizeClass)verticalSizeClass forceTouchCapability:(UIForceTouchCapability)forceTouchCapability - traitCollectionContext:(id)traitCollectionContext + containerWindowSize:(CGSize)windowSize { return [[[self class] alloc] initWithDisplayScale:displayScale userInterfaceIdiom:userInterfaceIdiom horizontalSizeClass:horizontalSizeClass verticalSizeClass:verticalSizeClass forceTouchCapability:forceTouchCapability - traitCollectionContext:traitCollectionContext]; + containerWindowSize:windowSize]; } + (ASTraitCollection *)traitCollectionWithASEnvironmentTraitCollection:(ASEnvironmentTraitCollection)traits { - return [[[self class] alloc] initWithDisplayScale:traits.displayScale - userInterfaceIdiom:traits.userInterfaceIdiom - horizontalSizeClass:traits.horizontalSizeClass - verticalSizeClass:traits.verticalSizeClass - forceTouchCapability:traits.forceTouchCapability - traitCollectionContext:traits.displayContext]; + return [[[self class] alloc] initWithDisplayScale:traits.displayScale + userInterfaceIdiom:traits.userInterfaceIdiom + horizontalSizeClass:traits.horizontalSizeClass + verticalSizeClass:traits.verticalSizeClass + forceTouchCapability:traits.forceTouchCapability + containerWindowSize:traits.containerWindowSize]; } + (ASTraitCollection *)traitCollectionWithUITraitCollection:(UITraitCollection *)traitCollection - traitCollectionContext:(id)traitCollectionContext + containerWindowSize:(CGSize)windowSize { ASTraitCollection *asyncTraitCollection = nil; if (AS_AT_LEAST_IOS9) { @@ -71,7 +71,7 @@ horizontalSizeClass:traitCollection.horizontalSizeClass verticalSizeClass:traitCollection.verticalSizeClass forceTouchCapability:traitCollection.forceTouchCapability - traitCollectionContext:traitCollectionContext]; + containerWindowSize:windowSize]; } else if (AS_AT_LEAST_IOS8) { asyncTraitCollection = [[[self class] alloc] initWithDisplayScale:traitCollection.displayScale @@ -79,7 +79,7 @@ horizontalSizeClass:traitCollection.horizontalSizeClass verticalSizeClass:traitCollection.verticalSizeClass forceTouchCapability:0 - traitCollectionContext:traitCollectionContext]; + containerWindowSize:windowSize]; } else { asyncTraitCollection = [[[self class] alloc] init]; } @@ -95,7 +95,7 @@ .userInterfaceIdiom = self.userInterfaceIdiom, .verticalSizeClass = self.verticalSizeClass, .forceTouchCapability = self.forceTouchCapability, - .displayContext = self.traitCollectionContext, + .containerWindowSize = self.containerWindowSize, }; } @@ -105,7 +105,7 @@ self.horizontalSizeClass == traitCollection.horizontalSizeClass && self.verticalSizeClass == traitCollection.verticalSizeClass && self.userInterfaceIdiom == traitCollection.userInterfaceIdiom && - self.traitCollectionContext == traitCollection.traitCollectionContext && + CGSizeEqualToSize(self.containerWindowSize, traitCollection.containerWindowSize) && self.forceTouchCapability == traitCollection.forceTouchCapability; } diff --git a/AsyncDisplayKit/Private/ASEnvironmentInternal.mm b/AsyncDisplayKit/Private/ASEnvironmentInternal.mm index 727b073b01..9e80f609e0 100644 --- a/AsyncDisplayKit/Private/ASEnvironmentInternal.mm +++ b/AsyncDisplayKit/Private/ASEnvironmentInternal.mm @@ -207,7 +207,7 @@ ASEnvironmentState ASEnvironmentMergeObjectAndState(ASEnvironmentState childEnvi childTraitCollection.userInterfaceIdiom = parentTraitCollection.userInterfaceIdiom; childTraitCollection.forceTouchCapability = parentTraitCollection.forceTouchCapability; childTraitCollection.displayScale = parentTraitCollection.displayScale; - childTraitCollection.displayContext = parentTraitCollection.displayContext; + childTraitCollection.containerWindowSize = parentTraitCollection.containerWindowSize; childEnvironmentState.environmentTraitCollection = childTraitCollection; }