From 1872c1fb5e409c7c7cd9dee1a6a9ccd2a2758263 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 3 Oct 2018 16:14:01 -0700 Subject: [PATCH] Only clear ASCollectionView's data during deallocation (#1154) This is a follow up on #1136. Our experiment results show that clearing data frequently is the cause of our #1 crash. @maicki and I believe that this is because if the collection view is being used, silently clearing its data without notifying the backing UICollectionView can put it out-of-sync and causes mayhem next time the collection view processes a batch update. If you look at the stack trace closely, you'll notice that the crash doesn't occur on the same run loop that clearData is called. This made it extremely tricky to investigate and identify the root cause. Another interesting question would be whether or not we want to clear the data during deallocation at all, since the data will be cleared out soon anyway. --- CHANGELOG.md | 1 + Schemas/configuration.json | 3 ++- Source/ASCollectionView.mm | 2 +- Source/ASExperimentalFeatures.h | 2 +- Source/ASExperimentalFeatures.m | 2 +- 5 files changed, 6 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3701bce94..85b2f0843 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ - Optimize layout process by removing `ASRectMap`. [Adlai Holler](https://github.com/Adlai-Holler) - Remove necessity to use view to access rangeController in ASTableNode, ASCollectionNode. [Michael Schneider](https://github.com/maicki) - Remove display node's reliance on shared_ptr. [Adlai Holler](https://github.com/Adlai-Holler) +- [ASCollectionView] Fix a crash that is caused by clearing a collection view's data while it's still being used. [Huy Nguyen](https://github.com/nguyenhuy) [#1154](https://github.com/TextureGroup/Texture/pull/1154) - Clean up timing of layout tree flattening/ copying of unflattened tree for Weaver. [Michael Zuccarino](https://github.com/mikezucc) [#1157](https://github.com/TextureGroup/Texture/pull/1157) ## 2.7 diff --git a/Schemas/configuration.json b/Schemas/configuration.json index 29c4375a4..33b1f43c3 100644 --- a/Schemas/configuration.json +++ b/Schemas/configuration.json @@ -21,7 +21,8 @@ "exp_network_image_queue", "exp_dealloc_queue_v2", "exp_collection_teardown", - "exp_framesetter_cache" + "exp_framesetter_cache", + "exp_clear_data_during_deallocation" ] } } diff --git a/Source/ASCollectionView.mm b/Source/ASCollectionView.mm index 94406b0ee..0c36204a0 100644 --- a/Source/ASCollectionView.mm +++ b/Source/ASCollectionView.mm @@ -577,7 +577,7 @@ - (void)_asyncDelegateOrDataSourceDidChange { ASDisplayNodeAssertMainThread(); - if (_asyncDataSource == nil && _asyncDelegate == nil && !ASActivateExperimentalFeature(ASExperimentalSkipClearData)) { + if (_asyncDataSource == nil && _asyncDelegate == nil && _isDeallocating && ASActivateExperimentalFeature(ASExperimentalClearDataDuringDeallocation)) { [_dataController clearData]; } } diff --git a/Source/ASExperimentalFeatures.h b/Source/ASExperimentalFeatures.h index 2af937d66..626975636 100644 --- a/Source/ASExperimentalFeatures.h +++ b/Source/ASExperimentalFeatures.h @@ -23,7 +23,7 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) { ASExperimentalNetworkImageQueue = 1 << 5, // exp_network_image_queue ASExperimentalCollectionTeardown = 1 << 6, // exp_collection_teardown ASExperimentalFramesetterCache = 1 << 7, // exp_framesetter_cache - ASExperimentalSkipClearData = 1 << 8, // exp_skip_clear_data + ASExperimentalClearDataDuringDeallocation = 1 << 8, // exp_clear_data_during_deallocation ASExperimentalFeatureAll = 0xFFFFFFFF }; diff --git a/Source/ASExperimentalFeatures.m b/Source/ASExperimentalFeatures.m index b5751c873..73abea48b 100644 --- a/Source/ASExperimentalFeatures.m +++ b/Source/ASExperimentalFeatures.m @@ -21,7 +21,7 @@ @"exp_dealloc_queue_v2", @"exp_collection_teardown", @"exp_framesetter_cache", - @"exp_skip_clear_data"])); + @"exp_clear_data_during_deallocation"])); if (flags == ASExperimentalFeatureAll) { return allNames;