From 2b08c1d7bf9e867d6450feb6a246ce36dcdf2cd3 Mon Sep 17 00:00:00 2001 From: Julian Rex Date: Wed, 18 Sep 2019 18:13:07 -0400 Subject: [ios, macos] Fix MGLOfflinePack invalidate crash (#15582) --- platform/darwin/src/MGLOfflinePack.mm | 11 +- platform/darwin/src/MGLOfflineStorage.mm | 4 + platform/darwin/test/MGLOfflinePackTests.m | 40 ----- platform/darwin/test/MGLOfflinePackTests.mm | 58 ++++++++ platform/darwin/test/MGLOfflineStorageTests.mm | 198 ++++++++++++++++++++++++- platform/darwin/test/MGLTestAssertionHandler.h | 18 +++ platform/darwin/test/MGLTestAssertionHandler.m | 77 ++++++++++ 7 files changed, 360 insertions(+), 46 deletions(-) delete mode 100644 platform/darwin/test/MGLOfflinePackTests.m create mode 100644 platform/darwin/test/MGLOfflinePackTests.mm create mode 100644 platform/darwin/test/MGLTestAssertionHandler.h create mode 100644 platform/darwin/test/MGLTestAssertionHandler.m (limited to 'platform/darwin') 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.m deleted file mode 100644 index a33665ff0a..0000000000 --- a/platform/darwin/test/MGLOfflinePackTests.m +++ /dev/null @@ -1,40 +0,0 @@ -#import -#import - -@interface MGLOfflinePackTests : XCTestCase - -@end - -@implementation MGLOfflinePackTests - -- (void)testInvalidation { - MGLOfflinePack *invalidPack = [[MGLOfflinePack alloc] init]; - - XCTAssertEqual(invalidPack.state, MGLOfflinePackStateInvalid, @"Offline pack should be invalid when initialized independently of MGLOfflineStorage."); - - XCTAssertThrowsSpecificNamed(invalidPack.region, NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when accessing its region."); - XCTAssertThrowsSpecificNamed(invalidPack.context, NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when accessing its context."); - XCTAssertThrowsSpecificNamed([invalidPack resume], NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when being resumed."); - XCTAssertThrowsSpecificNamed([invalidPack suspend], NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when being suspended."); -} - -- (void)testProgressBoxing { - MGLOfflinePackProgress progress = { - .countOfResourcesCompleted = 3, - .countOfResourcesExpected = 2, - .countOfBytesCompleted = 7, - .countOfTilesCompleted = 1, - .countOfTileBytesCompleted = 6, - .maximumResourcesExpected = UINT64_MAX, - }; - MGLOfflinePackProgress roundTrippedProgress = [NSValue valueWithMGLOfflinePackProgress:progress].MGLOfflinePackProgressValue; - - XCTAssertEqual(progress.countOfResourcesCompleted, roundTrippedProgress.countOfResourcesCompleted, @"Completed resources should round-trip."); - XCTAssertEqual(progress.countOfResourcesExpected, roundTrippedProgress.countOfResourcesExpected, @"Expected resources should round-trip."); - XCTAssertEqual(progress.countOfBytesCompleted, roundTrippedProgress.countOfBytesCompleted, @"Completed bytes should round-trip."); - XCTAssertEqual(progress.countOfTilesCompleted, roundTrippedProgress.countOfTilesCompleted, @"Completed tiles should round-trip."); - XCTAssertEqual(progress.countOfTileBytesCompleted, roundTrippedProgress.countOfTileBytesCompleted, @"Completed tile bytes should round-trip."); - XCTAssertEqual(progress.maximumResourcesExpected, roundTrippedProgress.maximumResourcesExpected, @"Maximum expected resources should round-trip."); -} - -@end diff --git a/platform/darwin/test/MGLOfflinePackTests.mm b/platform/darwin/test/MGLOfflinePackTests.mm new file mode 100644 index 0000000000..6b454ee8ca --- /dev/null +++ b/platform/darwin/test/MGLOfflinePackTests.mm @@ -0,0 +1,58 @@ +#import +#import +#import "MGLOfflinePack_Private.h" +#import "MGLTestAssertionHandler.h" + +@interface MGLOfflinePackTests : XCTestCase + +@end + +@implementation MGLOfflinePackTests + +- (void)testInvalidation { + MGLOfflinePack *invalidPack = [[MGLOfflinePack alloc] init]; + + XCTAssertEqual(invalidPack.state, MGLOfflinePackStateInvalid, @"Offline pack should be invalid when initialized independently of MGLOfflineStorage."); + + XCTAssertThrowsSpecificNamed(invalidPack.region, NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when accessing its region."); + XCTAssertThrowsSpecificNamed(invalidPack.context, NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when accessing its context."); + XCTAssertThrowsSpecificNamed([invalidPack resume], NSException, MGLInvalidOfflinePackException, @"Invalid offline pack should raise an exception when being resumed."); + 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, + .countOfResourcesExpected = 2, + .countOfBytesCompleted = 7, + .countOfTilesCompleted = 1, + .countOfTileBytesCompleted = 6, + .maximumResourcesExpected = UINT64_MAX, + }; + MGLOfflinePackProgress roundTrippedProgress = [NSValue valueWithMGLOfflinePackProgress:progress].MGLOfflinePackProgressValue; + + XCTAssertEqual(progress.countOfResourcesCompleted, roundTrippedProgress.countOfResourcesCompleted, @"Completed resources should round-trip."); + XCTAssertEqual(progress.countOfResourcesExpected, roundTrippedProgress.countOfResourcesExpected, @"Expected resources should round-trip."); + XCTAssertEqual(progress.countOfBytesCompleted, roundTrippedProgress.countOfBytesCompleted, @"Completed bytes should round-trip."); + XCTAssertEqual(progress.countOfTilesCompleted, roundTrippedProgress.countOfTilesCompleted, @"Completed tiles should round-trip."); + XCTAssertEqual(progress.countOfTileBytesCompleted, roundTrippedProgress.countOfTileBytesCompleted, @"Completed tile bytes should round-trip."); + XCTAssertEqual(progress.maximumResourcesExpected, roundTrippedProgress.maximumResourcesExpected, @"Maximum expected resources should round-trip."); +} + +@end 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 +#import #import "MGLOfflineStorage_Private.h" #import "NSBundle+MGLAdditions.h" #import "NSDate+MGLAdditions.h" - -#import +#import "MGLTestAssertionHandler.h" #include #pragma clang diagnostic ignored "-Wshadow" + @interface MGLOfflineStorageTests : XCTestCase @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([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 * _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 * _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 +#import + +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 + -- cgit v1.2.1