Prevent crashes on range errors when selecting device #129290

eliasyishak · 2023-06-21T16:45:33Z

Prevent the cli from crashing when a user selects a number that is not valid for flutter run device selection

Fixes issue:

[flutter_tools] RangeError in TargetDeviceSelection.userSelectDevice #129191

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

eliasyishak · 2023-06-21T17:01:23Z

packages/flutter_tools/lib/src/runner/target_devices.dart

+
+    Device? chosenDevice;
+    while (chosenDevice == null) {
+      final String userInputString = await _readUserInput(devices.length);
+      if (userInputString.toLowerCase() == 'q') {
+        throwToolExit('');
+      }
+      final int deviceIndex = int.parse(userInputString) - 1;
+      if (deviceIndex > -1 && deviceIndex < devices.length) {
+        chosenDevice = devices[deviceIndex];
+      }
    }
-    return devices[int.parse(userInput) - 1];
+    return chosenDevice;


@vashworth hey I noticed that we have two different places we are implementing logic for selecting one of the available devices... this PR has fixes for both places but wondering if we should simplify this and have a function that is used by both TargetDevices and TargetDevicesWithExtendedWirelessDeviceDiscovery?

Seems like it was a deliberate decision to have these two classes separate though so just wanted to check in with you

Yeah, although they're similar, they're separate because TargetDevices knows in advance exactly what numbers are valid options, whereas TargetDevicesWithExtendedWirelessDeviceDiscovery does not since options get added asynchronously.

If you look into the TargetDevices._readUserInput function, it uses terminal.promptForCharInput, which handles the while loop and validates it's an acceptable input, so you shouldn't need to change this.

Since TargetDevicesWithExtendedWirelessDeviceDiscovery does not know in advance what options are valid, it can't use terminal.promptForCharInput, so it accepts any number as input and then validates it's a valid option.

Ah good call, I will revert the changes made to TargetDevices then and find a test that uses TargetDevicesWithExtendedWirelessDeviceDiscovery. I think the test I tried to piggy back on was using TargetDevices method for selecting a device

`globals.terminal.promptForCharInput` method already checks that the input is valid

eliasyishak · 2023-06-21T19:05:27Z

packages/flutter_tools/test/general.shard/runner/target_devices_test.dart

