Fix crash when attaching to a device with multiple active flutter apps #175147
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request fixes a crash that occurs when discoveryStatus.stop() is called multiple times on a stream that emits multiple URIs. The fix introduces a flag to ensure it's only called once. My review suggests a more declarative approach using asBroadcastStream and take(1).listen which achieves the same result with cleaner code and might also improve robustness in related loop structures.
| var stopped = false; | ||
| vmServiceUri = vmServiceUri.map((Uri uri) { | ||
| discoveryStatus.stop(); | ||
| if (!stopped) { | ||
| discoveryStatus.stop(); | ||
| stopped = true; | ||
| } | ||
| return uri; | ||
| }); |
There was a problem hiding this comment.
While this approach correctly prevents multiple calls to discoveryStatus.stop(), it can be implemented more declaratively using asBroadcastStream and take(1).listen. This avoids the manual stopped flag and improves readability.1
Converting to a broadcast stream also makes the code more robust by preventing a potential StateError if the while (true) loop further down were to execute more than once, as it would attempt to listen to the same single-subscription stream multiple times.
vmServiceUri = vmServiceUri.asBroadcastStream();
vmServiceUri.take(1).listen((_) {
if (!discoveryStatus.done) {
discoveryStatus.stop();
}
});Style Guide References
Footnotes
-
The style guide encourages optimizing for readability. The suggested change is more declarative and easier to understand at a glance compared to maintaining a manual flag. ↩
5848d93 to
42bf375
Compare
42bf375 to
5902c7d
Compare
flutter/flutter@f331a55...29a238d 2025-09-15 engine-flutter-autoroll@skia.org Roll Skia from d84a369255c4 to f950263bb3d4 (1 revision) (flutter/flutter#175354) 2025-09-15 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 4ZIBcdI2x_y8trVYz... to wzk_HjPLGu-mlg5hC... (flutter/flutter#175349) 2025-09-15 engine-flutter-autoroll@skia.org Roll Dart SDK from 24179911b2fe to 50e61e5bff51 (2 revisions) (flutter/flutter#175346) 2025-09-15 engine-flutter-autoroll@skia.org Roll Skia from 785f8859c7b9 to d84a369255c4 (5 revisions) (flutter/flutter#175342) 2025-09-15 engine-flutter-autoroll@skia.org Roll Dart SDK from 628b3f869d9b to 24179911b2fe (1 revision) (flutter/flutter#175331) 2025-09-15 engine-flutter-autoroll@skia.org Roll Skia from 4fb7e988c981 to 785f8859c7b9 (1 revision) (flutter/flutter#175330) 2025-09-14 engine-flutter-autoroll@skia.org Roll Skia from 64c5ab69997f to 4fb7e988c981 (1 revision) (flutter/flutter#175322) 2025-09-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from TrB_3av7CK7a5Wb0h... to 4ZIBcdI2x_y8trVYz... (flutter/flutter#175319) 2025-09-14 engine-flutter-autoroll@skia.org Roll Skia from 7b489cee9eca to 64c5ab69997f (1 revision) (flutter/flutter#175316) 2025-09-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 5deba9e4e108 to 628b3f869d9b (1 revision) (flutter/flutter#175314) 2025-09-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from b1AYfAFOnvBMHSsYL... to TrB_3av7CK7a5Wb0h... (flutter/flutter#175306) 2025-09-13 engine-flutter-autoroll@skia.org Roll Dart SDK from e82f3fc8b2d5 to 5deba9e4e108 (1 revision) (flutter/flutter#175302) 2025-09-13 engine-flutter-autoroll@skia.org Roll Skia from 3321829b90dd to 7b489cee9eca (1 revision) (flutter/flutter#175298) 2025-09-13 41930132+hellohuanlin@users.noreply.github.com [ios]Do not re-adds delaying recognizer on iOS 26 (flutter/flutter#175097) 2025-09-13 engine-flutter-autoroll@skia.org Roll Skia from b2cdcf07b2b5 to 3321829b90dd (22 revisions) (flutter/flutter#175295) 2025-09-13 engine-flutter-autoroll@skia.org Roll Dart SDK from 11dedad2d062 to e82f3fc8b2d5 (3 revisions) (flutter/flutter#175294) 2025-09-12 rodrigogmdias@gmail.com Add semanticIndexOffset argument to SliverList.builder, SliverGrid.builder, and SliverFixedExtentList.builder (flutter/flutter#174856) 2025-09-12 chingjun@google.com Fix crash when attaching to a device with multiple active flutter apps (flutter/flutter#175147) 2025-09-12 56561849+Rushikeshbhavsar20@users.noreply.github.com Update transformHitTests documentation for clarity (flutter/flutter#174286) 2025-09-12 engine-flutter-autoroll@skia.org Roll Skia from ead9277819fc to b2cdcf07b2b5 (1 revision) (flutter/flutter#175226) 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 louisehsu@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://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
flutter#175147) `vmServiceUri` is a stream, and might emit multiple values. But `discoveryStatus.stop()` can only be called once.
flutter#175147) `vmServiceUri` is a stream, and might emit multiple values. But `discoveryStatus.stop()` can only be called once.
flutter#175147) `vmServiceUri` is a stream, and might emit multiple values. But `discoveryStatus.stop()` can only be called once.
vmServiceUriis a stream, and might emit multiple values. ButdiscoveryStatus.stop()can only be called once.