[camera] Remove `@throw` from iOS implementation #5034

stuartmorgan · 2023-09-29T14:44:20Z

Using @throw in iOS code violates the style guide, so it shouldn't be done in the plugin as mechanism for communicating errors. More importantly, NSError is not intended to be used with @throw/@catch, and is causing issues when compiled with the iOS 17 SDK.

This removes all use of @throw, and all @catch (NSError* e), in favor of other methods of communicating errors. It explicitly does not try to fix all the other strange things about this code (having an NSError out-param in an init method, using Cocoa and NSURL error domains and codes for some reason), and instead preserves existing behavior as much as possible. In practice, none of these codepaths should ever actually happen (they indicate programming errors within the plugin, not unexpected runtime behavior), and all of this code will go away when converting to Pigeon anyway, so there's not much value in trying to unwind this structure further.

Fixes flutter/flutter#135195

Pre-launch Checklist

stuartmorgan · 2023-09-29T14:45:35Z

packages/camera/camera_avfoundation/ios/Classes/FLTCam.m

+                 NSLocalizedDescriptionKey :
+                     [NSString stringWithFormat:@"Unknown resolution preset %@", resolutionPreset]
+               }];
+    return nil;


This is technically a behavioral change since not returning nil on error was clearly wrong, but also this should never be able to happen anyway.

stuartmorgan · 2023-09-29T14:47:35Z

packages/camera/camera_avfoundation/ios/Classes/FLTCam.m

@@ -162,7 +168,9 @@ - (instancetype)initWithCameraName:(NSString *)cameraName
  _motionManager = [[CMMotionManager alloc] init];
  [_motionManager startAccelerometerUpdates];

