Skip to content

[beta] Show error on macOS if missing Local Network permissions #162119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 3 commits into from
Feb 7, 2025
Merged

[beta] Show error on macOS if missing Local Network permissions #162119

auto-submit merged 3 commits into from
Feb 7, 2025

Conversation

@loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Jan 23, 2025 •
edited
Loading

⚠️ This targets branch flutter-3.29-candidate.0 for the Flutter beta.

Cherrypicks #161846 to flutter-3.29-candidate.0.

Impacted Users: Users that use macOS Sequoia and haven't turned on Local Network permissions for their IDE or terminal app. See: #150131
Impact Description: Without this improved error message, flutter run outputs this error: SocketException: Send failed, OS Error: No route to host, errno = 65.
Workaround: Turn on Local Network permissions for your IDE or terminal app. However, it's difficult for customers to discover this from the error message.
Risk: This change could regress the tool's Dart VM discovery.
Test Coverage: Yes added tests
Validation Steps: In System Settings > Privacy & Security > Local Network, toggle off Visual Studio Code. In VS Code, run a Flutter app on a physical device.

@loic-sharma
Show error on macOS if missing Local Network permissions (flutter#161846
385397b 
)

### Background

macOS Sequoia requires the user's permission to do multicast operations,
which the Flutter tool does to connect to the Dart VM. If the app does
not have permission to communicate with devices on the local network,
the following happens:

