apache / cordova-plugin-file

Apache Cordova File Plugin
https://cordova.apache.org/
Apache License 2.0
741 stars 756 forks source link

Backward compatibility vs SDK Levels #564

Closed globules-io closed 1 year ago

globules-io commented 1 year ago

Is this plugin supposed to be backward compatible with all Android versions or at least down to 6.0.1 ? Right now on a brand new project, when targeting SDK Level 23, device ready is never fired, even on an empty project with only this plugin installed. What's the official SDK Level support nowadays ?

Thanks

Reproduced with cordova-android@11.0.0 cordova@11.1.0 cordova-plugin-file@7.0.0 Android@6.0.1

breautek commented 1 year ago

cordova-plugin-file@7 supports cordova-android@11, so by extension it supports API 22 - 32.

There are caveats however on API 29 with external storage (Because API 29 doesn't support File operations for external storage on API 29).

when targeting SDK Level 23,

By targeting SDK, if you mean you're setting the android Target SDK version to 23, then this isn't really a supported configuration. Cordova-android@11 expects the target SDK to be 31 or 32, as required by current Google Play deployment rules. Changing the Target SDK may enable or disable features, which may cause things not to work as expected.

Lastly testing on both an API 33 emulator & API 23 emulator, with both the default target SDK as well as setting target SDK 23 (also requires setting compileSdkVersion back to 33 since it will default to the same as targetSdkVersion) passes the "Hello World" test (the cordova app template fires device ready) for me.

If it isn't working for you, we may need more information about your environment.

globules-io commented 1 year ago

Well, I tried with

    <preference name="android-minSdkVersion" value="21" />
    <preference name="android-targetSdkVersion" value="31" />
    <preference name="android-maxSdkVersion" value="31" />

then tested on real Android 6.0.1 and it never fires ready. So from my point of view, it's not even compatible

Also tried

    <preference name="android-minSdkVersion" value="22" />

And

    <preference name="android-minSdkVersion" value="23" />

Same result

deviceready has not fired after 5 seconds.
cordova.js:1230 Channel not fired: onFileSystemPathsReady
breautek commented 1 year ago

I'm still unable to reproduce it with those configurations via Android Emulator Unfortunately, I don't have a physical device with Android 6 either that I can use to test with.

Do you see a difference in behaviour when using an Android Emulator vs the physical device?

Here's a few things I'd try so we can isolate the issue... If the device has an attached SD card, try removing it.

cordova.js:1230 Channel not fired: onFileSystemPathsReady

Seems like the routine for obtaining the file paths is not returning, or is slow at returning, which is probably the cause of the deviceready hang. This is done here: https://github.com/apache/cordova-plugin-file/blob/master/www/fileSystemPaths.js#L61

For some reason, we don't attach an error handler that might actually show some useful information.

If you could (for the sake of troubleshooting), modifying:

exec(after, null, 'File', 'requestAllPaths', []);

to

exec(after, function(error) {
    console.error(error);
}, 'File', 'requestAllPaths', []);

You'll want to clone/fork the repo to make this change, then to install it:

cordova plugin remove cordova-plugin-file
cordova plugin add ../path/to/repo/cordova-plugin-file

If the error handler doesn't present us with any useful information, then we will have to start debugging on the native side, to see what https://github.com/apache/cordova-plugin-file/blob/b32be30e4bd012f9d9568a9cc11926b2ad6c14f7/src/android/FileUtils.java#L1016 is actually doing.

globules-io commented 1 year ago

OK I will try to take some time to debug this...

breautek commented 1 year ago

Thanks, and let me know and I can provide assistance where I can.

Also... FWIW, the next cordova major release is planned to drop support for Android 6. In cordova-android@12 the new min SDK will be 24 by default, which is Android 7 if I recall correctly.

So if the issue is exclusively occurring on Android 6, then it may not be worth investigating (unless if you truly require android 6 support of course...)

globules-io commented 1 year ago
exec(after, function(error) {
    console.error(error);
}, 'File', 'requestAllPaths', []);

There is no error thrown. Nothing at all ....

breautek commented 1 year ago
exec(after, function(error) {
    console.error(error);
}, 'File', 'requestAllPaths', []);

There is no error thrown. Nothing at all ....

Ok.

Then something in native is hanging or going to a path that doesn't lead to calling the callback context on the native side.

The native project is found at <cordova-project>/platforms/android, which can be opened up in Android Studio. When you open the project up in Android Studio, it will prompt you to update things like AGP / Android Gradle Plugin. Dismiss any upgrade prompts as it will probably break the cordova project.

Then I'd add a breakpoint on line https://github.com/apache/cordova-plugin-file/blob/b32be30e4bd012f9d9568a9cc11926b2ad6c14f7/src/android/FileUtils.java#L1016.

And step through to see if it makes it to https://github.com/apache/cordova-plugin-file/blob/b32be30e4bd012f9d9568a9cc11926b2ad6c14f7/src/android/FileUtils.java#L406

The requestAllPaths method is suppose to return a JSONObject which is passed back to the webview.

In fact... I see it doesn't actually call on the callback context on error :face_exhaling: so you won't get an error on the webview side. So I suspect that this routine is hitting into an exception at some point inside requestAllPaths. Once we find out that exception, we probably can get a better idea why it's not working.

globules-io commented 1 year ago

On the native side, real quick, all I see is a bunch of errors like

avc: denied { ioctl } for path="socket:[225593]" dev="sockfs" ino=225593 ioctlcmd=7704 scontext=u:r:system_server:s0 tcontext=u:r:system_server:s0 tclass=unix_stream_socket permissive=0

but I'm not actually sure it's related. At this point, I'd rather have a maintainer trying to debug his own code ....

globules-io commented 1 year ago

https://github.com/apache/cordova-plugin-file/blob/master/src/android/FileUtils.java#L401

} else if (action.equals("requestAllPaths")) {
            cordova.getThreadPool().execute(
                    new Runnable() {
                        public void run() {
                            try {

                                 Log.e(LOG_TAG, "DID NOT SHIT THE BED");
                    callbackContext.success(requestAllPaths());
                } catch (JSONException e) {
                    // TODO Auto-generated catch block
                                       Log.e(LOG_TAG, "SHAT THE BED");
                    e.printStackTrace();
                }
                        }
                    }
            );
        }

