[ios, osx] Tightened offline task state

Fixed a race condition that could occur if a progress update comes after a call to remove the task but before the removal is complete. Avoid synchronously setting the task’s state inside -resume and -suspend; instead, rely on the observer to set the state asynchronously.

4 files changed, 43 insertions, 12 deletions

diff --git a/platform/darwin/src/MGLOfflineStorage.mm b/platform/darwin/src/MGLOfflineStorage.mm
index 274cef7688..b245273954 100644
--- a/platform/darwin/src/MGLOfflineStorage.mm
+++ b/platform/darwin/src/MGLOfflineStorage.mm
@@ -104,8 +104,9 @@
 }
 
 - (void)removeTask:(MGLOfflineTask *)task withCompletionHandler:(MGLOfflineTaskRemovalCompletionHandler)completion {
+    mbgl::OfflineRegion *mbglOfflineRegion = task.mbglOfflineRegion;
     [task invalidate];
-    self.mbglFileSource->deleteOfflineRegion(std::move(*task.mbglOfflineRegion), [&, task, completion](std::exception_ptr exception) {
+    self.mbglFileSource->deleteOfflineRegion(std::move(*mbglOfflineRegion), [&, completion](std::exception_ptr exception) {
         NSError *error;
         if (exception) {
             error = [NSError errorWithDomain:MGLErrorDomain code:-1 userInfo:@{
@@ -113,7 +114,7 @@
             }];
         }
         if (completion) {
-            dispatch_async(dispatch_get_main_queue(), [&, task, completion, error](void) {
+            dispatch_async(dispatch_get_main_queue(), [&, completion, error](void) {
                 completion(error);
             });
         }
diff --git a/platform/darwin/src/MGLOfflineTask.mm b/platform/darwin/src/MGLOfflineTask.mm
index 9f6aa3312b..4698a492fe 100644
--- a/platform/darwin/src/MGLOfflineTask.mm
+++ b/platform/darwin/src/MGLOfflineTask.mm
@@ -7,6 +7,12 @@
 #include <mbgl/storage/default_file_source.hpp>
 #include <mbgl/util/string.hpp>
 
+/**
+ Assert that the current offline task is valid.
+ 
+ This macro should be used at the beginning of any public-facing instance method
+ of `MGLOfflineTask`. For private methods, an assertion is more appropriate.
+ */
 #define MGLAssertOfflineTaskIsValid() \
     do { \
         if (_state == MGLOfflineTaskStateInvalid) { \
@@ -32,7 +38,7 @@ private:
 
 @interface MGLOfflineTask ()
 
-@property (nonatomic, readwrite) mbgl::OfflineRegion *mbglOfflineRegion;
+@property (nonatomic, nullable, readwrite) mbgl::OfflineRegion *mbglOfflineRegion;
 @property (nonatomic, readwrite) MGLOfflineTaskState state;
 @property (nonatomic, readwrite) MGLOfflineTaskProgress progress;
 
@@ -54,8 +60,7 @@ private:
         _state = MGLOfflineTaskStateUnknown;
         
         mbgl::DefaultFileSource *mbglFileSource = [[MGLOfflineStorage sharedOfflineStorage] mbglFileSource];
-        MBGLOfflineRegionObserver *mbglObserver = new MBGLOfflineRegionObserver(self);
-        mbglFileSource->setOfflineRegionObserver(*_mbglOfflineRegion, std::make_unique<MBGLOfflineRegionObserver>(*mbglObserver));
+        mbglFileSource->setOfflineRegionObserver(*_mbglOfflineRegion, std::make_unique<MBGLOfflineRegionObserver>(self));
     }
     return self;
 }
@@ -85,7 +90,6 @@ private:
     
     mbgl::DefaultFileSource *mbglFileSource = [[MGLOfflineStorage sharedOfflineStorage] mbglFileSource];
     mbglFileSource->setOfflineRegionDownloadState(*_mbglOfflineRegion, mbgl::OfflineRegionDownloadState::Active);
-    self.state = MGLOfflineTaskStateActive;
 }
 
 - (void)suspend {
@@ -93,13 +97,27 @@ private:
     
     mbgl::DefaultFileSource *mbglFileSource = [[MGLOfflineStorage sharedOfflineStorage] mbglFileSource];
     mbglFileSource->setOfflineRegionDownloadState(*_mbglOfflineRegion, mbgl::OfflineRegionDownloadState::Inactive);
-    self.state = MGLOfflineTaskStateInactive;
 }
 
 - (void)invalidate {
-    MGLAssertOfflineTaskIsValid();
+    NSAssert(_state != MGLOfflineTaskStateInvalid, @"Cannot invalidate an already invalid offline task.");
     
     self.state = MGLOfflineTaskStateInvalid;
+    self.mbglOfflineRegion = nil;
+}
+
+- (void)setState:(MGLOfflineTaskState)state {
+    if (!self.mbglOfflineRegion) {
+        // A progress update has arrived after the call to
+        // -[MGLOfflineStorage removeTask:withCompletionHandler:] but before the
+        // removal is complete and the completion handler is called.
+        NSAssert(_state == MGLOfflineTaskStateInvalid, @"A valid MGLOfflineTask has no mbgl::OfflineRegion.");
+        return;
+    }
+    
+    NSAssert(_state != MGLOfflineTaskStateInvalid, @"Cannot change the state of an invalid offline task.");
+    
+    _state = state;
 }
 
 - (void)requestProgress {
@@ -120,7 +138,7 @@ private:
 }
 
 - (void)offlineRegionStatusDidChange:(mbgl::OfflineRegionStatus)status {
-    MGLAssertOfflineTaskIsValid();
+    NSAssert(_state != MGLOfflineTaskStateInvalid, @"Cannot change update progress of an invalid offline task.");
     
     switch (status.downloadState) {
         case mbgl::OfflineRegionDownloadState::Inactive:
diff --git a/platform/darwin/src/MGLOfflineTask_Private.h b/platform/darwin/src/MGLOfflineTask_Private.h
index 1c1ff59b84..7a425d7647 100644
--- a/platform/darwin/src/MGLOfflineTask_Private.h
+++ b/platform/darwin/src/MGLOfflineTask_Private.h
@@ -2,12 +2,20 @@
 
 #include <mbgl/storage/default_file_source.hpp>
 
+NS_ASSUME_NONNULL_BEGIN
+
 @interface MGLOfflineTask (Private)
 
-@property (nonatomic) mbgl::OfflineRegion *mbglOfflineRegion;
+@property (nonatomic, nullable) mbgl::OfflineRegion *mbglOfflineRegion;
 
 - (instancetype)initWithMBGLRegion:(mbgl::OfflineRegion *)region;
 
+/**
+ Invalidates the task and ensures that no future progress update can ever
+ revalidate it.
+ */
 - (void)invalidate;
 
 @end
+
+NS_ASSUME_NONNULL_END
diff --git a/platform/darwin/src/MGLTilePyramidOfflineRegion.mm b/platform/darwin/src/MGLTilePyramidOfflineRegion.mm
index 3d51917c3e..6507ceef77 100644
--- a/platform/darwin/src/MGLTilePyramidOfflineRegion.mm
+++ b/platform/darwin/src/MGLTilePyramidOfflineRegion.mm
@@ -33,8 +33,12 @@
         }
         
         if (!styleURL.scheme) {
-            // Assume a relative path into the application bundle.
-            styleURL = [NSURL URLWithString:[@"asset://" stringByAppendingString:styleURL.absoluteString]];
+            [NSException raise:@"Invalid style URL" format:
+             @"%@ does not support setting a relative file URL as the style URL. "
+             @"To download the online resources required by this style, "
+             @"specify a URL to an online copy of this style. "
+             @"For Mapbox-hosted styles, use the mapbox: scheme.",
+             NSStringFromClass([self class])];
         }
         
         _styleURL = styleURL;