[ios] Adds invalidate test, and test that reloads packs during removal

5 files changed, 103 insertions, 125 deletions

diff --git a/platform/darwin/src/MGLOfflinePack.mm b/platform/darwin/src/MGLOfflinePack.mm
index 0f2e8180fa..6648867a62 100644
--- a/platform/darwin/src/MGLOfflinePack.mm
+++ b/platform/darwin/src/MGLOfflinePack.mm
@@ -135,9 +135,12 @@ private:
 - (void)invalidate {
     MGLLogInfo(@"Invalidating pack.");
     MGLAssert(_state != MGLOfflinePackStateInvalid, @"Cannot invalidate an already invalid offline pack.");
+    MGLAssert(self.mbglOfflineRegion, @"Should have a valid region");
 
     self.state = MGLOfflinePackStateInvalid;
-    _mbglFileSource->setOfflineRegionObserver(*self.mbglOfflineRegion, nullptr);
+    if (self.mbglOfflineRegion) {
+        _mbglFileSource->setOfflineRegionObserver(*self.mbglOfflineRegion, nullptr);
+    }
     self.mbglOfflineRegion = nil;
 }
 
diff --git a/platform/darwin/src/MGLOfflineStorage.mm b/platform/darwin/src/MGLOfflineStorage.mm
index 93f986a518..32d1735bc0 100644
--- a/platform/darwin/src/MGLOfflineStorage.mm
+++ b/platform/darwin/src/MGLOfflineStorage.mm
@@ -426,11 +426,15 @@ const MGLExceptionName MGLUnsupportedRegionTypeException = @"MGLUnsupportedRegio
 
 - (void)_removePack:(MGLOfflinePack *)pack withCompletionHandler:(MGLOfflinePackRemovalCompletionHandler)completion {
     mbgl::OfflineRegion *mbglOfflineRegion = pack.mbglOfflineRegion;
+
     [pack invalidate];
+
     if (!mbglOfflineRegion) {
+        MGLAssert(pack.state == MGLOfflinePackStateInvalid, @"State should be invalid");
         completion(nil);
         return;
     }
+    
     _mbglFileSource->deleteOfflineRegion(std::move(*mbglOfflineRegion), [&, completion](std::exception_ptr exception) {
         NSError *error;
         if (exception) {
diff --git a/platform/darwin/test/MGLOfflinePackTests.m b/platform/darwin/test/MGLOfflinePackTests.mm
index a33665ff0a..1306b182d1 100644
--- a/platform/darwin/test/MGLOfflinePackTests.m
+++ b/platform/darwin/test/MGLOfflinePackTests.mm
@@ -1,5 +1,7 @@
 #import <Mapbox/Mapbox.h>
 #import <XCTest/XCTest.h>
+#import "MGLOfflinePack_Private.h"
+#import "MGLTestAssertionHandler.h"
 
 @interface MGLOfflinePackTests : XCTestCase
 
@@ -18,6 +20,21 @@
     XCTAssertThrowsSpecificNamed([invalidPack suspend], NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when being suspended.");
 }
 
+- (void)testInvalidatingAnInvalidPack {
+    MGLOfflinePack *invalidPack = [[MGLOfflinePack alloc] init];
+
+    XCTAssertThrowsSpecificNamed([invalidPack invalidate], NSException, NSInternalInconsistencyException, @"Invalid offline pack should raise an exception when being invalidated.");
+
+    // Now try again, without asserts
+    NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
+    [[[NSThread currentThread] threadDictionary] setValue:[[MGLTestAssertionHandler alloc] init] forKey:NSAssertionHandlerKey];
+
+    // Make sure this doesn't crash without asserts
+    [invalidPack invalidate];
+    
+    [[[NSThread currentThread] threadDictionary] setValue:oldHandler forKey:NSAssertionHandlerKey];
+}
+
 - (void)testProgressBoxing {
     MGLOfflinePackProgress progress = {
         .countOfResourcesCompleted = 3,
diff --git a/platform/darwin/test/MGLOfflineStorageTests.mm b/platform/darwin/test/MGLOfflineStorageTests.mm
index 0138d911bb..c4f8b84446 100644
--- a/platform/darwin/test/MGLOfflineStorageTests.mm
+++ b/platform/darwin/test/MGLOfflineStorageTests.mm
@@ -311,52 +311,10 @@
     XCTAssertEqual([MGLOfflineStorage sharedOfflineStorage].packs.count, countOfPacks - 1, @"Removed pack should have been removed from the canonical collection of packs owned by the shared offline storage object. This assertion can fail if this test is run before -testAAALoadPacks or -testAddPack.");
 }
 
-- (void)testRemovePackTwiceInSuccession {
-
-    [self addPackIfNeeded];
-    
-    NSUInteger countOfPacks = [MGLOfflineStorage sharedOfflineStorage].packs.count;
-
-    MGLOfflinePack *pack = [MGLOfflineStorage sharedOfflineStorage].packs.lastObject;
-    XCTAssertNotNil(pack, @"Added pack should still exist.");
-
-    [self keyValueObservingExpectationForObject:[MGLOfflineStorage sharedOfflineStorage] keyPath:@"packs" handler:^BOOL(id _Nonnull observedObject, NSDictionary * _Nonnull change) {
-        const auto changeKind = static_cast<NSKeyValueChange>([change[NSKeyValueChangeKindKey] unsignedLongValue]);
-        NSIndexSet *indices = change[NSKeyValueChangeIndexesKey];
-        return changeKind == NSKeyValueChangeRemoval && indices.count == 1;
-    }];
+- (void)addPacks:(NSInteger)count {
     
-    XCTestExpectation *completionHandlerExpectation = [self expectationWithDescription:@"remove pack completion handler"];
-
-    [[MGLOfflineStorage sharedOfflineStorage] removePack:pack withCompletionHandler:nil];
-
-    NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
-    [[[NSThread currentThread] threadDictionary] setValue:[[MGLTestAssertionHandler alloc] init] forKey:NSAssertionHandlerKey];
-    
-    [[MGLOfflineStorage sharedOfflineStorage] removePack:pack withCompletionHandler:^(NSError * _Nullable error) {
-        XCTAssertEqual(pack.state, MGLOfflinePackStateInvalid, @"Removed pack should be invalid in the completion handler.");
-        [completionHandlerExpectation fulfill];
-    }];
-    
-    [self waitForExpectationsWithTimeout:5 handler:nil];
-
-    [[[NSThread currentThread] threadDictionary] setValue:oldHandler forKey:NSAssertionHandlerKey];
-
-    XCTAssertEqual(pack.state, MGLOfflinePackStateInvalid, @"Removed pack should have been invalidated synchronously.");
-
-    XCTAssertEqual([MGLOfflineStorage sharedOfflineStorage].packs.count, countOfPacks - 1, @"Removed pack should have been removed from the canonical collection of packs owned by the shared offline storage object. This assertion can fail if this test is run before -testAAALoadPacks or -testAddPack.");
-}
-
-- (void)addPackIfNeeded {
     XCTestExpectation *expectation = [self expectationWithDescription:@"added packs"];
-    
-    [self addPacks:1 completion:^{
-        [expectation fulfill];
-    }];
-    [self waitForExpectations:@[expectation] timeout:1.0];
-}
 
-- (void)addPacks:(NSInteger)count completion:(dispatch_block_t)block {
     NSURL *styleURL = [MGLStyle lightStyleURLWithVersion:8];
         
     MGLCoordinateBounds bounds[] = {
@@ -390,101 +348,97 @@
         }];
     }
     
-    if (block) {
-        dispatch_group_notify(group, dispatch_get_main_queue(), block);
-    }
+    dispatch_group_notify(group, dispatch_get_main_queue(), ^{
+        [expectation fulfill];
+    });
+    
+    [self waitForExpectations:@[expectation] timeout:1.0];
 }
 
-// Test to explore https://github.com/mapbox/mapbox-gl-native/issues/15536
-- (void)testRemovePacksOnBackgroundQueueWhileReloading {
+- (void)testRemovePackTwiceInSuccession {
+
+    [self addPacks:1];
+    
     NSUInteger countOfPacks = [MGLOfflineStorage sharedOfflineStorage].packs.count;
 
-    if (countOfPacks < 4) {
-        XCTestExpectation *expectation = [self expectationWithDescription:@"added packs"];
+    MGLOfflinePack *pack = [MGLOfflineStorage sharedOfflineStorage].packs.lastObject;
+    XCTAssertNotNil(pack, @"Added pack should still exist.");
 
-        [self addPacks:4 completion:^{
-            [expectation fulfill];
-        }];
-        [self waitForExpectations:@[expectation] timeout:10.0];
-    }
+    [self keyValueObservingExpectationForObject:[MGLOfflineStorage sharedOfflineStorage] keyPath:@"packs" handler:^BOOL(id _Nonnull observedObject, NSDictionary * _Nonnull change) {
+        const auto changeKind = static_cast<NSKeyValueChange>([change[NSKeyValueChangeKindKey] unsignedLongValue]);
+        NSIndexSet *indices = change[NSKeyValueChangeIndexesKey];
+        return changeKind == NSKeyValueChangeRemoval && indices.count == 1;
+    }];
+    
+    XCTestExpectation *completionHandlerExpectation = [self expectationWithDescription:@"remove pack completion handler"];
+
+    [[MGLOfflineStorage sharedOfflineStorage] removePack:pack withCompletionHandler:nil];
+
+    NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
+    [[[NSThread currentThread] threadDictionary] setValue:[[MGLTestAssertionHandler alloc] init] forKey:NSAssertionHandlerKey];
+    
+    [[MGLOfflineStorage sharedOfflineStorage] removePack:pack withCompletionHandler:^(NSError * _Nullable error) {
+        XCTAssertEqual(pack.state, MGLOfflinePackStateInvalid, @"Removed pack should be invalid in the completion handler.");
+        [completionHandlerExpectation fulfill];
+    }];
+    
+    [self waitForExpectationsWithTimeout:5 handler:nil];
+
+    [[[NSThread currentThread] threadDictionary] setValue:oldHandler forKey:NSAssertionHandlerKey];
+
+    XCTAssertEqual(pack.state, MGLOfflinePackStateInvalid, @"Removed pack should have been invalidated synchronously.");
+
+    XCTAssertEqual([MGLOfflineStorage sharedOfflineStorage].packs.count, countOfPacks - 1, @"Removed pack should have been removed from the canonical collection of packs owned by the shared offline storage object. This assertion can fail if this test is run before -testAAALoadPacks or -testAddPack.");
+    
+    NSLog(@"Test `%@` complete", NSStringFromSelector(_cmd));
+}
+
+- (void)test15536RemovePacksWhileReloading {
+
+    // This test triggers
+    //
+    // throw std::runtime_error("Malformed offline region definition");
+    //
+    // in offline.cpp
+    //
+    // Reloading packs, while trying to remove them is currently problematic.
     
-    countOfPacks = [MGLOfflineStorage sharedOfflineStorage].packs.count;
+    [self addPacks:4];
+    
+    NSInteger countOfPacks = [MGLOfflineStorage sharedOfflineStorage].packs.count;
     XCTAssert(countOfPacks > 0);
 
     // Now delete packs one by one
-    dispatch_queue_t queue = dispatch_queue_create("com.mapbox.testRemovePacks", DISPATCH_QUEUE_SERIAL);
-    
-    XCTestExpectation *expectation = [self expectationWithDescription:@"all packs removed"];
+    XCTestExpectation *expectation = [self expectationWithDescription:@"All packs removed"];
     expectation.expectedFulfillmentCount = countOfPacks;
     
     MGLOfflineStorage *storage = [MGLOfflineStorage sharedOfflineStorage];
-    
+    NSArray *packs = [storage.packs copy];
+
+    // Simulate what happens the first time sharedOfflineStorage is accessed
     [storage reloadPacks];
-    
-    dispatch_async(queue, ^{
-        
-        NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
-        [[[NSThread currentThread] threadDictionary] setValue:[[MGLTestAssertionHandler alloc] init] forKey:NSAssertionHandlerKey];
+                
+    NSArray *validPacks = [packs filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id  _Nullable evaluatedObject, NSDictionary<NSString *,id> * _Nullable bindings) {
+        MGLOfflinePack *pack = (MGLOfflinePack*)evaluatedObject;
+        return pack.state != MGLOfflinePackStateInvalid;
+    }]];
 
-        
-        NSArray *packs = storage.packs;
-        
-        if (!packs) {
-            return;
-        }
-        
-        NSArray *validPacks = [packs filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id  _Nullable evaluatedObject, NSDictionary<NSString *,id> * _Nullable bindings) {
-            MGLOfflinePack *pack = (MGLOfflinePack*)evaluatedObject;
-            return pack.state != MGLOfflinePackStateInvalid;
-        }]];
-        
-        for (MGLOfflinePack *pack in validPacks) {
-            dispatch_group_t group = dispatch_group_create();
-            dispatch_group_enter(group);
-//          [storage removePack:pack withCompletionHandler:nil];
-            [storage removePack:pack withCompletionHandler:^(NSError * _Nullable error) {
-                dispatch_group_leave(group);
-            }];
-            dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
-                        
-            [expectation fulfill];
-        }
-        
-        [[[NSThread currentThread] threadDictionary] setValue:oldHandler forKey:NSAssertionHandlerKey];
+    NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
+    [[[NSThread currentThread] threadDictionary] setValue:[[MGLTestAssertionHandler alloc] init] forKey:NSAssertionHandlerKey];
 
-    });
+    for (MGLOfflinePack *pack in validPacks) {
+        [storage removePack:pack withCompletionHandler:^(NSError * _Nullable error) {
+            [expectation fulfill];
+        }];
+    }
     
-    [self waitForExpectations:@[expectation] timeout:60.0];
-    
-    countOfPacks = [MGLOfflineStorage sharedOfflineStorage].packs.count;
-    XCTAssertEqual(countOfPacks, 0);
-
-    
-    
-    //    so on `keyPath`  `packs` change (`NSKeyValueChange.settings`)
-//    we dispatch to a `SerialQueue` and remove the pack one by one and wait for the first one to be completed before proceeding to reduce the load on the Database access.
-//    serialQueue.async { [weak self] in
-//    guard let packs = storage.packs else {
-//    return []
-//    }
-//    let validPacks = packs.filter { $0.state != .invalid }
-//
-//    for pack in validPacks {
-//        let packGroup = DispatchGroup()
-//        packGroup.enter()
-//        storage.removePack(pack) { [weak self] (error) in
-//            packGroup.leave()
-//        }
-//        // Wait until the pack completion handler is executed
-//        // Marking the pack as removed for us
-//        packGroup.wait()
-//    }
-//    }
+    [[[NSThread currentThread] threadDictionary] setValue:oldHandler forKey:NSAssertionHandlerKey];
+    
+    [self waitForExpectations:@[expectation] timeout:10.0];
+    
+    NSLog(@"Test `%@` complete", NSStringFromSelector(_cmd));
 }
 
-
-
-
 - (void)testCountOfBytesCompleted {
     XCTAssertGreaterThan([MGLOfflineStorage sharedOfflineStorage].countOfBytesCompleted, 0UL);
 }
diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj
index a138e4ef06..66541ca813 100644
--- a/platform/ios/ios.xcodeproj/project.pbxproj
+++ b/platform/ios/ios.xcodeproj/project.pbxproj
@@ -552,7 +552,7 @@
 		DA2DBBCE1D51E80400D38FF9 /* MGLStyleLayerTests.m in Sources */ = {isa = PBXBuildFile; fileRef = DA2DBBCD1D51E80400D38FF9 /* MGLStyleLayerTests.m */; };
 		DA2E88561CC036F400F24E7B /* Mapbox.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = DA8847D21CBAF91600AB86E3 /* Mapbox.framework */; };
 		DA2E88611CC0382C00F24E7B /* MGLGeometryTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DA2E885C1CC0382C00F24E7B /* MGLGeometryTests.mm */; };
