The half-fixed bug: two upstream crashes in dio and flutter/packages

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.

Project Overview

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.

Bug Fix or Performance Improvement

1. dio: 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)

Enter fullscreen mode Exit fullscreen mode

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.

2. flutter/packages: a crash when a picked file can’t be copied

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);

Enter fullscreen mode Exit fullscreen mode

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 flutter/flutter#100025, an earlier report of the same class of failure.

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));

Enter fullscreen mode Exit fullscreen mode

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.

Code

• dio: cfug/dio#2550, merged
• flutter/packages: flutter/packages#12107, merged and shipped in file_selector_android 0.5.2+9

My Improvements

dio: widen the guard instead of adding a branch

The 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;
}

Enter fullscreen mode Exit fullscreen mode

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, '');

Enter fullscreen mode Exit fullscreen mode

flutter/packages: return null instead of an invalid object

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;
}

Enter fullscreen mode Exit fullscreen mode

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.

What I took from both

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.