This is a submission for DEV's Summer Bug Smash: Clear the Lineup powered by Sentry.
Both of these bugs were already known before I touched them. One had a linked issue and a fix that covered half the cases. The other had a unit test that asserted the crash, with a comment above it saying // Remove when fixed!
.
They also had the same shape. In both projects the correct behavior already existed elsewhere in the same file: a sibling transformer that handled the input, a sibling branch that reported the failure instead of dying. One path had been left out of it.
So neither fix invents behavior. Each one extends a decision the project had already made to the branch that missed it.
dio (12.8k stars) is the HTTP client most Dart and Flutter apps are built on: interceptors, form data, request cancellation, timeouts, and a pluggable Transformer
layer that turns raw response bytes into the typed object your code receives.
flutter/packages (5.3k stars) is the Flutter team's own monorepo of first-party plugins. The one here is file_selector_android
, the Android implementation behind file_selector
, the plugin you call when the user needs to pick a file from their device.
FormatException
on an empty response body
FusedTransformer
is dio's default transformer. When a request sets a custom responseDecoder
and the server returns an empty body with a JSON content type (a normal 200
, 204
, or 304
), the transformer's slow path fed the empty string straight into jsonDecode
:
FormatException: Unexpected end of input (at character 1)
SyncTransformer
and BackgroundTransformer
are handed the identical input and both guard the empty string and return it, which dio then normalizes to null
for typed JSON responses. Only the default transformer crashes.
This was the residual half of #2279. PR #2285 had already fixed the empty-body behavior for the fast path, the one with no custom decoder, but the slow path was never given the same guard. The issue looked closed and the fix looked shipped, and anyone who happened to set a responseDecoder
still got an exception.
file_selector_android
crashed the app when a selected file couldn't be copied to a readable location: a SecurityException
from a content provider that has withdrawn permission, or an IOException
on a full disk.
FileSelectorApiImpl.toFileResponse
looked like it handled this. Abbreviated:
String uriPath;
FileSelectorNativeException nativeError = null;
try {
uriPath = FileUtils.getPathFromCopyOfFileFromUri(
activityPluginBinding.getActivity(), uri);
} catch (IOException e) {
// If closing the output stream fails, we cannot be sure that the
// target file was written in full. […]
uriPath = null;
} catch (SecurityException e) {
// Calling `ContentResolver#openInputStream()` has been reported to throw a
// `SecurityException` on some devices in certain circumstances. Instead of crashing, we
// return `null`.
//
// See https://github.com/flutter/flutter/issues/100025 for more details.
uriPath = null;
} catch (IllegalArgumentException e) {
uriPath = FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH;
nativeError = new FileSelectorNativeException(/* … */);
}
return new FileResponse(uriPath, contentResolver.getType(uri), name, size, bytes, nativeError);
In the two branches that crash, uriPath = null
is not an oversight. It is deliberate, and commented as such: "Instead of crashing, we return null." The comment cites
The method then builds a FileResponse
on that null anyway. path
is non-null in the Pigeon-generated FileResponse
, so the constructor rejects it at runtime, and the branch written to avoid a crash produces one.
The third branch survives because IllegalArgumentException
assigns a placeholder path instead of null
, so it reaches Dart with a populated nativeError
. The file already had a way to report an unreadable file without dying. The two branches that chose null
just didn't use it.
The crash had also shifted shape over time, which is part of why it survived. flutter/flutter#159568 reported it as an IllegalStateException
, thrown by the older Java Pigeon generator; since the move to the Kotlin generator, the same failure surfaces as an NPE.
The test suite knew about it too. There was a passing test named openFileThrowsNullPointerException_whenSecurityExceptionInGetPathFromCopyOfFileFromUri
, which asserted the crash:
assertThrows(
NullPointerException.class,
() -> listenerArgumentCaptor.getValue()
.onActivityResult(222, Activity.RESULT_OK, resultMockIntent));
Above it, a comment: "The behavior is actually an error case and should be fixed … Remove when fixed!" So the crash was documented, asserted, and passing CI.
file_selector_android
0.5.2+9The whole fix is one extra condition:
if (isJsonContent &&
decodedResponse != null &&
decodedResponse.isNotEmpty) {
// slow path decoder, since there was a custom decoder specified
return jsonDecode(decodedResponse);
} else if (customResponseDecoder != null) {
return decodedResponse;
}
The obvious patch is a new early return for the empty case. I didn't write one, because the branch that already handles it correctly sits directly underneath: else if (customResponseDecoder != null) { return decodedResponse; }
. Adding isNotEmpty
to the guard lets an empty body fall through into it, and dio's existing normalization turns that into null
for a typed JSON response.
So the fix adds no new code path. It narrows a claim that was too broad: "this string is JSON" becomes "this non-empty string is JSON."
A non-empty malformed body still throws, as it did before and as SyncTransformer
and BackgroundTransformer
do. I wanted the three transformers to agree on this input. Making the default one more forgiving than the other two would have been a different change.
The regression test pins the behavior to the specific combination that was broken (empty body, JSON content type, custom decoder) rather than to the fix:
String decoder(List<int> bytes, RequestOptions o, ResponseBody rb) =>
utf8.decode(bytes, allowMalformed: true);
final response = await FusedTransformer().transformResponse(
RequestOptions(
responseType: ResponseType.json,
responseDecoder: decoder,
),
ResponseBody.fromBytes([], 200, headers: {
Headers.contentTypeHeader: [Headers.jsonContentType],
}),
);
expect(response, '');
Again the correct behavior already existed one level up. The callers of toFileResponse
handle a null return: they complete the Dart result with completeWithError("Failed to read file: …")
, which is what they do everywhere else a file can't be read. toFileResponse
was the only place that pushed on regardless.
if (uriPath == null) {
// `getPathFromCopyOfFileFromUri` can fail to produce a path: either by
// throwing (handled above by returning a null `uriPath`) or by returning
// null directly. Return null so the caller surfaces the failure to Dart,
// instead of building a `FileResponse` with a null `path`, which the
// non-null field rejects at runtime.
// See https://github.com/flutter/flutter/issues/159568.
return null;
}
Checking uriPath == null
rather than catching the exception at the call site was deliberate. getPathFromCopyOfFileFromUri
can also return null without throwing, and that path led to the same crash, so one check covers both.
The failure now reaches Dart as a PlatformException
you can catch, which is what the uriPath = null
branches were aiming for.
I inverted the test that asserted the crash rather than deleting it. It now drives onActivityResult
for real and asserts that the callback completes with a failure whose message contains "Failed to read file"
. I added a second one for the single-file openFile
path, which had the same defect and no coverage at all.
I also kept the PR scoped to the crash. #159568 suggests routing these branches through the typed FileSelectorNativeException
mechanism, the way #8184 did elsewhere. That is the better long-term design, but it needs a Pigeon regeneration and a Dart-facing API change, which would put a breaking change in the same PR as a crash fix. The API cleanup is worth doing on its own.
The reflex when you find a crash is to add a check. In both of these the check already existed, in the transformer next door or in the caller one frame up, so most of the work was reading enough of the surrounding code to find it. Once you have, the patch is small enough to review in one sitting.
Both bugs had also been looked at before. dio's had an issue and a fix that covered the fast path. The Android one had a test asserting the crash and a comment saying to remove it when fixed. Writing a bug down is not the same as fixing it, and it makes the bug much harder to see the second time around.