[ios, macos] Fix MGLOfflinePack invalidate crash (#15582)

6 files changed, 320 insertions, 6 deletions

diff --git a/platform/darwin/src/MGLOfflinePack.mm b/platform/darwin/src/MGLOfflinePack.mm
index 0f2e8180fa..5f4ae9c838 100644
--- a/platform/darwin/src/MGLOfflinePack.mm
+++ b/platform/darwin/src/MGLOfflinePack.mm
@@ -135,10 +135,15 @@ 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);
-    self.mbglOfflineRegion = nil;
+    @synchronized (self) {
+        self.state = MGLOfflinePackStateInvalid;
+        if (self.mbglOfflineRegion) {
+            _mbglFileSource->setOfflineRegionObserver(*self.mbglOfflineRegion, nullptr);
+        }
+        self.mbglOfflineRegion = nil;
+    }
 }
 
 - (void)setState:(MGLOfflinePackState)state {
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..6b454ee8ca 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,22 @@
     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];
+    MGLTestAssertionHandler *newHandler = [[MGLTestAssertionHandler alloc] initWithTestCase:self];
+    [[[NSThread currentThread] threadDictionary] setValue:newHandler 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 ee4bcc2c65..b44d4c51cd 100644
--- a/platform/darwin/test/MGLOfflineStorageTests.mm
+++ b/platform/darwin/test/MGLOfflineStorageTests.mm
@@ -1,15 +1,16 @@
 #import <Mapbox/Mapbox.h>
+#import <XCTest/XCTest.h>
 
 #import "MGLOfflineStorage_Private.h"
 #import "NSBundle+MGLAdditions.h"
 #import "NSDate+MGLAdditions.h"
-
-#import <XCTest/XCTest.h>
+#import "MGLTestAssertionHandler.h"
 
 #include <mbgl/util/run_loop.hpp>
 
 #pragma clang diagnostic ignored "-Wshadow"
 
+
 @interface MGLOfflineStorageTests : XCTestCase <MGLOfflineStorageDelegate>
 @end
 
@@ -34,7 +35,7 @@
 
 - (void)setUp {
     [super setUp];
-
+    
     static dispatch_once_t onceToken;
     dispatch_once(&onceToken, ^{
         XCTestExpectation *expectation = [self keyValueObservingExpectationForObject:[MGLOfflineStorage sharedOfflineStorage] keyPath:@"packs" handler:^BOOL(id _Nonnull observedObject, NSDictionary * _Nonnull change) {
@@ -310,6 +311,197 @@
     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)addPacks:(NSInteger)count {
+    
+    XCTestExpectation *expectation = [self expectationWithDescription:@"added packs"];
+
+    NSURL *styleURL = [MGLStyle lightStyleURLWithVersion:8];
+        
+    MGLCoordinateBounds bounds[] = {
+        {{51.5, -0.2},   {51.6, -0.1}},     // London
+        {{60.1, 24.8},   {60.3, 25.1}},     // Helsinki
+        {{38.9, -77.1},  {38.9, -77.0}},    // DC
+        {{37.7, -122.5}, {37.9, -122.4}}    // SF
+    };
+        
+    int arraySize = sizeof(bounds)/sizeof(bounds[0]);
+    
+    count = MIN(count, arraySize);
+    
+    dispatch_group_t group = dispatch_group_create();
+    
+    for (int i = 0; i < count; i++) {
+        
+        dispatch_group_enter(group);
+        MGLTilePyramidOfflineRegion *region = [[MGLTilePyramidOfflineRegion alloc] initWithStyleURL:styleURL bounds:bounds[i] fromZoomLevel:20 toZoomLevel:20];
+        NSData *context = [NSKeyedArchiver archivedDataWithRootObject:@{
+            @"index": @(i)
+        }];
+
+        [[MGLOfflineStorage sharedOfflineStorage] addPackForRegion:region
+                                                       withContext:context
+                                                 completionHandler:^(MGLOfflinePack * _Nullable pack, NSError * _Nullable error) {
+            XCTAssertNotNil(pack);
+            XCTAssertNil(error);
+            
+            dispatch_group_leave(group);
+        }];
+    }
+    
+    dispatch_group_notify(group, dispatch_get_main_queue(), ^{
+        [expectation fulfill];
+    });
+    
+    [self waitForExpectations:@[expectation] timeout:1.0];
+}
+
+- (void)testRemovePackTwiceInSuccession {
+
+    [self addPacks:1];
+    
+    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;
+    }];
+    
+    XCTestExpectation *completionHandlerExpectation = [self expectationWithDescription:@"remove pack completion handler"];
+
+    [[MGLOfflineStorage sharedOfflineStorage] removePack:pack withCompletionHandler:nil];
+
+    NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
+    MGLTestAssertionHandler *newHandler = [[MGLTestAssertionHandler alloc] initWithTestCase:self];
+
+    [[[NSThread currentThread] threadDictionary] setValue:newHandler 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.
+    
+    [self addPacks:4];
+    
+    NSInteger countOfPacks = [MGLOfflineStorage sharedOfflineStorage].packs.count;
+    XCTAssert(countOfPacks > 0);
+
+    // Now delete packs one by one
+    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];
+                
+    NSArray *validPacks = [packs filteredArrayUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(id  _Nullable evaluatedObject, NSDictionary<NSString *,id> * _Nullable bindings) {
+        MGLOfflinePack *pack = (MGLOfflinePack*)evaluatedObject;
+        return pack.state != MGLOfflinePackStateInvalid;
+    }]];
+
+    NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
+    MGLTestAssertionHandler *newHandler = [[MGLTestAssertionHandler alloc] initWithTestCase:self];
+
+    [[[NSThread currentThread] threadDictionary] setValue:newHandler forKey:NSAssertionHandlerKey];
+
+    for (MGLOfflinePack *pack in validPacks) {
+        [storage removePack:pack withCompletionHandler:^(NSError * _Nullable error) {
+            [expectation fulfill];
+        }];
+    }
+    
+    [[[NSThread currentThread] threadDictionary] setValue:oldHandler forKey:NSAssertionHandlerKey];
+    
+    [self waitForExpectations:@[expectation] timeout:10.0];
+    
+    // TODO: What should we expect here? All packs removed?
+    
+    NSLog(@"Test `%@` complete", NSStringFromSelector(_cmd));
+}
+
+// Test to explore https://github.com/mapbox/mapbox-gl-native/issues/15536
+- (void)test15536RemovePacksOnBackgroundQueueWhileReloading {
+
+    [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"];
+    expectation.expectedFulfillmentCount = countOfPacks;
+    
+    MGLOfflineStorage *storage = [MGLOfflineStorage sharedOfflineStorage];
+    
+    // Simulate what happens the first time sharedOfflineStorage is accessed
+    [storage reloadPacks];
+
+//  NSArray *packs = [storage.packs copy];
+    
+    dispatch_async(queue, ^{
+        NSArray *packs = storage.packs;
+        NSAssertionHandler *oldHandler = [NSAssertionHandler currentHandler];
+        MGLTestAssertionHandler *newHandler = [[MGLTestAssertionHandler alloc] initWithTestCase:self];
+
+        [[[NSThread currentThread] threadDictionary] setValue:newHandler 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;
+        }]];
+        
+        for (MGLOfflinePack *pack in validPacks) {
+            // NOTE: pack can be invalid, as we have two threads potentially
+            // modifying the same MGLOfflinePack.
+            
+            dispatch_group_t group = dispatch_group_create();
+            dispatch_group_enter(group);
+            [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];
+    });
+    
+    [self waitForExpectations:@[expectation] timeout:60.0];
+    
+    // TODO: What should we expect here? All packs removed?
+    
+    NSLog(@"Test `%@` complete", NSStringFromSelector(_cmd));
+}
+
 - (void)testCountOfBytesCompleted {
     XCTAssertGreaterThan([MGLOfflineStorage sharedOfflineStorage].countOfBytesCompleted, 0UL);
 }
diff --git a/platform/darwin/test/MGLTestAssertionHandler.h b/platform/darwin/test/MGLTestAssertionHandler.h
new file mode 100644
index 0000000000..f1aa39921e
--- /dev/null
+++ b/platform/darwin/test/MGLTestAssertionHandler.h
@@ -0,0 +1,18 @@
+#import <Foundation/Foundation.h>
+#import <XCTest/XCTest.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+// Use to catch or log assertions that occur in dispatch blocks, timers or
+// other asynchronous operations.
+@interface MGLTestAssertionHandler : NSAssertionHandler
+
+- (instancetype)initWithTestCase:(XCTestCase *)testCase;
+@property (nonatomic, weak) XCTestCase *testCase;
+
+// If YES, use `_XCTPreformattedFailureHandler` to "fail" the test,
+// otherwise log the assert.
+@property (nonatomic) BOOL shouldFail;
+@end
+
+NS_ASSUME_NONNULL_END
diff --git a/platform/darwin/test/MGLTestAssertionHandler.m b/platform/darwin/test/MGLTestAssertionHandler.m
new file mode 100644
index 0000000000..4b504427b5
--- /dev/null
+++ b/platform/darwin/test/MGLTestAssertionHandler.m
@@ -0,0 +1,77 @@
+#import "MGLTestAssertionHandler.h"
+
+@implementation MGLTestAssertionHandler
+
+- (instancetype)initWithTestCase:(XCTestCase *)testCase {
+    if ((self = [super init])) {
+        _testCase = testCase;
+    }
+    return self;
+}
+
+- (void)handleFailureInMethod:(SEL)selector
+                       object:(id)object
+                         file:(NSString *)fileName
+                   lineNumber:(NSInteger)line
+                  description:(NSString *)format, ...
+{
+    va_list args;
+    va_start(args, format);
+    NSString *description = [[NSString alloc] initWithFormat:format arguments:args];
+    va_end(args);
+    
+    NSString *condition = [NSString stringWithFormat:
+                           @"`[%@ %@]`",
+                           object, NSStringFromSelector(selector)
+                           ];
+
+    if (self.testCase && self.shouldFail) {
+        _XCTPreformattedFailureHandler(self.testCase,
+                                       YES,
+                                       fileName,
+                                       line,
+                                       condition,
+                                       description
+                                       );
+    }
+    else {
+        NSLog(@"Assertion Failure: %@:%lu: %@ - %@",
+              fileName,
+              line,
+              condition,
+              description);
+    }
+}
+ 
+- (void)handleFailureInFunction:(NSString *)functionName
+                           file:(NSString *)fileName
+                     lineNumber:(NSInteger)line
+                    description:(NSString *)format, ...
+{
+    va_list args;
+    va_start(args, format);
+    NSString *description = [[NSString alloc] initWithFormat:format arguments:args];
+    va_end(args);
+
+    NSString *condition = [NSString stringWithFormat:
+                           @"`%@`",
+                           functionName];
+
+    if (self.testCase && self.shouldFail) {
+        _XCTPreformattedFailureHandler(self.testCase,
+                                       YES,
+                                       fileName,
+                                       line,
+                                       condition,
+                                       description);
+    }
+    else {
+        NSLog(@"Assertion Failure: %@:%lu: %@ - %@",
+              fileName,
+              line,
+              condition,
+              description);
+    }
+}
+@end
+