+  List<String> input = <String>[];

  @override
  Future<String> readUserInput() async {
-    return input;
+    // If only one value is provided for the input, continue
+    // to return that one input value without popping
+    //
+    // If more than one input values are provided, we are simulating
+    // the user selecting more than one option for a device, so we will pop
+    // them out from the front
+    if (input.length > 1) {
+      return input.removeAt(0);
+    }
+
+    return input[0];


hey @vashworth, i reworked this method so that we provide lists of inputs for selecting a device.

Thoughts? The while loop in the TargetDeviceSelection.userSelectDevice() method makes it a bit difficult to mock multiple inputs over a test

That makes sense to me!

vashworth · 2023-06-21T19:47:51Z

packages/flutter_tools/test/general.shard/runner/target_devices_test.dart

+            // Having the `0` first is an invalid choice for a device, the second
+            // item in the list, `1` is a valid option though which will return a
+            // valid device
+            targetDevices.deviceSelection.input = <String>['0', '1'];


Could you add another input here that's out of range on the top end too (so 2 in this case)?

Oh good catch!

vashworth

LGTM

…ter/flutter#129290)

flutter/flutter@c40baf4...042c036 2023-06-22 ahmedelsaayid@gmail.com Remove unnecessary variable `_hasPrimaryFocus` (flutter/flutter#129066) 2023-06-22 tessertaha@gmail.com Fix Material 3 Scrollable `TabBar` (flutter/flutter#125974) 2023-06-22 utisam@gmail.com Fix: Closing bottom sheet and removing FAB cause assertion failure (flutter/flutter#128566) 2023-06-22 tessertaha@gmail.com Add `InputDecorationTheme.merge` (flutter/flutter#129011) 2023-06-22 engine-flutter-autoroll@skia.org Roll Packages from 9af50d4 to 95bc1c6 (6 revisions) (flutter/flutter#129351) 2023-06-22 42216813+eliasyishak@users.noreply.github.com Prevent crashes on range errors when selecting device (flutter/flutter#129290) 2023-06-22 zanderso@users.noreply.github.com Revert "Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision)" (flutter/flutter#129353) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 703c9a14ac7f to 8cc6d6d5efdb (1 revision) (flutter/flutter#129339) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from d9530e2b87de to 703c9a14ac7f (1 revision) (flutter/flutter#129337) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from c6251a69a09a to d9530e2b87de (1 revision) (flutter/flutter#129334) 2023-06-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from 08aaa88bf67f to c6251a69a09a (10 revisions) (flutter/flutter#129331) 2023-06-22 jason-simmons@users.noreply.github.com Manual roll of packages to 9af50d4 (flutter/flutter#129328) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 090fae83548a to 08aaa88bf67f (3 revisions) (flutter/flutter#129306) 2023-06-21 yjbanov@google.com [framework,web] add FlutterTimeline and semantics benchmarks that use it (flutter/flutter#128366) 2023-06-21 fluttergithubbot@gmail.com Roll pub packages (flutter/flutter#128966) 2023-06-21 6655696+guidezpl@users.noreply.github.com Remove incorrect non-nullable assumption from `ShapeDecoration.lerp` (flutter/flutter#129298) 2023-06-21 jmccandless@google.com Gracefully handle negative position in getWordAtOffset (flutter/flutter#128464) 2023-06-21 christopherfujino@gmail.com [flutter_tools] add a gradle error handler for could not open cache directory (flutter/flutter#129222) 2023-06-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from f973fb4636d3 to 090fae83548a (5 revisions) (flutter/flutter#129293) 2023-06-21 rmolivares@renzo-olivares.dev Selection area right click behavior should match native (flutter/flutter#128224) 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 Please CC dit@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

…ter/flutter#129290)

Prevent user from typing 0 during device selection

b6b6094

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 21, 2023

Add test + correct selection logic for TargetDevices

472c8fe

eliasyishak commented Jun 21, 2023

View reviewed changes

eliasyishak added 4 commits June 21, 2023 13:47

Revert unnecessary logic for TargetDevices

5326296

`globals.terminal.promptForCharInput` method already checks that the input is valid

Removing accompanying test for TargetDevices

931565d

Clean up

530f8a9

Adding test to validate having invalid user inputs for device

dc1315a

eliasyishak commented Jun 21, 2023

View reviewed changes

eliasyishak marked this pull request as ready for review June 21, 2023 19:05

vashworth reviewed Jun 21, 2023

View reviewed changes

Add device id that exceeds top bounds

2091fcd

vashworth approved these changes Jun 21, 2023

View reviewed changes

eliasyishak added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 22, 2023

auto-submit bot merged commit 18b94b7 into flutter:master Jun 22, 2023
124 checks passed

eliasyishak mentioned this pull request Jun 22, 2023

[flutter_tools] RangeError in TargetDeviceSelection.userSelectDevice #129191

Closed

eliasyishak deleted the 129191-target-device-range-error branch June 22, 2023 16:18

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 2e05371c88da (17 revisions) flutter/packages#4276

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

9805d43

…ter/flutter#129290)

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 0da8012cc77d (18 revisions) flutter/packages#4277

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

9350eb8

…ter/flutter#129290)

engine-flutter-autoroll mentioned this pull request Jun 22, 2023

Roll Flutter from c40baf47c5c8 to 042c0366c9a9 (20 revisions) flutter/packages#4278

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 22, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

6a4070d

…ter/flutter#129290)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

6aa5d90

…ter/flutter#129290)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

4b65b55

…ter/flutter#129290)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

cbcaaba

…ter/flutter#129290)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

18b94b7f5 Prevent crashes on range errors when selecting device (flut…

372fa4b

…ter/flutter#129290)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent crashes on range errors when selecting device #129290

Prevent crashes on range errors when selecting device #129290

eliasyishak commented Jun 21, 2023

eliasyishak Jun 21, 2023

vashworth Jun 21, 2023

eliasyishak Jun 21, 2023

eliasyishak Jun 21, 2023

vashworth Jun 21, 2023

vashworth Jun 21, 2023

eliasyishak Jun 21, 2023

vashworth left a comment

Prevent crashes on range errors when selecting device #129290

Prevent crashes on range errors when selecting device #129290

Conversation

eliasyishak commented Jun 21, 2023

Pre-launch Checklist

eliasyishak Jun 21, 2023

Choose a reason for hiding this comment

vashworth Jun 21, 2023

Choose a reason for hiding this comment

eliasyishak Jun 21, 2023

Choose a reason for hiding this comment

eliasyishak Jun 21, 2023

Choose a reason for hiding this comment

vashworth Jun 21, 2023

Choose a reason for hiding this comment

vashworth Jun 21, 2023

Choose a reason for hiding this comment

eliasyishak Jun 21, 2023

Choose a reason for hiding this comment

vashworth left a comment

Choose a reason for hiding this comment