I do not see "DID NOT SHIT THE BED" nor "SHAT THE BED" while logcat'ing it so it looks like it doesn't even reach that statement...

breautek commented 1 year ago

but I'm not actually sure it's related. At this point, I'd rather have a maintainer trying to debug his own code ....

I would but unfortunately it's hard to debug when the issue does not occur locally. With that being said this code was last touched about 8 years ago.

avc: denied { ioctl } for path="socket:[225593]" dev="sockfs" ino=225593 ioctlcmd=7704 scontext=u:r:system_server:s0 tcontext=u:r:system_server:s0 tclass=unix_stream_socket permissive=0

Logcat is very verbose (it's essentially the linux syslog) it will output anything and everything that is going on in the system. The debug tab in AS filters logcat to only your app process for you, but if you're using the logcat tab directly, you'll need to manually apply the filters yourself to make it remotely usable.

In AS you can use Breakpoints and "debug" the project (The green icon that looks like a bug) which will build and deploy the project from AS with the debugger attached. Might be easier than using log statements and then trying to look for them in logcat, which can easily be missed. But if the Runnable is not running at all..., then I'd be curious if it's reaching the cordova.getThreadPool().execute( line or if it's even entering the if condition at all...

globules-io commented 1 year ago

I'm even logging the action at https://github.com/apache/cordova-plugin-file/blob/master/src/android/FileUtils.java#L280 and it doesn't even reach it. I'm really not sure what's going on...

breautek commented 1 year ago

I'm even logging the action at https://github.com/apache/cordova-plugin-file/blob/master/src/android/FileUtils.java#L280 and it doesn't even reach it. I'm really not sure what's going on...

Can you provide me the contents of <cordovaProject>/platforms/android/app/src/main/res/xml/config.xml?

globules-io commented 1 year ago
<?xml version='1.0' encoding='utf-8'?>
<widget id="com.test.debug" version="1.0.0" xmlns="http://www.w3.org/ns/widgets" xmlns:android="http://schemas.android.com/apk/res/android" xmlns:cdv="http://cordova.apache.org/ns/1.0">
    <name>Debug</name>
    <description>Debug</description>
    <author email="dev@globules.io" href="https://globules.io">globules.io</author>
    <content src="index.html" />
    <allow-intent href="http://*/*" />
    <allow-intent href="https://*/*" />
    <allow-intent href="tel:*" />
    <allow-intent href="sms:*" />
    <allow-intent href="mailto:*" />
    <allow-intent href="geo:*" />
    <allow-intent href="market:*" />
    <icon density="ldpi" src="res/icons/android/icon-36.png" />
    <icon density="mdpi" src="res/icons/android/icon-48.png" />
    <icon density="hdpi" src="res/icons/android/icon-72.png" />
    <icon density="xhdpi" src="res/icons/android/icon-96.png" />
    <icon density="xxhdpi" src="res/icons/android/icon-144.png" />
    <icon density="xxxhdpi" src="res/icons/android/icon-192.png" />
    <preference name="loglevel" value="DEBUG" />
    <preference name="scheme" value="app" />
    <preference name="hostname" value="com.test.debug" />
    <preference name="Orientation" value="landscape" />
    <preference name="DisallowOverscroll" value="true" />
    <preference name="ShowSplashScreenSpinner" value="false" />
    <preference name="StatusBarBackgroundColor" value="#FFFFFF" />
    <preference name="StatusBarOverlaysWebView" value="false" />
    <preference name="AutoHideSplashScreen" value="true" />
    <preference name="AllowBackForwardNavigationGestures" value="false" />
    <preference name="Allow3DTouchLinkPreview" value="false" />
    <preference name="AndroidWindowSplashScreenAnimatedIcon" value="res/screens/android/icon-288.png" />
    <preference name="AndroidWindowSplashScreenBackground" value="#222" />
    <preference name="android-minSdkVersion" value="23" />
    <preference name="android-targetSdkVersion" value="31" />
    <preference name="android-maxSdkVersion" value="31" />
    <preference name="AndroidPersistentFileLocation" value="Compatibility" />
    <feature name="File">
        <param name="android-package" value="org.apache.cordova.file.FileUtils" />
        <param name="onload" value="true" />
    </feature>
    <allow-navigation href="cdvfile:*" />
</widget>

Some unrelated preferences but that should not affect

breautek commented 1 year ago

Just wanted to confirm that

<feature name="File">
        <param name="android-package" value="org.apache.cordova.file.FileUtils" />
        <param name="onload" value="true" />
    </feature>

was present...

Could you perhaps just zip up your project and I'll dig a bit deeper. None of this is really making any sense but hopefully I can point out a problem in the project structure that at least would explain the behaviour or something. (And if so, the question becomes how/why cordova is producing that kind of state)

In the meantime, can you also ensure that Cordova is completely up to date (including sub dependencies). Easiest way to do that is to simply reinstall cordova, (npm uninstall -g cordova && npm install -g cordova) so that all dependencies will install at the latest satisfactory versions across the entire dependency tree.

globules-io commented 1 year ago

Cordova had already been updated to latest version (11.1.0). I'll see what I can do. Can't waste too much time on this today... Quick note : I am not asked for permissions, which I find weird. Could it be related to permissions ? The plugin only requires

         <config-file target="AndroidManifest.xml" parent="/*">
            <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
        </config-file>

Also, could you as well zip and upload your blank test project? So I can test it on the Android 6.0.1 device I have here? The one that works on the emulator I mean...

breautek commented 1 year ago

Quick note : I am not asked for permissions, which I find weird. Could it be related to permissions ? The plugin only requires

As of API 19, WRITE_EXTERNAL_STORAGE is only required when writing to non-application-specific external directories.

And as of API 29, WRITE_EXTERNAL_STORAGE is completely obsolete and doesn't grant you any special permissions (External storage is locked down pretty tight).

So no, it shouldn't be the cause.

breautek commented 1 year ago

Also, could you as well zip and upload your blank test project? So I can test it on the Android 6.0.1 device I have here? The one that works on the emulator I mean...

Sure thing api23test.zip

globules-io commented 1 year ago

What about https://developer.android.com/reference/android/Manifest.permission#MANAGE_EXTERNAL_STORAGE and https://developer.android.com/reference/android/Manifest.permission#READ_EXTERNAL_STORAGE and https://developer.android.com/reference/android/Manifest.permission#READ_MEDIA_VIDEO

Note: Starting in API level 33, this permission has no effect. If your app accesses other apps' media files, request one or more of these permissions instead: [READ_MEDIA_IMAGES](https://developer.android.com/reference/android/Manifest.permission#READ_MEDIA_IMAGES), [READ_MEDIA_VIDEO](https://developer.android.com/reference/android/Manifest.permission#READ_MEDIA_VIDEO), [READ_MEDIA_AUDIO](https://developer.android.com/reference/android/Manifest.permission#READ_MEDIA_AUDIO). Learn more about the [storage permissions](https://developer.android.com/training/data-storage/shared/media#storage-permission) that are associated with media files.

breautek commented 1 year ago

WRITE_EXTERNAL_STORAGE permission became obsolete in API 29/30 (In API 29, existing apps could opt back to the legacy system, which uses the WRITE_EXTERNAL_STORAGE permission, but if you're using the new scoped storage system, the WRITE_EXTERNAL_STORAGE permission isn't used. In API 30, scoped storage is now forcefully enabled.

In either case, the sister READ_EXTERNAL_STORAGE permission was still used to read non-application external storage files, and that is what is becoming obsolete starting in API 33.

globules-io commented 1 year ago

Ok so your build works here. So the only difference I see, is some preference and the splashscreen. I will strip down until I reach the same config.xml as you, maybe that'll do the trick to identify what's up ...

breautek commented 1 year ago

Ok so your build works here. So the only difference I see, is some preference and the splashscreen. I will strip down until I reach the same config.xml as you, maybe that'll do the trick to identify what's up ...

I would have expected to be the same as your project as it uses the cordova app template. I didn't make any manual changes other than adding the sdk version preferences to match what you had yesterday.

globules-io commented 1 year ago

Started working after I removed

    <preference name="scheme" value="app" />
    <preference name="hostname" value="io.globules.test" />
    <preference name="Orientation" value="landscape" />

At this point, pretty sure it has to do with hostname + scheme. @breautek can you add scheme + hostname on your side and test your build please ?

breautek commented 1 year ago

At this point, pretty sure it has to do with hostname + scheme.

Can't do it right now but just wanted to quickly mention that app is not a valid scheme for android, it must be either http or https. By default it uses https (and I think it falls back to https if it's an invalid scheme)

This is a conflict with what iOS requires, which cannot be any well known/standard protocol, so things like http, https, ssh, etc are all illegal for iOS.

Therefore the preferences should be in their own <platform> blocks

globules-io commented 1 year ago

That was the issue, https works but app does not work. This should throw an error though, not through this plugin but via Cordova, if the scheme is not proper, it should alert the end user.... I will close now. Thanks for your help @breautek

breautek commented 1 year ago

That was the issue, https works but app does not work. This should throw an error though, not through this plugin but via Cordova, if the scheme is not proper, it should alert the end user.... I will close now. Thanks for your help @breautek

Glad we found the root issue,

But yah, this would be an issue in cordova-android if it's not properly falling back or throwing an error. I'll create an issue for it.

globules-io commented 1 year ago

Honestly, I don't see why we would have different schemes... Since we're doing "one code base" approach, you most probably don't want to setup your backend for 2 different schemes. So down the line, I guess it should be limited to http / https. I fail to see a use case where one would need 2 different schemes. If one needs to know what platform is making remote calls, just check the user agent ? Just a thought....

breautek commented 1 year ago

The limitation comes from the underlying native apis.

The ios WKUrlSchemeHandler forbids standard protocols to be used as the app scheme, whereas the android WebViewAssetLoader only supports http or https.

globules-io commented 1 year ago

Well this is my point exactly. http is obsolete, so people would use https by default. Now if iOS only supports its own thing, then why even set a scheme ? For Android only https makes sense... I believe it should be automatic and the preference should be removed.

breautek commented 1 year ago

Now if iOS only supports its own thing, then why even set a scheme ?

iOS will treat the scheme, even custom ones as an origin and will trust the context, whereas loading from the filesystem (as it did in the past) will have the origin set to null, as W3C states it should do when the protocol is file.

Having a real origin will bypass the "requires secure context" check inside the WKWebView. So if you use schemes, browser features that requires the secure context flag will be enabled.

On iOS, the schemes are opt in, because Apple doesn't consider the file-base system as insecure, and switching schemes changes the origin, which web storage containers like cookies and local storage are tied to, so this results a loss of access of what might previously be stored. For some people, that might be a big deal.

Android we default with schemes enabled, but can be opt out for the same reason above (origin changes). Google states using the file-base solution is an insecure approach and thus should be disabled. I agree http is obsolete, but there was a demand for it for few different reasons, so the option for http was added in a later release.

Again, using schemes on android, (and I think this only applies if the scheme is https) will make Google treat the webview document as a secure context, enabling browser features that requires the secure context flag.

Having the scheme customizable (including allowing http for android) also makes it easier for people wanting to remove the ionic webview dependency which did implement schemes for both platforms (for a quite a long time before Cordova did), particularly if they wanted to match the scheme they were using before to avoid a origin change.

globules-io commented 1 year ago

Right, so it's down to the restrictions around Android only, so the scheme preference shouldn't not be by platform, it should be "global" to http or https, and validated as such (or throw an error) and iOS should just default to what we set for Android, see what I mean? Setting anything else for iOS is kind of pointless. We have to fallback onto what Android wants, as from a back end stand point, we don't need different schemes by platform anyways.