Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g commented Oct 30, 2023

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 awaits 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 flutter/flutter#108900

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

Copy link
Contributor

@vashworth vashworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this!

@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 31, 2023
@auto-submit auto-submit bot merged commit c9fec61 into flutter:main Oct 31, 2023
@stuartmorgan-g stuartmorgan-g deleted the image-picker-ios-multi-present-crash branch October 31, 2023 14:52
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 31, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 31, 2023
flutter/packages@2af6954...c9fec61

2023-10-31 [email protected] [image_picker] Prevent multiple active calls on iOS (flutter/packages#5272)
2023-10-30 [email protected] [in_app_purchase_android] Add missing response code to BillingResponse enum (flutter/packages#5120)
2023-10-30 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.4 to 2.22.5 (flutter/packages#5263)
2023-10-30 [email protected] Bump play-service-maps dependency from version 18.1.0 to 18.2.0 (flutter/packages#5243)
2023-10-30 [email protected] Roll Flutter from a4ec627 to e12d1a7 (3 revisions) (flutter/packages#5267)
2023-10-30 [email protected] [camerax] Loosen restrictions of fallback strategies for choosing resolutions (flutter/packages#5239)
2023-10-30 [email protected] [pigeon] Don't wrap non-nullable primitives in Obj-C (flutter/packages#5214)
2023-10-30 [email protected] [flutter_lints] Replace "flutter pub add --dev" with "dev:" (flutter/packages#5260)
2023-10-28 [email protected] Roll Flutter from 5907c97 to a4ec627 (18 revisions) (flutter/packages#5255)
2023-10-27 [email protected] [go_router] Fixes deep-link with no path on cold start (flutter/packages#5113)
2023-10-27 [email protected] [two_dimensional_scrollables] Fix pinned row painting when one axis is reversed (flutter/packages#5187)
2023-10-27 [email protected] Roll Flutter from c555599 to 5907c97 (45 revisions) (flutter/packages#5252)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
ksokolovskyi pushed a commit to ksokolovskyi/flutter that referenced this pull request Oct 31, 2023
flutter/packages@2af6954...c9fec61

2023-10-31 [email protected] [image_picker] Prevent multiple active calls on iOS (flutter/packages#5272)
2023-10-30 [email protected] [in_app_purchase_android] Add missing response code to BillingResponse enum (flutter/packages#5120)
2023-10-30 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.4 to 2.22.5 (flutter/packages#5263)
2023-10-30 [email protected] Bump play-service-maps dependency from version 18.1.0 to 18.2.0 (flutter/packages#5243)
2023-10-30 [email protected] Roll Flutter from a4ec627 to e12d1a7 (3 revisions) (flutter/packages#5267)
2023-10-30 [email protected] [camerax] Loosen restrictions of fallback strategies for choosing resolutions (flutter/packages#5239)
2023-10-30 [email protected] [pigeon] Don't wrap non-nullable primitives in Obj-C (flutter/packages#5214)
2023-10-30 [email protected] [flutter_lints] Replace "flutter pub add --dev" with "dev:" (flutter/packages#5260)
2023-10-28 [email protected] Roll Flutter from 5907c97 to a4ec627 (18 revisions) (flutter/packages#5255)
2023-10-27 [email protected] [go_router] Fixes deep-link with no path on cold start (flutter/packages#5113)
2023-10-27 [email protected] [two_dimensional_scrollables] Fix pinned row painting when one axis is reversed (flutter/packages#5187)
2023-10-27 [email protected] Roll Flutter from c555599 to 5907c97 (45 revisions) (flutter/packages#5252)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@reidbaker reidbaker mentioned this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: image_picker platform-ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[image_picker][iOS] Occasional crash, maybe due to multiple modal viewControllers being presented

2 participants