From 35adc49c24d7735e7123b72571c6a91a5789b2f9 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Tue, 4 Jul 2017 19:19:09 +0100 Subject: [PATCH 1/2] Fix a crash in table view caused by executing an empty change set during layoutSubviews - Previously, when a change set is empty, `ASDataController` forwards the change set to its delegate right away, without dispatching to its editing queue and then back to main. - This behaviour can potentially cause bad internal states in UITableView which trigger a crash reported in https://github.com/TextureGroup/Texture/issues/83. - Fix by still reusing the existing pending map, because the data source's state has not changed, but go through the editing queue and main queue tunnel. --- Source/Details/ASDataController.mm | 43 +++++++++++++----------------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/Source/Details/ASDataController.mm b/Source/Details/ASDataController.mm index 932a12dac..959ef4fda 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -551,37 +551,32 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet } } - // Since we waited for _editingTransactionGroup at the beginning of this method, at this point we can guarantee that _pendingMap equals to _visibleMap. - // So if the change set is empty, we don't need to modify data and can safely schedule to notify the delegate. - if (changeSet.isEmpty) { - [_mainSerialQueue performBlockOnMainThread:^{ - [_delegate dataController:self willUpdateWithChangeSet:changeSet]; - [_delegate dataController:self didUpdateWithChangeSet:changeSet]; - }]; - return; - } - - BOOL canDelegateLayout; + BOOL canDelegateLayout = (_layoutDelegate != nil); ASElementMap *newMap; id layoutContext; { as_activity_scope(as_activity_create("Latch new data for collection update", changeSet.rootActivity, OS_ACTIVITY_FLAG_DEFAULT)); - // Mutable copy of current data. - ASElementMap *previousMap = _pendingMap; - ASMutableElementMap *mutableMap = [previousMap mutableCopy]; - canDelegateLayout = (_layoutDelegate != nil); + // Step 1: Populate a new map that reflects the data source's state and use it as pendingMap + if (changeSet.isEmpty) { + // If the change set is empty, nothing has changed so we can just reuse the previous map + newMap = self.pendingMap; + } else { + // Mutable copy of current data. + ASElementMap *previousMap = self.pendingMap; + ASMutableElementMap *mutableMap = [previousMap mutableCopy]; - // Step 1: Update the mutable copies to match the data source's state - [self _updateSectionContextsInMap:mutableMap changeSet:changeSet]; - ASPrimitiveTraitCollection existingTraitCollection = [self.node primitiveTraitCollection]; - [self _updateElementsInMap:mutableMap changeSet:changeSet traitCollection:existingTraitCollection shouldFetchSizeRanges:(! canDelegateLayout) previousMap:previousMap]; + // Step 1.1: Update the mutable copies to match the data source's state + [self _updateSectionContextsInMap:mutableMap changeSet:changeSet]; + ASPrimitiveTraitCollection existingTraitCollection = [self.node primitiveTraitCollection]; + [self _updateElementsInMap:mutableMap changeSet:changeSet traitCollection:existingTraitCollection shouldFetchSizeRanges:(! canDelegateLayout) previousMap:previousMap]; - // Step 2: Clone the new data - newMap = [mutableMap copy]; + // Step 1.2: Clone the new data + newMap = [mutableMap copy]; + } self.pendingMap = newMap; - // Step 3: Ask layout delegate for contexts + // Step 2: Ask layout delegate for contexts if (canDelegateLayout) { layoutContext = [_layoutDelegate layoutContextWithElements:newMap]; } @@ -592,7 +587,7 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet dispatch_group_async(_editingTransactionGroup, _editingTransactionQueue, ^{ __block __unused os_activity_scope_state_s preparationScope = {}; // unused if deployment target < iOS10 as_activity_scope_enter(as_activity_create("Prepare nodes for collection update", AS_ACTIVITY_CURRENT, OS_ACTIVITY_FLAG_DEFAULT), &preparationScope); - // Step 4: Allocate and layout elements if can't delegate + // Step 3: Allocate and layout elements if can't delegate NSArray *elementsToProcess; if (canDelegateLayout) { // Allocate all nodes before handling them to the layout delegate. @@ -617,7 +612,7 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet as_activity_scope_leave(&preparationScope); [_delegate dataController:self willUpdateWithChangeSet:changeSet]; - // Step 5: Deploy the new data as "completed" and inform delegate + // Step 4: Deploy the new data as "completed" and inform delegate self.visibleMap = newMap; [_delegate dataController:self didUpdateWithChangeSet:changeSet]; From 4da9659c2c99c8d0fcd5ecf6c72c26f877de06fc Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Tue, 4 Jul 2017 20:08:58 +0100 Subject: [PATCH 2/2] Update CHANGELOG --- CHANGELOG.md | 1 + Source/Details/ASDataController.mm | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98e348db8..b85ccc6f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ##2.3.5 - Fix an issue where inserting/deleting sections could lead to inconsistent supplementary element behavior. [Adlai Holler](https://github.com/Adlai-Holler) - Overhaul logging and add activity tracing support. [Adlai Holler](https://github.com/Adlai-Holler) +- Fix a crash where scrolling a table view after entering editing mode could lead to bad internal states in the table. [Huy Nguyen](https://github.com/nguyenhuy) [#416](https://github.com/TextureGroup/Texture/pull/416/) ##2.3.4 - [Yoga] Rewrite YOGA_TREE_CONTIGUOUS mode with improved behavior and cleaner integration [Scott Goodson](https://github.com/appleguy) diff --git a/Source/Details/ASDataController.mm b/Source/Details/ASDataController.mm index 959ef4fda..9265d8d0a 100644 --- a/Source/Details/ASDataController.mm +++ b/Source/Details/ASDataController.mm @@ -558,12 +558,12 @@ - (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet as_activity_scope(as_activity_create("Latch new data for collection update", changeSet.rootActivity, OS_ACTIVITY_FLAG_DEFAULT)); // Step 1: Populate a new map that reflects the data source's state and use it as pendingMap + ASElementMap *previousMap = self.pendingMap; if (changeSet.isEmpty) { // If the change set is empty, nothing has changed so we can just reuse the previous map - newMap = self.pendingMap; + newMap = previousMap; } else { // Mutable copy of current data. - ASElementMap *previousMap = self.pendingMap; ASMutableElementMap *mutableMap = [previousMap mutableCopy]; // Step 1.1: Update the mutable copies to match the data source's state