-		DA2E88621CC0382C00F24E7B /* MGLOfflinePackTests.m in Sources */ = {isa = PBXBuildFile; fileRef = DA2E885D1CC0382C00F24E7B /* MGLOfflinePackTests.m */; };
+		DA2E88621CC0382C00F24E7B /* MGLOfflinePackTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DA2E885D1CC0382C00F24E7B /* MGLOfflinePackTests.mm */; };
 		DA2E88631CC0382C00F24E7B /* MGLOfflineRegionTests.m in Sources */ = {isa = PBXBuildFile; fileRef = DA2E885E1CC0382C00F24E7B /* MGLOfflineRegionTests.m */; };
 		DA2E88651CC0382C00F24E7B /* MGLStyleTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DA2E88601CC0382C00F24E7B /* MGLStyleTests.mm */; };
 		DA35A29E1CC9E94C00E826B2 /* MGLCoordinateFormatter.h in Headers */ = {isa = PBXBuildFile; fileRef = DA35A29D1CC9E94C00E826B2 /* MGLCoordinateFormatter.h */; settings = {ATTRIBUTES = (Public, ); }; };
@@ -1247,7 +1247,7 @@
 		DA2E88511CC036F400F24E7B /* test.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = test.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
 		DA2E88551CC036F400F24E7B /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		DA2E885C1CC0382C00F24E7B /* MGLGeometryTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLGeometryTests.mm; path = ../../darwin/test/MGLGeometryTests.mm; sourceTree = "<group>"; };
-		DA2E885D1CC0382C00F24E7B /* MGLOfflinePackTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = MGLOfflinePackTests.m; path = ../../darwin/test/MGLOfflinePackTests.m; sourceTree = "<group>"; };
+		DA2E885D1CC0382C00F24E7B /* MGLOfflinePackTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLOfflinePackTests.mm; path = ../../darwin/test/MGLOfflinePackTests.mm; sourceTree = "<group>"; };
 		DA2E885E1CC0382C00F24E7B /* MGLOfflineRegionTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = MGLOfflineRegionTests.m; path = ../../darwin/test/MGLOfflineRegionTests.m; sourceTree = "<group>"; };
 		DA2E88601CC0382C00F24E7B /* MGLStyleTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLStyleTests.mm; path = ../../darwin/test/MGLStyleTests.mm; sourceTree = "<group>"; };
 		DA33895F1FA3EAB7001EA329 /* pt-BR */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = "pt-BR"; path = "pt-BR.lproj/Foundation.strings"; sourceTree = "<group>"; };
