Revert "[path_provider_android] Changes internal implementation to use JNI"

[Renzo-Olivares]

Reverts #9770

Reason: Fix flutter/flutter tree

Tracking issue: flutter/flutter#184689

Crash below:

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8685239933882993057/+/u/run_test.dart_for_tool_tests_commands_shard_and_subshard_1_1/stdout

'package:flutter_tools/src/platform_plugins.dart': Failed assertion: line 444 pos 15: 'pluginClass != null || dartPluginClass != null || defaultPackage != null': is not true.

[gemini-code-assist]

Code Review

This pull request reverts the path_provider_android implementation from JNI/FFI to a Pigeon-based architecture, including a version downgrade to 2.2.23 and the removal of JNI dependencies. Feedback highlights a non-existent Android Gradle Plugin version and multiple potential NullPointerException vulnerabilities in the PathProviderPlugin Java code, particularly concerning Context usage and directory access. A suggestion to clear the context reference on engine detachment was also provided.

[gemini-code-assist]

The Android Gradle Plugin version 8.13.1 appears to be a typo, as this version does not exist in the Google Maven repository. It is likely intended to be 8.1.3, 8.3.1, or another valid version compatible with the project's Gradle configuration.

[gemini-code-assist]

The context field should be initialized before calling PathProviderApi.setUp. If a message is received from the Flutter side immediately after the API is set up but before the context is assigned, it could lead to a NullPointerException in the handler methods.

  private void setUp(BinaryMessenger messenger, Context context) {
    this.context = context;
    try {
      PathProviderApi.setUp(messenger, this);
    } catch (Exception ex) {
      Log.e(TAG, "Received exception while setting up PathProviderPlugin", ex);
    }
  }

[gemini-code-assist]

It is good practice to null out the context reference when the plugin is detached from the engine to avoid holding onto the application context longer than necessary, even though getApplicationContext() is generally safe.

Suggested change

	public void onDetachedFromEngine(@NonNull FlutterPluginBinding binding) {
	PathProviderApi.setUp(binding.getBinaryMessenger(), null);
	}
	@Override
	public void onDetachedFromEngine(@NonNull FlutterPluginBinding binding) {
	PathProviderApi.setUp(binding.getBinaryMessenger(), null);
	context = null;
	}

[gemini-code-assist]

context.getCacheDir() can return null if the cache directory does not exist and cannot be created (e.g., due to storage issues). A null check should be added to prevent a NullPointerException when calling .getPath().

    File cacheDir = context.getCacheDir();
    return cacheDir == null ? null : cacheDir.getPath();

[gemini-code-assist]

Similar to getTemporaryPath, context.getCacheDir() can return null. Adding a null check here ensures robustness.

    File cacheDir = context.getCacheDir();
    return cacheDir == null ? null : cacheDir.getPath();

[gemini-code-assist]

context.getExternalCacheDirs() can return null if the shared/external storage is not currently available. Iterating over a null array will result in a crash.

    File[] dirs = context.getExternalCacheDirs();
    if (dirs != null) {
      for (File dir : dirs) {
        if (dir != null) {
          paths.add(dir.getAbsolutePath());
        }
      }
    }

[gemini-code-assist]

context.getExternalFilesDirs() can return null if the storage is unavailable. A null check should be performed before iterating over the results.

    File[] dirs = context.getExternalFilesDirs(getStorageDirectoryString(directory));
    if (dirs != null) {
      for (File dir : dirs) {
        if (dir != null) {
          paths.add(dir.getAbsolutePath());
        }
      }
    }

[hellohuanlin]

I wonder how the versioning works? Was the previous PR published already (it has a version bump).

I'm also curious if the original PR will cause the same crash for plugin users.

[jmagman]

I wonder how the versioning works? Was the previous PR published already (it has a version bump).

This won't unpublish the package. You would need to bump the version. Technically you can unpublish a pub package but I'm not sure the process for that. @stuartmorgan-g would know. Generally we don't do it.

[bparrishMines]

cc @liamappelbe

[bparrishMines]

If possible the flutter/flutter repo may need to set path_provider_android: 2.2.23 temporarily. I'm assuming Flutter is using the latest version from pub.dev.

[jmagman]

If possible the flutter/flutter repo may need to set path_provider_android: 2.2.23 temporarily. I'm assuming Flutter is using the latest version from pub.dev.

I was trying to do that here, but I'm still hitting test failures. I wasn't able to reproduce them locally. flutter/flutter#184688

[Renzo-Olivares]

I'm also trying updating the assert we are hitting, it looks like it might just be an oversight that we are hitting this crash but I don't have enough context flutter/flutter#184695.

the ffiPlugin flag was added in flutter/flutter#96225 , but it was not added to assert of WindowsPlugin for some reason (it was added to the assert of LinuxPlugin).

[stuartmorgan-g]

Since so far the issues are specific to our CI I would rather we avoid a full revert cycle if possible. I have unpublished 2.3.0 from pub.dev temporarily while we work on fixes to our systems; it looks like flutter/flutter already had a real fix landed, and I should be able to fix flutter/packages today, and then undo the unpublshing to check that everything is good without doing full revert/reland cycles.

(If we do revert, we should not revert the changes outside of the package itself, as it causes a ton more CI runs and potential flake)