Filter out .appex extension processes from LLDB attachment on iOS 17+#183724
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where flutter run could attach the debugger to an app extension instead of the main application on iOS 17+ devices. The fix involves filtering out processes with .appex in their executable path. The change is logical and includes a new test case. My feedback includes a suggestion to improve the new test to make it more robust by verifying that the correct process ID is being attached to.
hellohuanlin
left a comment
There was a problem hiding this comment.
LGTM % some comments
| 'info': <String, Object?>{'outcome': 'success'}, | ||
| 'result': <String, Object?>{ | ||
| 'installedApplications': [ | ||
| <String, Object?>{'installationURL': '/asdf'}, |
There was a problem hiding this comment.
can you provide a more realistic installationURL (e.g. /path/to/MyApp.app)
There was a problem hiding this comment.
changed as well to use /path/to/MyApp.app
| }, | ||
| }), | ||
| runningProcesses: [ | ||
| // Extension process — lower PID, would be picked first without the fix |
There was a problem hiding this comment.
It will be unclear to readers what "the fix" means after this PR is landed. I'd explicitly say that this is to ensure app extension is not picked even with lower PID, and link to the github issue here.
There was a problem hiding this comment.
changed it, could you have a look, pls?
|
Thanks for taking the time to review it hellohuanlin and for the comments, I'll make the necessary changes next week |
- Use a realistic installationURL (/path/to/MyApp.app) and update
executable paths accordingly
- Clarify comment to explicitly state the test ensures LLDB attaches
to the main app rather than one of its embedded extensions, and
link to the original issue (flutter#183263)
| // Find the process that was launched using the installationURL. | ||
| // Filter out app extension processes (.appex) to avoid attaching the | ||
| // debugger to a widget extension or other extension instead of the | ||
| // main app process. |
There was a problem hiding this comment.
Nit: can you link to the issue here? (I saw you linked in unit test, but non-test code is read much more often, so I'd recommend linking here)
There was a problem hiding this comment.
done, and sorry for the dismiss, wasn't on purpose
@Silfalion |
7b88e14
|
done okorohelijah |
…flutter#183724) Fixes flutter#183263 On iOS 17+, `flutter run` uses `devicectl` for process discovery. When an app has an embedded extension (e.g. WidgetKit), both the main app and extension processes match the `installationURL` because the extension lives inside the `.app` bundle at `PlugIns/*.appex/`. `.firstOrNull` picks whichever appears first in the process list — if the extension has a lower PID, LLDB attaches to the wrong process, causing `flutter run` to hang or fail. This adds a filter to exclude `.appex` processes from the matching, ensuring LLDB always attaches to the main app. ### Changes - `core_devices.dart`: Added `!process.executable!.contains('.appex')` filter to process matching - `core_devices_test.dart`: Added test verifying extension processes are skipped when both main app and extension are running ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Elijah Okoroh <okorohelijah@google.com>
Fixes #183263
On iOS 17+,
flutter runusesdevicectlfor process discovery. When an app has an embedded extension (e.g. WidgetKit), both the main app and extension processes match theinstallationURLbecause the extension lives inside the.appbundle atPlugIns/*.appex/..firstOrNullpicks whichever appears first in the process list — if the extension has a lower PID, LLDB attaches to the wrong process, causingflutter runto hang or fail.This adds a filter to exclude
.appexprocesses from the matching, ensuring LLDB always attaches to the main app.Changes
core_devices.dart: Added!process.executable!.contains('.appex')filter to process matchingcore_devices_test.dart: Added test verifying extension processes are skipped when both main app and extension are runningPre-launch Checklist
///).