@@ -2089,7 +2089,7 @@
 				1F95931C1E6DE2E900D5B294 /* MGLNSDateAdditionsTests.mm */,
 				96036A0520059BBA00510F3D /* MGLNSOrthographyAdditionsTests.m */,
 				DAE7DEC11E245455007505A6 /* MGLNSStringAdditionsTests.m */,
-				DA2E885D1CC0382C00F24E7B /* MGLOfflinePackTests.m */,
+				DA2E885D1CC0382C00F24E7B /* MGLOfflinePackTests.mm */,
 				DA2E885E1CC0382C00F24E7B /* MGLOfflineRegionTests.m */,
 				55E2AD121E5B125400E8C587 /* MGLOfflineStorageTests.mm */,
 				35B8E08B1D6C8B5100E768D2 /* MGLPredicateTests.mm */,
@@ -3308,7 +3308,7 @@
 				9658C155204761FC00D8A674 /* MGLMapViewScaleBarTests.m in Sources */,
 				409D0A0D1ED614CE00C95D0C /* MGLAnnotationViewIntegrationTests.swift in Sources */,
 				9686D1BD22D9357700194EA0 /* MGLMapViewZoomTests.mm in Sources */,
-				DA2E88621CC0382C00F24E7B /* MGLOfflinePackTests.m in Sources */,
+				DA2E88621CC0382C00F24E7B /* MGLOfflinePackTests.mm in Sources */,
 				55E2AD131E5B125400E8C587 /* MGLOfflineStorageTests.mm in Sources */,
 				07D8C6FF1F67562C00381808 /* MGLComputedShapeSourceTests.m in Sources */,
 				920A3E5D1E6F995200C16EFC /* MGLSourceQueryTests.m in Sources */,