Fix flutter_tools crashing on invalid UTF-8 in log output (fixes #184646)#184685
Conversation
…ter#184646) The custom Utf8Decoder in flutter_tools was calling throwToolExit() when encountering invalid UTF-8 bytes (e.g., U+FFFD replacement character), which crashed the entire 'flutter run' process. Changed to print a warning instead of crashing, allowing the development workflow to continue uninterrupted. This is important because debugPrint() and other logging often contain invalid UTF-8 from external sources (APIs, Bluetooth, network data, etc.). Updated tests to verify the decoded string is returned instead of throwing ToolExit.
There was a problem hiding this comment.
Code Review
This pull request modifies the Utf8Decoder to issue a warning instead of throwing a ToolExit when encountering malformed UTF-8, while also removing the base/common.dart dependency. The test suite is expanded to cover additional decoding edge cases. Review feedback suggests that the validation check may introduce performance overhead and recommends using the logging infrastructure instead of print to prevent console flooding and maintain compatibility with machine-readable output.
justinmc
left a comment
There was a problem hiding this comment.
Are we sure that this shouldn't throw? Have we considered catching this error at a higher level instead?
Also, why does this only happen on web?
|
@justinmc Thanks for the review! Let me address your questions: 1. Are we sure this shouldn't throw? Have we considered catching this error at a higher level?We believe printing a warning instead of throwing is the right approach. Here's why: Why throwing is too aggressive:
Why catching at a higher level is more complex:
2. Why does this only happen on web?This doesn't happen on the web because web exclusively uses VM Service protocol to receive logs from the running app, whereas other platforms use different approaches: How App Output Works
How flutter_tools Receives the Output
Root Cause: Web is the only platform that exclusively relies on VM Service protocol to receive stdout/stderr from the running app, which goes through |
|
I ran the example code: import 'package:flutter/material.dart';
void main() {
runApp(const MyApp());
}
class MyApp extends StatelessWidget {
const MyApp({super.key});
@override
Widget build(BuildContext context) {
const String crashString = "�";
return MaterialApp(
title: 'App crasher',
home: Center(
child: TextButton(onPressed: () => debugPrint(crashString), child: const Text("Crash app")),
),
);
}
}And here's I captured behavior in different platforms: On Windows On Web |
|
I'm a bit hesitant to change this behavior across the tool since a crash is much more likely to prompt users to file issues than a warning. However, I do agree that I think a better approach would be to update |
|
@bkonyi I apologize for the late reply. Whatever, thanks for the thoughtful feedback! I understand your concern about losing crash detection for real errors. However, I'd like to clarify why I believe the current approach is appropriate: Why We Don't Need Two Rules Here1. The crash ONLY happens in app log processing
2. Invalid UTF-8 in app logs is expected
3. Real tool errors are caught elsewhere
Proposed AlternativeIf you're concerned about future use cases, we could:
What do you think? Does this address your concern? |
Not a problem! 😄 We actually have places in the tool where we're explicitly disabling this check for invalid characters (here's one example), so this shouldn't be too hard to do elsewhere without disabling this warning. While you're right that the tool failing to parse UTF-8 is a tooling error, we do use the same I think the main issue is that we're using the |
|
@bkonyi Thanks for the thoughtful suggestion! I'll |
|
@bkonyi, @justinmc , I conductuced a details investigation about this. Let me document what I've discovered: 2018-2026: The Same Bug Reported 13+ Times
Last merged PR on this issue was PR #26650 (2019) - The Incomplete Fix What jonahwilliams Did: He wrapped
The Problem - Incomplete Implementation: Looking at the code he added: class Utf8Decoder extends Converter<List<int>, String> {
const Utf8Decoder({this.reportErrors = true}); // Parameter exists
static const _systemDecoder = cnv.Utf8Decoder(allowMalformed: true); // System decoder is permissive
final bool reportErrors;
@override
String convert(List<int> input, [int start = 0, int? end]) {
final String result = _systemDecoder.convert(input, start, end);
// If reportErrors: true and result contains U+FFFD CRASH
if (reportErrors && result.contains('\u{FFFD}')) {
throwToolExit('Bad UTF-8 encoding...'); // THROWS!
}
return result;
}
}Then he created this global constant: const Encoding utf8 = Utf8Codec(); // Always reportErrors: true (STRICT)He had the infrastructure ( @bkonyi Recommended
I separated the two use cases: 1. Created two distinct constants: const Encoding utf8 = Utf8Codec(); // Strict (for tool data)
const Encoding utf8AllowMalformed = Utf8Codec(reportErrors: false); // Permissive (for app logs)2. Applied permissive decoding to app log streams:
3. Kept strict decoding where needed:
|
|
An existing Git SHA, To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with |
| 'https://github.com/flutter/flutter/issues/new/choose\n' | ||
| 'The source bytes were:\n$input\n\n', | ||
| // ignore: avoid_print | ||
| print( |
There was a problem hiding this comment.
We should revert this change and continue invoking throwToolExit for the reportErrors case.
There was a problem hiding this comment.
Oww! Exactly. I didnot notice. Right said. I'll do
|
Thanks @Istiak-Ahmed78! This looks much better! I think this will fix most of the common cases where we can encounter invalid UTF-8. |
|
@bkonyi , I reverted the change you suggested. Let me know if more changes needed |
|
Heads up @Istiak-Ahmed78 there are some test failures here. |
Aww. Okey. I'll check |
@justinmc , I addressed the |
|
@justinmc can I get you to reapprove this PR please? :) |
Summary
Changes the
Utf8Decoderinflutter_toolsto print a warning instead of callingthrowToolExit()when encountering invalid UTF-8 bytes (such as the Unicode replacement character U+FFFD). Previously, this would crash the entireflutter runprocess, disrupting the developer workflow.Context
When an app logs data containing invalid UTF-8 (e.g., from external APIs, Bluetooth devices, or network responses), the custom UTF-8 decoder in
flutter_toolswould detect the replacement character and terminate the tool. This was overly aggressive sincedebugPrint()and similar logging functions are expected to handle arbitrary data.Changes
packages/flutter_tools/lib/src/convert.dart: ReplacedthrowToolExit()with a warningprint()that reports the issue but allows the tool to continue running.packages/flutter_tools/test/general.shard/convert_test.dart: Updated existing tests and added new tests to verify:reportErrors: falsedecodes silentlyBefore
After
Pre-launch Checklist
Fixes #184646