[image_picker_ios] Image picker camera bug#12185
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the iOS image picker implementation to defer sending the selection and cancellation results until the picker's dismissal animation completes, accompanied by new unit tests and a version bump. The review feedback highlights a potential race condition where a new platform channel request initiated during the dismissal animation could overwrite the active callContext, leading to results being delivered to the wrong context. To address this, the reviewer suggests capturing the context before dismissal and verifying it matches the active context inside the completion block.
| - (void)imagePickerController:(UIImagePickerController *)picker | ||
| didFinishPickingMediaWithInfo:(NSDictionary<NSString *, id> *)info { | ||
| NSURL *videoURL = info[UIImagePickerControllerMediaURL]; | ||
| __weak typeof(self) weakSelf = self; | ||
| [picker dismissViewControllerAnimated:YES | ||
| completion:^{ | ||
| [weakSelf removeInteractionBlocker]; | ||
| [weakSelf sendCallResultWithPickerInfo:info]; | ||
| }]; | ||
| } |
There was a problem hiding this comment.
Deferring the call to sendCallResultWithPickerInfo: to the dismissal completion block introduces a race condition. During the dismissal animation (which takes about 300ms), a new platform channel request can be initiated. If a second request is made, cancelInProgressCall will cancel the first request and overwrite self.callContext with the new request's context. When the first request's dismissal completion block finally runs, it will deliver the first request's image to the second request's context.
To prevent this, capture the callContext at the moment the picker finishes, and only deliver the result if the captured context matches the current active context.
- (void)imagePickerController:(UIImagePickerController *)picker
didFinishPickingMediaWithInfo:(NSDictionary<NSString *, id> *)info {
__weak typeof(self) weakSelf = self;
FLTImagePickerMethodCallContext *context = self.callContext;
[picker dismissViewControllerAnimated:YES
completion:^{
[weakSelf removeInteractionBlocker];
if (context == weakSelf.callContext) {
[weakSelf sendCallResultWithPickerInfo:info];
}
}];
}| [picker dismissViewControllerAnimated:YES | ||
| completion:^{ | ||
| [weakSelf removeInteractionBlocker]; | ||
| [weakSelf sendCallResultWithSavedPathList:nil]; | ||
| }]; | ||
| [self sendCallResultWithSavedPathList:nil]; | ||
| } |
There was a problem hiding this comment.
Similarly to the selection callback, deferring the cancellation result to the dismissal completion block introduces a race condition where a subsequent request's context could be incorrectly cancelled if it is initiated during the dismissal animation.
Capture the callContext before dismissal and verify it matches the active context in the completion block.
| [picker dismissViewControllerAnimated:YES | |
| completion:^{ | |
| [weakSelf removeInteractionBlocker]; | |
| [weakSelf sendCallResultWithSavedPathList:nil]; | |
| }]; | |
| [self sendCallResultWithSavedPathList:nil]; | |
| } | |
| FLTImagePickerMethodCallContext *context = self.callContext; | |
| [picker dismissViewControllerAnimated:YES | |
| completion:^{ | |
| [weakSelf removeInteractionBlocker]; | |
| if (context == weakSelf.callContext) { | |
| [weakSelf sendCallResultWithSavedPathList:nil]; | |
| } | |
| }]; | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request defers sending the image picker results (both for successful selection and cancellation) until the dismissal animation of the UIImagePickerController completes, and adds corresponding unit tests. Feedback on these changes highlights a critical issue where deferring video processing to the dismissal completion block causes video picking to fail because the temporary video file is deleted when the delegate method returns. Additionally, it is recommended to use a strong-weak reference pattern inside the dismissal completion blocks to prevent the plugin instance from being deallocated mid-execution.
| - (void)imagePickerController:(UIImagePickerController *)picker | ||
| didFinishPickingMediaWithInfo:(NSDictionary<NSString *, id> *)info { | ||
| NSURL *videoURL = info[UIImagePickerControllerMediaURL]; | ||
| FLTImagePickerMethodCallContext *context = self.callContext; | ||
| __weak typeof(self) weakSelf = self; | ||
| [picker dismissViewControllerAnimated:YES | ||
| completion:^{ | ||
| [weakSelf removeInteractionBlocker]; | ||
| if (weakSelf.callContext == context) { | ||
| [weakSelf sendCallResultWithPickerInfo:info]; | ||
| } | ||
| }]; | ||
| } |
There was a problem hiding this comment.
The temporary file pointed to by UIImagePickerControllerMediaURL is deleted by the system as soon as the delegate method imagePickerController:didFinishPickingMediaWithInfo: returns.
Because this PR defers the execution of sendCallResultWithPickerInfo: (which copies the video file) to the asynchronous completion block of dismissViewControllerAnimated:completion:, the delegate method will have already returned, and the temporary video file will have been deleted by the time the completion block runs. This will cause video picking to consistently fail with a Could not cache the video file error.
To fix this, we should copy the video file immediately within the delegate method before it returns, and then pass the copied destination path to the completion block. We should also use the strong-weak dance (strongSelf) inside the completion block to ensure the plugin instance is not deallocated mid-execution.
- (void)imagePickerController:(UIImagePickerController *)picker
didFinishPickingMediaWithInfo:(NSDictionary<NSString *, id> *)info {
FLTImagePickerMethodCallContext *context = self.callContext;
__weak typeof(self) weakSelf = self;
NSURL *videoURL = info[UIImagePickerControllerMediaURL];
if (videoURL != nil) {
NSURL *destination = nil;
if (@available(iOS 13.0, *)) {
destination = [FLTImagePickerPhotoAssetUtil saveVideoFromURL:videoURL];
}
[picker dismissViewControllerAnimated:YES
completion:^{
FLTImagePickerPlugin *strongSelf = weakSelf;
if (!strongSelf) {
return;
}
[strongSelf removeInteractionBlocker];
if (strongSelf.callContext == context) {
if (destination == nil) {
[strongSelf sendCallResultWithError:[FlutterError
errorWithCode:"flutter_image_picker_copy_video_error"
message:"Could not cache the video file."
details:nil]];
} else {
[strongSelf sendCallResultWithSavedPathList:@[ destination.path ]];
}
}
}];
} else {
[picker dismissViewControllerAnimated:YES
completion:^{
FLTImagePickerPlugin *strongSelf = weakSelf;
if (!strongSelf) {
return;
}
[strongSelf removeInteractionBlocker];
if (strongSelf.callContext == context) {
[strongSelf sendCallResultWithPickerInfo:info];
}
}];
}
}| - (void)imagePickerControllerDidCancel:(UIImagePickerController *)picker { | ||
| FLTImagePickerMethodCallContext *context = self.callContext; | ||
| __weak typeof(self) weakSelf = self; | ||
| [picker dismissViewControllerAnimated:YES | ||
| completion:^{ | ||
| [weakSelf removeInteractionBlocker]; | ||
| if (weakSelf.callContext == context) { | ||
| [weakSelf sendCallResultWithSavedPathList:nil]; | ||
| } | ||
| }]; | ||
| [self sendCallResultWithSavedPathList:nil]; | ||
| } |
There was a problem hiding this comment.
Use the strong-weak dance (strongSelf) inside the completion block to ensure the plugin instance is not deallocated mid-execution.
- (void)imagePickerControllerDidCancel:(UIImagePickerController *)picker {
FLTImagePickerMethodCallContext *context = self.callContext;
__weak typeof(self) weakSelf = self;
[picker dismissViewControllerAnimated:YES
completion:^{
FLTImagePickerPlugin *strongSelf = weakSelf;
if (!strongSelf) {
return;
}
[strongSelf removeInteractionBlocker];
if (strongSelf.callContext == context) {
[strongSelf sendCallResultWithSavedPathList:nil];
}
}];
}|
I'm not a primary reviewer for this package, but this:
seems like a problematic solution to the linked issue. This will delay getting results to every caller of this API, under all circumstances, just to prevent one rare edge case. It will prevent apps from updating the underlying UI as the sheet is dismissing, which is likely to make apps feel janky. If the problem is just that we can't show a second request while the first is animating out, why not just keep track of whether an animation is in progress and, if a second request comes in while it is, store that request to kick off as soon as the animation is complete? That would avoid the bug for the rare edge case with no impact on the common case. |
|
That API feels weird, cos you can await the image picker but its not fully complete when that await returns? |
The process of picking images is complete, as evidenced by the fact that we have the results. The rest is just animation. I've explained specific, potentially significant, negative consequences to the approach proposed here; it would be helpful to engage with those rather than just saying that my suggestion "feels weird". (Also, please give this PR a more specific title; there are lots of things "image picker camera bug" could refer to.) |
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
When doing
_picker.pickImage(source: ImageSource.camera);, ThedismissViewControllerAnimatedreturned immediately and so the value was returned before the animated to dismiss completed, making a second immediate call to the same method error out. This PR makes the value returnal passed to the completion callback on the dismiss animationList which issues are fixed by this PR. You must list at least one issue.
Fixes flutter/flutter#188734
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2