From c9fec61b6cdea5dc912575337e336c86dc32af8c Mon Sep 17 00:00:00 2001 From: stuartmorgan Date: Tue, 31 Oct 2023 07:52:04 -0700 Subject: [PATCH] [image_picker] Prevent multiple active calls on iOS (#5272) The image picker plugin's implementation doesn't currently handle multiple calls correctly due to the use of an ivar to track the response object; the original entry point handles that by cancelling earlier requests when new ones come in, but as we added more entry points we didn't replicate that logic. This adds it to all picker entry points. (Longer term, we should instead handle multiple concurrent calls, but this is consistent with historical behavior, and is better than having some `await`s on the Dart side never return as can happen now.) The newer PHPicker code path not only didn't cancel, but used an ivar for the picker view controller, which in some cases could result in the same controller being presented multiple times, crashing the app (see referenced issue). While the new cancel calls will prevent that case from happening, to prevent anything similar from happening in the future this removes the ivar entirely, since we can just pass the controller to the necessary methods (as is already being done with the `UIImagePickerController` paths). Fixes https://github.com/flutter/flutter/issues/108900 --- .../image_picker_ios/CHANGELOG.md | 4 + .../ios/RunnerTests/ImagePickerPluginTests.m | 79 +++++++++++++++++++ .../ios/Classes/FLTImagePickerPlugin.m | 33 ++++---- .../image_picker_ios/pubspec.yaml | 2 +- 4 files changed, 98 insertions(+), 20 deletions(-) diff --git a/packages/image_picker/image_picker_ios/CHANGELOG.md b/packages/image_picker/image_picker_ios/CHANGELOG.md index 158a881134..83b953a7ef 100644 --- a/packages/image_picker/image_picker_ios/CHANGELOG.md +++ b/packages/image_picker/image_picker_ios/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.8.8+3 + +* Fixes a possible crash when calling a picker method while another is waiting on permissions. + ## 0.8.8+2 * Adds pub topics to package metadata. diff --git a/packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m b/packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m index 3ccca17f2e..4f22c319a7 100644 --- a/packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m +++ b/packages/image_picker/image_picker_ios/example/ios/RunnerTests/ImagePickerPluginTests.m @@ -531,4 +531,83 @@ [self waitForExpectationsWithTimeout:30 handler:nil]; } +- (void)testPickMultiImageDuplicateCallCancels API_AVAILABLE(ios(14)) { + id mockPhotoLibrary = OCMClassMock([PHPhotoLibrary class]); + OCMStub([mockPhotoLibrary authorizationStatusForAccessLevel:PHAccessLevelReadWrite]) + .andReturn(PHAuthorizationStatusNotDetermined); + OCMExpect([mockPhotoLibrary requestAuthorizationForAccessLevel:PHAccessLevelReadWrite + handler:OCMOCK_ANY]); + + FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; + + XCTestExpectation *firstCallExpectation = [self expectationWithDescription:@"first call"]; + [plugin pickMultiImageWithMaxSize:[FLTMaxSize makeWithWidth:@100 height:@100] + quality:nil + fullMetadata:@YES + completion:^(NSArray *result, FlutterError *error) { + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.code, @"multiple_request"); + [firstCallExpectation fulfill]; + }]; + [plugin pickMultiImageWithMaxSize:[FLTMaxSize makeWithWidth:@100 height:@100] + quality:nil + fullMetadata:@YES + completion:^(NSArray *result, FlutterError *error){ + }]; + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +- (void)testPickMediaDuplicateCallCancels API_AVAILABLE(ios(14)) { + id mockPhotoLibrary = OCMClassMock([PHPhotoLibrary class]); + OCMStub([mockPhotoLibrary authorizationStatusForAccessLevel:PHAccessLevelReadWrite]) + .andReturn(PHAuthorizationStatusNotDetermined); + OCMExpect([mockPhotoLibrary requestAuthorizationForAccessLevel:PHAccessLevelReadWrite + handler:OCMOCK_ANY]); + + FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; + + FLTMediaSelectionOptions *options = + [FLTMediaSelectionOptions makeWithMaxSize:[FLTMaxSize makeWithWidth:@(100) height:@(200)] + imageQuality:@(50) + requestFullMetadata:@YES + allowMultiple:@YES]; + XCTestExpectation *firstCallExpectation = [self expectationWithDescription:@"first call"]; + [plugin pickMediaWithMediaSelectionOptions:options + completion:^(NSArray *result, FlutterError *error) { + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.code, @"multiple_request"); + [firstCallExpectation fulfill]; + }]; + [plugin pickMediaWithMediaSelectionOptions:options + completion:^(NSArray *result, FlutterError *error){ + }]; + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + +- (void)testPickVideoDuplicateCallCancels API_AVAILABLE(ios(14)) { + id mockPhotoLibrary = OCMClassMock([PHPhotoLibrary class]); + OCMStub([mockPhotoLibrary authorizationStatusForAccessLevel:PHAccessLevelReadWrite]) + .andReturn(PHAuthorizationStatusNotDetermined); + OCMExpect([mockPhotoLibrary requestAuthorizationForAccessLevel:PHAccessLevelReadWrite + handler:OCMOCK_ANY]); + + FLTImagePickerPlugin *plugin = [[FLTImagePickerPlugin alloc] init]; + + FLTSourceSpecification *source = [FLTSourceSpecification makeWithType:FLTSourceTypeCamera + camera:FLTSourceCameraRear]; + XCTestExpectation *firstCallExpectation = [self expectationWithDescription:@"first call"]; + [plugin pickVideoWithSource:source + maxDuration:nil + completion:^(NSString *result, FlutterError *error) { + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.code, @"multiple_request"); + [firstCallExpectation fulfill]; + }]; + [plugin pickVideoWithSource:source + maxDuration:nil + completion:^(NSString *result, FlutterError *error){ + }]; + [self waitForExpectationsWithTimeout:30 handler:nil]; +} + @end diff --git a/packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m b/packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m index 65120c4254..6a4fde82bb 100644 --- a/packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m +++ b/packages/image_picker/image_picker_ios/ios/Classes/FLTImagePickerPlugin.m @@ -31,12 +31,6 @@ @interface FLTImagePickerPlugin () -/** - * The PHPickerViewController instance used to pick multiple - * images. - */ -@property(strong, nonatomic) PHPickerViewController *pickerViewController API_AVAILABLE(ios(14)); - /** * The UIImagePickerController instances that will be used when a new * controller would normally be created. Each call to @@ -117,15 +111,16 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic config.filter = [PHPickerFilter imagesFilter]; } - _pickerViewController = [[PHPickerViewController alloc] initWithConfiguration:config]; - _pickerViewController.delegate = self; - _pickerViewController.presentationController.delegate = self; + PHPickerViewController *pickerViewController = + [[PHPickerViewController alloc] initWithConfiguration:config]; + pickerViewController.delegate = self; + pickerViewController.presentationController.delegate = self; self.callContext = context; if (context.requestFullMetadata) { - [self checkPhotoAuthorizationForAccessLevel]; + [self checkPhotoAuthorizationWithPHPicker:pickerViewController]; } else { - [self showPhotoLibraryWithPHPicker:_pickerViewController]; + [self showPhotoLibraryWithPHPicker:pickerViewController]; } } @@ -201,6 +196,7 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic fullMetadata:(NSNumber *)fullMetadata completion:(nonnull void (^)(NSArray *_Nullable, FlutterError *_Nullable))completion { + [self cancelInProgressCall]; FLTImagePickerMethodCallContext *context = [[FLTImagePickerMethodCallContext alloc] initWithResult:completion]; context.maxSize = maxSize; @@ -220,6 +216,7 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic - (void)pickMediaWithMediaSelectionOptions:(nonnull FLTMediaSelectionOptions *)mediaSelectionOptions completion:(nonnull void (^)(NSArray *_Nullable, FlutterError *_Nullable))completion { + [self cancelInProgressCall]; FLTImagePickerMethodCallContext *context = [[FLTImagePickerMethodCallContext alloc] initWithResult:completion]; context.maxSize = [mediaSelectionOptions maxSize]; @@ -244,6 +241,7 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic maxDuration:(nullable NSNumber *)maxDurationSeconds completion: (nonnull void (^)(NSString *_Nullable, FlutterError *_Nullable))completion { + [self cancelInProgressCall]; FLTImagePickerMethodCallContext *context = [[FLTImagePickerMethodCallContext alloc] initWithResult:^void(NSArray *paths, FlutterError *error) { if (paths.count > 1) { @@ -393,7 +391,8 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic } } -- (void)checkPhotoAuthorizationForAccessLevel API_AVAILABLE(ios(14)) { +- (void)checkPhotoAuthorizationWithPHPicker:(PHPickerViewController *)pickerViewController + API_AVAILABLE(ios(14)) { PHAccessLevel requestedAccessLevel = PHAccessLevelReadWrite; PHAuthorizationStatus status = [PHPhotoLibrary authorizationStatusForAccessLevel:requestedAccessLevel]; @@ -404,13 +403,9 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic handler:^(PHAuthorizationStatus status) { dispatch_async(dispatch_get_main_queue(), ^{ if (status == PHAuthorizationStatusAuthorized) { - [self - showPhotoLibraryWithPHPicker:self-> - _pickerViewController]; + [self showPhotoLibraryWithPHPicker:pickerViewController]; } else if (status == PHAuthorizationStatusLimited) { - [self - showPhotoLibraryWithPHPicker:self-> - _pickerViewController]; + [self showPhotoLibraryWithPHPicker:pickerViewController]; } else { [self errorNoPhotoAccess:status]; } @@ -420,7 +415,7 @@ typedef NS_ENUM(NSInteger, ImagePickerClassType) { UIImagePickerClassType, PHPic } case PHAuthorizationStatusAuthorized: case PHAuthorizationStatusLimited: - [self showPhotoLibraryWithPHPicker:_pickerViewController]; + [self showPhotoLibraryWithPHPicker:pickerViewController]; break; case PHAuthorizationStatusDenied: case PHAuthorizationStatusRestricted: diff --git a/packages/image_picker/image_picker_ios/pubspec.yaml b/packages/image_picker/image_picker_ios/pubspec.yaml index 6b71c36d5b..121dedc41f 100755 --- a/packages/image_picker/image_picker_ios/pubspec.yaml +++ b/packages/image_picker/image_picker_ios/pubspec.yaml @@ -2,7 +2,7 @@ name: image_picker_ios description: iOS implementation of the image_picker plugin. repository: https://github.com/flutter/packages/tree/main/packages/image_picker/image_picker_ios issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+image_picker%22 -version: 0.8.8+2 +version: 0.8.8+3 environment: sdk: ">=2.19.0 <4.0.0"