-  [self setCaptureSessionPreset:_resolutionPreset];
+  if (![self setCaptureSessionPreset:_resolutionPreset withError:error]) {


This also technically a behavioral change in that now the error actually gets returned instead of us having an unhandled @throw of an NSError, which... I don't even actually know what that would do. Crash the app probably?

But as above, this should not actually be a thing that can happen as far as I can tell.

stuartmorgan · 2023-09-29T14:48:52Z

packages/camera/camera_avfoundation/ios/Classes/FLTCam.m

-                                  @"No capture session available for current capture session."
-                            }];
-        @throw error;
+        *error = [NSError errorWithDomain:NSCocoaErrorDomain


All of these NSErrors are directly moved from the code that was @throwing them, to preserve behavior. As noted in the PR description, I'm deliberately not changing the weird domain and code used in these errors for now.

This was some deeply strange Obj-C code.

Using `@throw` in iOS code violates the style guide, so it shouldn't be done in the plugin as mechanism for communicating errors. More importantly, `NSError` is not intended to be used with `@throw`/`@catch`, and is causing issues when compiled with the iOS 17 SDK. This removes all use of `@throw`, and all `@catch (NSError* e)`, in favor of other methods of communicating errors. It explicitly does not try to fix all the other strange things about this code (having an `NSError` out-param in an init method, using Cocoa and NSURL error domains and codes for some reason), and instead preserves existing behavior as much as possible. In practice, none of these codepaths should ever actually happen (they indicate programming errors within the plugin, not unexpected runtime behavior), and all of this code will go away when converting to Pigeon anyway, so there's not much value in trying to unwind this structure further. Fixes flutter/flutter#135195

hellohuanlin

I saw these last year when dry-running a swift migration on camera plugin. Thanks for fixing - it will make future migration easier.

hellohuanlin · 2023-09-29T16:31:15Z

packages/camera/camera_avfoundation/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.9.13+6
+
+* Fixes incorrect use of `NSError` that could cause crashes on launch.


Nit: ... cause crashes on launch on iOS 17

I unfortunately don't know if it's "on iOS 17" or "when compiled against the iOS 17 SDK" or some other slight variant of that, which is why I left it vague.

hellohuanlin · 2023-09-29T16:34:13Z

packages/camera/camera_avfoundation/example/ios/RunnerTests/CameraPropertiesTests.m

@@ -19,7 +19,7 @@ - (void)testFLTGetFLTFlashModeForString {
  XCTAssertEqual(FLTFlashModeAuto, FLTGetFLTFlashModeForString(@"auto"));
  XCTAssertEqual(FLTFlashModeAlways, FLTGetFLTFlashModeForString(@"always"));
  XCTAssertEqual(FLTFlashModeTorch, FLTGetFLTFlashModeForString(@"torch"));
-  XCTAssertThrows(FLTGetFLTFlashModeForString(@"unkwown"));
+  XCTAssertEqual(FLTFlashModeInvalid, FLTGetFLTFlashModeForString(@"unknown"));


Nit: should it be FLTFlashModeUnknown? in case we have new flash mode in the future.

If we have a new flash mode in the future then we need to update both the Dart and Obj-C code at the same time. This code is purely for manual platform channel conversions, which means the only way to get a value we don't recognize is if someone adds serialization for that mode on the Dart side but not the corresponding deserialization code on the native side, which would make no sense.

If we have flash modes that the Dart side of camera_avfoundation has been updated to send over the method channel but the Obj-C side doesn't know about, then someone has made a serious mistake in adding a new flash mode. We shouldn't explicitly allow for that case with an "unknown", implying that's an okay state to be in.

(This is why Pigeon will moot all of this; in Pigeon it's impossible for an enum value defined in the interface to exist on one side but not the other.)

…kages#5034)

flutter/packages@d0e9a0e...e2ac440 2023-10-01 engine-flutter-autoroll@skia.org Roll Flutter from d3df8f6 to d42313c (4 revisions) (flutter/packages#5044) 2023-10-01 maurits@vnbskm.nl [webview_flutter] Adds app facing implementation to override console log (flutter/packages#4705) 2023-09-30 engine-flutter-autoroll@skia.org Roll Flutter from 57b5c3c to d3df8f6 (24 revisions) (flutter/packages#5043) 2023-09-30 10687576+bparrishMines@users.noreply.github.com [webview_flutter] Add a method for getting the user agent (flutter/packages#4472) 2023-09-29 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android] Fix race condition in flaky test (flutter/packages#5037) 2023-09-29 stuartmorgan@google.com [ci] Wait for LUCI test checkin in `release` (flutter/packages#4911) 2023-09-29 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android][webview_flutter_wkwebview] Adds support for `getUserAgent` for `webview_flutter` platform implementations (flutter/packages#4927) 2023-09-29 stuartmorgan@google.com [ci] Disable maps tests in Android emulator (flutter/packages#5003) 2023-09-29 33461698+BradenBagby@users.noreply.github.com [camera] Dispose resources correctly on setDescription (flutter/packages#4003) 2023-09-29 maurits@vnbskm.nl [webview_flutter_android] Adds Android implementation to override console log (flutter/packages#4702) 2023-09-29 stuartmorgan@google.com [camera] Remove `@throw` from iOS implementation (flutter/packages#5034) 2023-09-29 engine-flutter-autoroll@skia.org Roll Flutter from ff4a0f6 to 57b5c3c (47 revisions) (flutter/packages#5036) 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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Using `@throw` in iOS code violates the style guide, so it shouldn't be done in the plugin as mechanism for communicating errors. More importantly, `NSError` is not intended to be used with `@throw`/`@catch`, and is causing issues when compiled with the iOS 17 SDK. This removes all use of `@throw`, and all `@catch (NSError* e)`, in favor of other methods of communicating errors. It explicitly does not try to fix all the other strange things about this code (having an `NSError` out-param in an init method, using Cocoa and NSURL error domains and codes for some reason), and instead preserves existing behavior as much as possible. In practice, none of these codepaths should ever actually happen (they indicate programming errors within the plugin, not unexpected runtime behavior), and all of this code will go away when converting to Pigeon anyway, so there's not much value in trying to unwind this structure further. Fixes flutter/flutter#135195

stuartmorgan requested a review from hellohuanlin as a code owner September 29, 2023 14:44

github-actions bot added p: camera platform-ios labels Sep 29, 2023

stuartmorgan commented Sep 29, 2023

View reviewed changes

stuartmorgan force-pushed the camera-ios-throw-catch-fix branch from 47bc91e to 741d98b Compare September 29, 2023 14:53

hellohuanlin approved these changes Sep 29, 2023

View reviewed changes

Add switch handling

c1bffdc

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 29, 2023

auto-submit bot merged commit 95b9959 into flutter:main Sep 29, 2023

engine-flutter-autoroll mentioned this pull request Oct 2, 2023

Roll Packages from d0e9a0e1b3ac to e2ac440a547a (12 revisions) flutter/flutter#135859

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 2, 2023

95b9959 [camera] Remove @throw from iOS implementation (flutter/pac…

3f0d7f1

…kages#5034)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[camera] Remove `@throw` from iOS implementation #5034

[camera] Remove `@throw` from iOS implementation #5034

stuartmorgan commented Sep 29, 2023

stuartmorgan Sep 29, 2023

stuartmorgan Sep 29, 2023

stuartmorgan Sep 29, 2023

hellohuanlin left a comment

hellohuanlin Sep 29, 2023

stuartmorgan Sep 29, 2023

hellohuanlin Sep 29, 2023

stuartmorgan Sep 29, 2023

stuartmorgan Sep 29, 2023

		@@ -1,3 +1,7 @@
		## 0.9.13+6

		* Fixes incorrect use of `NSError` that could cause crashes on launch.

[camera] Remove @throw from iOS implementation #5034

[camera] Remove @throw from iOS implementation #5034

Conversation

stuartmorgan commented Sep 29, 2023

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[camera] Remove `@throw` from iOS implementation #5034

[camera] Remove `@throw` from iOS implementation #5034