Fjalapeno has submitted this change and it was merged.
Change subject: gallery: re-add out of bounds assertion
......................................................................
gallery: re-add out of bounds assertion
Fetching indices out of bounds is a programmer error, and should be an
assertion. The real problem causing T95601 was that the call to
"fetchBatchContainingIndex:withNthNeighbor:" was attempting to fetch out
of bounds. This patch fixes that problem and puts the assertion back in
place (to give us the feedback that was originally necessary to find issue)
while preserving fallback behavior when assertions are disabled in an attempt
to allow the app to continue to function (albeit in a semi undefined
state).
The original bug could be reproduced by scrolling any of the 5th last
images in a gallery. I reproduced the assertion by doing this when it
was added, then added a unit test w/ a fix.
Bug: T95601
Change-Id: Ie2bf16998726de73b98a8c8e9e9cb5f60fca8b8c
---
M Wikipedia/View Controllers/Image Gallery/WMFImageInfoController.m
M WikipediaUnitTests/WMFImageInfoControllerTests.m
2 files changed, 16 insertions(+), 3 deletions(-)
Approvals:
Fjalapeno: Looks good to me, approved
Mhurd: Looks good to me, but someone else must approve
jenkins-bot: Verified
diff --git a/Wikipedia/View Controllers/Image Gallery/WMFImageInfoController.m
b/Wikipedia/View Controllers/Image Gallery/WMFImageInfoController.m
index dd01da9..03875d8 100644
--- a/Wikipedia/View Controllers/Image Gallery/WMFImageInfoController.m
+++ b/Wikipedia/View Controllers/Image Gallery/WMFImageInfoController.m
@@ -139,14 +139,20 @@
- (NSArray*)fetchBatchContainingIndex:(NSInteger)index
withNthNeighbor:(NSUInteger)next {
NSAssert(next >= 0, @"No reason to call this method with next == 0");
NSMutableIndexSet* indexes = [NSMutableIndexSet indexSetWithIndex:index];
- [indexes addIndex:index + next];
+ NSUInteger const neighborIndex = index + next;
+ if (neighborIndex < self.uniqueArticleImages.count) {
+ [indexes addIndex:index + next];
+ }
return [self fetchBatchesContainingIndexes:indexes];
}
#pragma mark - Private Fetch
- (NSRange)batchRangeForTargetIndex:(NSUInteger)index {
+ NSParameterAssert(index < self.uniqueArticleImages.count);
if (index > self.uniqueArticleImages.count) {
+ IICLog(@"WARNING: attempted to fetch %lu which is beoynd upper bound
of %lu",
+ index, self.uniqueArticleImages.count);
return WMFRangeMakeNotFound();
}
NSUInteger const start = floorf(index / (float)self.infoBatchSize) *
self.infoBatchSize;
@@ -154,11 +160,14 @@
NSParameterAssert(range.location <= index);
NSParameterAssert(WMFRangeGetMaxIndex(range) >= index);
NSParameterAssert(WMFRangeGetMaxIndex(range) <=
self.uniqueArticleImages.count);
+ NSParameterAssert(!WMFRangeIsNotFoundOrEmpty(range));
return range;
}
- (id<MWKImageInfoRequest>)fetchBatch:(NSRange)batch {
+ NSParameterAssert(!WMFRangeIsNotFoundOrEmpty(batch));
if (WMFRangeIsNotFoundOrEmpty(batch)) {
+ IICLog(@"WARNING: attempted to fetch not found or empty range: %@",
NSStringFromRange(batch));
return nil;
} else if ([self.fetchedIndices containsIndexesInRange:batch]) {
IICLog(@"Batch %@ has already been fetched.",
NSStringFromRange(batch));
diff --git a/WikipediaUnitTests/WMFImageInfoControllerTests.m
b/WikipediaUnitTests/WMFImageInfoControllerTests.m
index 60394be..df8ad6f 100644
--- a/WikipediaUnitTests/WMFImageInfoControllerTests.m
+++ b/WikipediaUnitTests/WMFImageInfoControllerTests.m
@@ -104,8 +104,7 @@
- (void)testFetchBatchRanges {
- NSMutableIndexSet* indexesToFetch = [NSMutableIndexSet new];
- [indexesToFetch addIndex:0];
+ NSMutableIndexSet* indexesToFetch = [NSMutableIndexSet
indexSetWithIndex:0];
[indexesToFetch addIndex:self.controller.uniqueArticleImages.count - 1];
[self.controller fetchBatchesContainingIndexes:indexesToFetch];
[indexesToFetch enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL *stop) {
@@ -119,6 +118,11 @@
}];
}
+- (void)testIgnoreOutOfBoundsNeighbor {
+ [self.controller fetchBatchContainingIndex:0
withNthNeighbor:self.controller.uniqueArticleImages.count + 1];
+ [self verifyInfoFetcherCallForIndexes:[NSIndexSet indexSetWithIndex:0]];
+}
+
- (void)testFetchBatchAlongWithNeighborReturnsOneRequestForEachFetch {
[given([self.mockInfoFetcher
fetchInfoForPageTitles:[self
expectedTitlesForRange:[self.controller batchRangeForTargetIndex:0]]
--
To view, visit https://gerrit.wikimedia.org/r/203339
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie2bf16998726de73b98a8c8e9e9cb5f60fca8b8c
Gerrit-PatchSet: 3
Gerrit-Project: apps/ios/wikipedia
Gerrit-Branch: master
Gerrit-Owner: Bgerstle <[email protected]>
Gerrit-Reviewer: Bgerstle <[email protected]>
Gerrit-Reviewer: Dr0ptp4kt <[email protected]>
Gerrit-Reviewer: Fjalapeno <[email protected]>
Gerrit-Reviewer: Mhurd <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