1. Flutter tool starts a [multicast
lookup](https://github.com/flutter/flutter/blob/bb2d34126cc8161dbe4a1bf23c925e48b732f670/packages/flutter_tools/lib/src/mdns_discovery.dart#L238-L241)
2. The mDNS client [sends data on the
socket](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L219)
4. macOS blocks the operation. Dart's native socket implementation
throws an `OSError`
5. Dart's `Socket.send` [catches the
`OSError`](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1511-L1515),
wraps it in a `SocketException`, and [schedules a
microtask](https://github.com/dart-lang/sdk/blob/da6dc03a15822d83d9180bd766c02d11aacdc06b/sdk/lib/_internal/vm/bin/socket_patch.dart#L1513)
that [reports the exception through the socket's
stream](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/_internal/vm/bin/socket_patch.dart#L3011)
([`Socket` is a
`Stream`](https://api.dart.dev/dart-io/Socket-class.html))
6. The mDNS client [does not listen to the socket stream's
errors](https://github.com/flutter/packages/blob/973e8b59e24ba80d3c36a2bcfa914fcfd5e19943/packages/multicast_dns/lib/multicast_dns.dart#L155),
so [the error is sent to the current `Zone`'s uncaught error
handler](https://github.com/dart-lang/sdk/blob/95f00522676dff03f64fc715cb1835ad451faa4c/sdk/lib/async/stream_impl.dart#L553).

### Reproduction

To reproduce this error on macOS...

1. Open System Settings > Privacy & Security > Local Network and toggle
off Visual Studio Code
2. Run a Flutter app using a physical device

### Fix

Ideally, we'd make `MDnsClient.lookup` throw an exception for this
scenario. Unfortunately, the `MDnsClient` can have multiple lookup
operations in parallel, and the `SocketException` doesn't give us enough
information to match it back to a pending `MDnsClient` request. See
flutter/packages#8450 as an attempt to solve
this in the `MDnsClient` layer.

Instead, this fix introduces a `Zone` in the tool to catch the socket's
uncaught exception.

Follow-up to flutter#157638

See: flutter#150131

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [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.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[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
@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 23, 2025
@loic-sharma loic-sharma changed the title [stable] Show error on macOS if missing Local Network permissions (#161846) [stable] Show error on macOS if missing Local Network permissions Jan 23, 2025
@itsjustkevin itsjustkevin changed the title [stable] Show error on macOS if missing Local Network permissions [beta] Show error on macOS if missing Local Network permissions Jan 23, 2025
@itsjustkevin
Copy link
Contributor

itsjustkevin commented Jan 23, 2025

@loic-sharma this is for the beta branch with intention to land in the next stable build released. I have updated the title to keep the release engineer aware.

As this is a change to the beta branch, it does not require a changelog update.

@loic-sharma
Copy link
Member Author

loic-sharma commented Jan 23, 2025

Ah thanks for the correction!

@itsjustkevin
Copy link
Contributor

itsjustkevin commented Jan 23, 2025

Based on the risk, I would like @bkonyi to take a look as the reviewer. Let's hold off on merging until we are updated on the status of the flutter-3.29-candidate.0 beta.

CC @justinmc

@itsjustkevin itsjustkevin requested a review from bkonyi January 24, 2025 14:36
bkonyi
bkonyi approved these changes Jan 24, 2025
View reviewed changes
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

LGTM. This is the top crasher for the Flutter tool and this fix would be great to get cherry picked.

@justinmc
Copy link
Contributor

justinmc commented Jan 24, 2025 •
edited
Loading

The beta release is currently building, so this will have to go into the next one.

@loic-sharma
Copy link
Member Author

loic-sharma commented Jan 25, 2025

@justinmc Do I need to change the target branch to get in the next beta?

@justinmc
Copy link
Contributor

justinmc commented Jan 27, 2025

I think no, it will be the same branch flutter-3.29-candidate.0.

@bkonyi bkonyi added the cp: beta cherry pick this pull request to beta release candidate branch label Jan 30, 2025
@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2025
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2025
@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 6, 2025

autosubmit label was removed for flutter/flutter/162119, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@christopherfujino
Copy link
Contributor

christopherfujino commented Feb 6, 2025

There are test failures here:

 06:10 +474 ~2: test/commands.shard/hermetic/doctor_test.dart: doctor with grouped validators validate diagnose combines validator output
 Expected: '[✓] Category 1 [0ms]\n'
             '    • A helpful message\n'
             '    • A helpful message\n'
             '\n'
             '[!] Category 2 [0ms]\n'
             '    • A helpful message\n'
             '    ✗ A useful error message\n'
             '\n'
             '! Doctor found issues in 1 category.\n'
             ''
   Actual: '[✓] Category 1 [3ms]\n'
             '    • A helpful message\n'
             '    • A helpful message\n'
             '\n'
             '[!] Category 2 [3ms]\n'
             '    • A helpful message\n'
             '    ✗ A useful error message\n'
             '\n'
             '! Doctor found issues in 1 category.\n'
             ''
    Which: is different.
           Expected: ... tegory 1 [0ms]\n     ...
             Actual: ... tegory 1 [3ms]\n     ...
                                   ^
            Differ at offset 16

Although the failure makes me suspicious that these are not hermetic tests. cc @bkonyi

@bkonyi
Copy link
Contributor

bkonyi commented Feb 6, 2025

There are test failures here:

 06:10 +474 ~2: test/commands.shard/hermetic/doctor_test.dart: doctor with grouped validators validate diagnose combines validator output
 Expected: '[✓] Category 1 [0ms]\n'
             '    • A helpful message\n'
             '    • A helpful message\n'
             '\n'
             '[!] Category 2 [0ms]\n'
             '    • A helpful message\n'
             '    ✗ A useful error message\n'
             '\n'
             '! Doctor found issues in 1 category.\n'
             ''
   Actual: '[✓] Category 1 [3ms]\n'
             '    • A helpful message\n'
             '    • A helpful message\n'
             '\n'
             '[!] Category 2 [3ms]\n'
             '    • A helpful message\n'
             '    ✗ A useful error message\n'
             '\n'
             '! Doctor found issues in 1 category.\n'
             ''
    Which: is different.
           Expected: ... tegory 1 [0ms]\n     ...
             Actual: ... tegory 1 [3ms]\n     ...
                                   ^
            Differ at offset 16

Although the failure makes me suspicious that these are not hermetic tests. cc @bkonyi

This was resolved in #161917

@christopherfujino
Copy link
Contributor

christopherfujino commented Feb 6, 2025

There are test failures here:

 06:10 +474 ~2: test/commands.shard/hermetic/doctor_test.dart: doctor with grouped validators validate diagnose combines validator output
 Expected: '[✓] Category 1 [0ms]\n'
             '    • A helpful message\n'
             '    • A helpful message\n'
             '\n'
             '[!] Category 2 [0ms]\n'
             '    • A helpful message\n'
             '    ✗ A useful error message\n'
             '\n'
             '! Doctor found issues in 1 category.\n'
             ''
   Actual: '[✓] Category 1 [3ms]\n'
             '    • A helpful message\n'
             '    • A helpful message\n'
             '\n'
             '[!] Category 2 [3ms]\n'
             '    • A helpful message\n'
             '    ✗ A useful error message\n'
             '\n'
             '! Doctor found issues in 1 category.\n'
             ''
    Which: is different.
           Expected: ... tegory 1 [0ms]\n     ...
             Actual: ... tegory 1 [3ms]\n     ...
                                   ^
            Differ at offset 16

Although the failure makes me suspicious that these are not hermetic tests. cc @bkonyi

This was resolved in #161917

Thanks, I'll patch this in to the branch

@bkonyi @christopherfujino
[ flutter_tool ] Fix flakiness in doctor_test.dart (flutter#161917)
a1ea6cb 
This test was merging two ZeroExecutionTimeValidationResults into a new
ValidationResult with an actual execution duration set rather than
Duration.zero.

Fixes flutter#161918
@christopherfujino
Copy link
Contributor

christopherfujino commented Feb 6, 2025

I just pushed a CP of: 3bfc25f

@github-actions github-actions bot added the a: desktop Running on desktop label Feb 6, 2025
@christopherfujino christopherfujino added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 6, 2025
@auto-submit auto-submit bot merged commit bd28ab6 into flutter:flutter-3.29-candidate.0 Feb 7, 2025
159 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
@engine-flutter-autoroll
bd28ab6d9 [beta] Show error on macOS if missing Local Network permiss…
c8ce4c0 
…ions (flutter/flutter#162119)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
@engine-flutter-autoroll
bd28ab6d9 [beta] Show error on macOS if missing Local Network permiss…
3213131 
…ions (flutter/flutter#162119)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
@engine-flutter-autoroll
bd28ab6d9 [beta] Show error on macOS if missing Local Network permiss…
e42fd83 
…ions (flutter/flutter#162119)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
@engine-flutter-autoroll
bd28ab6d9 [beta] Show error on macOS if missing Local Network permiss…
7a9db76 
…ions (flutter/flutter#162119)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
@engine-flutter-autoroll
bd28ab6d9 [beta] Show error on macOS if missing Local Network permiss…
cd7dd89 
…ions (flutter/flutter#162119)
@reidbaker reidbaker mentioned this pull request Apr 4, 2025
Merged
9 tasks
Fintasys pushed a commit to Fintasys/flutter that referenced this pull request May 14, 2025
@loic-sharma @Fintasys
[beta] Show error on macOS if missing Local Network permissions (flut…
1eb714c 
…ter#162119)

⚠️ This targets branch `flutter-3.29-candidate.0` for the Flutter beta.

Cherrypicks flutter#161846 to `flutter-3.29-candidate.0`.

**Impacted Users**: Users that use macOS Sequoia and haven't turned on Local Network permissions for their IDE or terminal app. See: flutter#150131
**Impact Description**: Without this improved error message, `flutter run` outputs this error: `SocketException: Send failed, OS Error: No route to host, errno = 65`.
**Workaround**: Turn on Local Network permissions for your IDE or terminal app. However, it's difficult for customers to discover this from the error message.
**Risk**: This change could regress the tool's Dart VM discovery.
**Test Coverage**: Yes added tests
**Validation Steps**: In System Settings > Privacy & Security > Local Network, toggle off Visual Studio Code. In VS Code, run a Flutter app on a physical device.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bkonyi bkonyi bkonyi approved these changes

@itsjustkevin itsjustkevin Awaiting requested review from itsjustkevin

Assignees

No one assigned

Labels

a: desktop Running on desktop autosubmit Merge PR when tree becomes green via auto submit App cp: beta cherry pick this pull request to beta release candidate branch tool Affects the "flutter" command-line tool. See also t: labels.

Projects

Google Testing Queue
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@loic-sharma @itsjustkevin @justinmc @christopherfujino @bkonyi @camsim99