565138 - Implement extloader backend for Android

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, enhancement)

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Why does this block the merge into m-c?

Comment 2

[Michael Wu [:mwu]]

We just need a proper stub impl. Hacking the unix impl to turn into a stub as done currently in mozilla-droid doesn't seem very nice.

Comment 3

[Michael Wu [:mwu]]

Attachment: Add Android uriloader stub

Just for the purpose of compiling

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 446849 [details] [diff] [review]
Add Android uriloader stub

Why bother with your own mimeinfo implementation when it doesn't do anything?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

it will eventually, and I believe we have code that depends on this path not throwing exceptions in the front end that break otherwise.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #5)
> it will eventually, and I believe we have code that depends on this path not
> throwing exceptions in the front end that break otherwise.

My point was that nsMIMEInfoAndroid.cpp is not necessary in the old patch, but that's obsolete with the new patch anyway.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 449810 [details] [diff] [review]
patch

Someone else should review all the .java and Android specific parts.
And perhaps biesi could look at this too.

>+    static boolean openUriExternal(String aUriSpec, String aMimeType) {
>+        Intent intent = new Intent(Intent.ACTION_VIEW);
>+        intent.setDataAndType(android.net.Uri.parse(aUriSpec), aMimeType);
>+        intent.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
>+        try {
>+            GeckoApp.surfaceView.getContext().startActivity(intent);
>+            return true;
>+        } catch(ActivityNotFoundException e) {
>+            Log.i("GeckoAppShell", "couln't find activity to open " 
>+                  + aUriSpec +" (" + aMimeType + ")");
'+' should be in the previous line


>+nsAndroidHandlerApp::nsAndroidHandlerApp(nsAString& aName, 
>+					 nsAString& aDescription) :
There are tabs in the code and parameters should be aligned properly.

>+nsresult nsAndroidHandlerApp::SetDetailedDescription(const nsAString & aDescription)
>+{
>+    mDescription.Assign(aDescription);
>+
Use 2 space indentation.


>+nsresult nsAndroidHandlerApp::Equals(nsIHandlerApp *aHandlerApp, PRBool *_retval NS_OUTPARAM)
Why NS_OUTPARAM?

s/_retval/aRetVal/ 


>+{
>+  nsCOMPtr<nsAndroidHandlerApp> aApp = do_QueryInterface(aHandlerApp);
>+  *_retval = aApp.get() != nsnull && aApp->mName.Equals(mName) && 
>+    aApp->mDescription.Equals(mDescription);
aFoo naming is reserved for parameters. So please rename aApp.
And why you need aApp.get() != nsnull? You can just null check the nsCOMPtr
variable.

>+#include <android/log.h>
>+#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "GeckoUriLoader", args)
>+
>+NS_IMPL_ISUPPORTS2(nsMIMEInfoAndroid, nsIMIMEInfo, nsIHandlerInfo)
>+
>+nsMIMEInfoAndroid::~nsMIMEInfoAndroid()
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::LaunchDefaultWithFile(nsIFile* aFile)
>+{
>+    LaunchWithFile(aFile);
>+    return NS_OK;
Use 2 space indentation. Also elsewhere in the file.

>+nsMIMEInfoAndroid* 
>+nsMIMEInfoAndroid::GetMimeInfoForMimeType(const nsACString& aMimeType) {
>+    mozilla::AndroidBridge* bridge = mozilla::AndroidBridge::Bridge();
>+    jobject obj = bridge->GetHandlersForMimeType(nsCAutoString(aMimeType).get());
>+    jobjectArray arr = static_cast<jobjectArray>(obj);
>+    jsize len = bridge->JNI()->GetArrayLength(arr);
>+    nsresult rv;
>+    nsCOMPtr<nsIMutableArray> handlerApps =
>+        do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, nsnull);
>+    nsString empty = NS_LITERAL_STRING("");
We have EmptyString()

>+    if (len == 0)
>+        return nsnull;
>+
>+    for (jsize i = 0; i < len; i++) {
>+        jstring jstr = static_cast<jstring>(bridge->JNI()->GetObjectArrayElement(arr, i));
>+        const PRUnichar* str = bridge->JNI()->GetStringChars(jstr, false);
>+        nsString name(str);
>+        LOG(NS_ConvertUTF16toUTF8(name).get());
>+        nsAndroidHandlerApp* app = new nsAndroidHandlerApp(name, empty);
>+        handlerApps->AppendElement(app, PR_FALSE);
>+        bridge->JNI()->ReleaseStringChars(jstr, str);
>+        
I know nothing about this Java/JNI stuff

>+    }
>+    nsMIMEInfoAndroid *info = new nsMIMEInfoAndroid(aMimeType);
>+    info->mHandlerApps = handlerApps;
>+    PRUint32 length;
>+    handlerApps->GetLength(&length);
>+    return info;
Please make this and GetMimeInfoForFileExt to return
already_AddRefed<nsMIMEInfoAndroid>



>+}
>+
>+nsMIMEInfoAndroid* 
>+nsMIMEInfoAndroid::GetMimeInfoForFileExt(const nsACString& aFileExt) {
'{' should be in the next line.

>+    nsCString aMimeType;
The variable name shouldn't be in form aFoo

>+    nsCString ext(aFileExt);
Why you need this? Could you perhaps make 
GetMimeTypeFromExtension to take const nsACString& as a parameter?

>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::GetDescription(nsAString_internal& aDesc)
>+{
>+    aDesc.Assign(mDescription);
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::SetDescription(const nsAString_internal& aDesc)
>+{
>+    mDescription.Assign(aDesc);
>+    return NS_OK;
>+}
Huh, _internal? Why? nsAString& should work just fine. 


>+
>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::GetPreferredApplicationHandler(nsIHandlerApp** aApp)
>+{
>+    nsCOMPtr<nsIHandlerApp> tmp = do_QueryElementAt(mHandlerApps, 0);
>+    *aApp = tmp.forget().get();
or tmp.swap(*aApp);


>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::GetHasDefaultHandler(PRBool* hasDefault)
aHasDefault


>+{
>+    LOG("get has default handler");
>+    PRUint32 len;
>+    *hasDefault = PR_FALSE;
>+    if (mHandlerApps.get() == nsnull) {
Why not just
if (!mHandlerApps) {


>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::SetFileExtensions(const nsACString & aExtensions)
>+{    
>+    mExtensions.Clear();
>+    nsCString extList( aExtensions );
>+    
>+    PRInt32 breakLocation = -1;
>+    while ( (breakLocation= extList.FindChar(',') )!= -1)
>+    {
'{' should be in the previous line.

>+        mExtensions.AppendElement(Substring(extList.get(), extList.get() + breakLocation));
>+        extList.Cut(0, breakLocation+1 );
add space before and after '+', and remove space before ')'

>+    }
>+    if ( !extList.IsEmpty() )
Remove space after '(' and before ')'

>+        mExtensions.AppendElement( extList );
ditto

>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::ExtensionExists(const nsACString & aExtension, PRBool *_retval NS_OUTPARAM)
aRetVal and no need for NS_OUTPARAM


>+NS_IMETHODIMP
>+nsMIMEInfoAndroid::Equals(nsIMIMEInfo *aMIMEInfo, PRBool *_retval NS_OUTPARAM)
Same here.


>+class nsMIMEInfoAndroid : public nsIMIMEInfo
>+{
>+public:
>+    static nsMIMEInfoAndroid* GetMimeInfoForMimeType(const nsACString& aMimeType);
>+    static nsMIMEInfoAndroid* GetMimeInfoForFileExt(const nsACString& aFileExt);
>+    
>+NS_DECL_ISUPPORTS
>+NS_DECL_NSIMIMEINFO
>+NS_DECL_NSIHANDLERINFO
Use 2 space indentation in all new code.


>+
>+private:
>+    nsMIMEInfoAndroid(const char *aType = "");
>+    nsMIMEInfoAndroid(const nsACString& aMIMEType, nsCOMPtr<nsIMutableArray>);
No parameter name for the latter parameter?


>+already_AddRefed<nsIMIMEInfo>
>+nsOSHelperAppService::GetMIMEInfoFromOS(const nsACString& aMIMEType,
>+                                        const nsACString& aFileExt,
>+                                        PRBool* aFound)
>+{
>+    LOG("nsOSHelperAppService::GetMIMEInfoFromOS (%s, %s)", 
>+        nsDependentCString(aMIMEType).get(), 
>+        nsDependentCString(aFileExt).get());
>+    *aFound = PR_FALSE;
>+    nsIMIMEInfo* mimeInfo = nsnull;
Make this nsCOMPtr.

>+    mimeInfo = nsMIMEInfoAndroid::GetMimeInfoForMimeType(aMIMEType);
>+
>+    if (!mimeInfo)
>+        mimeInfo = nsMIMEInfoAndroid::GetMimeInfoForFileExt(aFileExt);
>+
>+    *aFound = !!mimeInfo;
>+
>+    NS_IF_ADDREF(mimeInfo);
Remove this

>+    LOG("GetMimeTypeFromOS (%p), %s", mimeInfo, *aFound ? "true" : "false");
>+    return mimeInfo;
and then
return mimeInfo.forget();

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 449810 [details] [diff] [review]
patch

>+        Log.i("GeckoMime", "length: " + list.size());
>+        String[] ret = new String[list.size()];
>+        for (int i = 0; i < list.size(); i++) {
>+            Log.i("GeckoMime", list.get(i).loadLabel(pm).toString());
Do you need this logging anymore?

>+nsresult nsAndroidHandlerApp::LaunchWithURI(nsIURI *aURI, nsIInterfaceRequestor *aWindowContext)
>+{
>+  return NS_OK;
>+}
Nobody calls this?

>+nsMIMEInfoAndroid* 
>+nsMIMEInfoAndroid::GetMimeInfoForMimeType(const nsACString& aMimeType) {
>+    mozilla::AndroidBridge* bridge = mozilla::AndroidBridge::Bridge();
>+    jobject obj = bridge->GetHandlersForMimeType(nsCAutoString(aMimeType).get());
The local frame in bridge does not work in this case because that local frame is already lost by the time this object is returned.

I recommend requiring the caller to make the local frame. Returning a global reference will also work but probably isn't worth the extra complexity.

Also, what happens when the array allocation on the java side fails?

>@@ -212,6 +214,42 @@
>     mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jNotifyXreExit);
> }
> 
>+jobject
>+AndroidBridge::GetHandlersForMimeType(const char *aMimeType) 
>+{
>+    ALOG("getting handlers for (%s)", aMimeType);
>+    AutoLocalJNIFrame jniFrame;
>+    jstring jstr = mJNIEnv->NewStringUTF(aMimeType);
Hmm.. I just realized we don't do proper error checking on a lot of the stuff in this file..

Also, Java doesn't really give you UTF8. It has a modified version of UTF8. UTF16 is the only correct way to work with java strings unless you're sure your UTF8 string happens to match Java's modified UTF8. (easiest way by being all ascii)

>diff -r 44b5c29b74cb -r 02c74b8842a0 widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h	Sat Jun 05 14:24:32 2010 -0400
>+++ b/widget/src/android/AndroidBridge.h	Tue Jun 08 02:31:04 2010 -0400
>@@ -102,6 +102,12 @@
>     void SetSurfaceView(jobject jobj);
>     AndroidGeckoSurfaceView& SurfaceView() { return mSurfaceView; }
> 
>+    jobject GetHandlersForMimeType(const char *aMimeType);
>+
>+    PRBool OpenUriExternal(nsCString& aUriSpec, nsCString& aMimeType);
bool is more popular these days.

>diff -r 44b5c29b74cb -r 02c74b8842a0 xpcom/io/SpecialSystemDirectory.cpp
>--- a/xpcom/io/SpecialSystemDirectory.cpp	Sat Jun 05 14:24:32 2010 -0400
>+++ b/xpcom/io/SpecialSystemDirectory.cpp	Tue Jun 08 02:31:04 2010 -0400
>@@ -648,7 +648,11 @@
>                     if (!tPath || !*tPath) {
>                         tPath = PR_GetEnv("TEMP");
>                         if (!tPath || !*tPath) {
>+#if defined(ANDROID)
>+                            tPath = "/cache/";
>+#else
>                             tPath = "/tmp/";
>+#endif
>                         }
>                     }
>                 }
???

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

> >+nsresult nsAndroidHandlerApp::LaunchWithURI(nsIURI *aURI, nsIInterfaceRequestor *aWindowContext)
> >+{
> >+  return NS_OK;
> >+}
> Nobody calls this?

Fennec never calls this since it has a simplifies UI that lacks a helper app chooser. This will have to be implemented for Firefox to work properly.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 450761 [details] [diff] [review]
patch

looks fine to me; one question though, it looks like $EXTERNAL_STORAGE gets used as the download dir... shouldn't that be $EXTERNAL_STORAGE/Downloads?  Can get fixed up later, just wondering.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 450799 [details] [diff] [review]
patch

carrying the review, just fixed a typo (interdiff is pretty broken)

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 450799 [details] [diff] [review]
patch

>     // SpecialSystemDirectory.cpp sets its TMPDIR from this env var
>-    setenv("TMPDIR", "/data/data/org.mozilla." MOZ_APP_NAME, 1);

Should delete the coment too.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This is causing some crashes on startup for me; crash and stack trace are below:

My guess is that the problem is that GetHandlesForMimeType creates its own AutoLocalJNIFrame, and the object is created within that frame.  That frame then gets deleted, thus freeing all temporaries, including this object.  It doesn't matter that there's an outer one -- they're separate nested scopes.

W/dalvikvm( 2352): JNI WARNING: 0x4410db68 is not a valid JNI reference
W/dalvikvm( 2352):              in Ldalvik/system/NativeStart;.run ()V (GetArrayLength)

at:

#0  0xad0356e4 in dvmAbort () at dalvik/vm/Init.c:1650
#1  0xad0272b6 in abortMaybe () at dalvik/vm/CheckJni.c:295
#2  0xad02828c in checkObject (env=0x13d980, jobj=0x4410db68, func=0xad068834 "Check_GetArrayLength")
    at dalvik/vm/CheckJni.c:490
#3  0xad02a38c in checkArray (env=0x13d980, jarr=0x4410db68, func=0xad068834 "Check_GetArrayLength")
    at dalvik/vm/CheckJni.c:708
#4  0xad02af66 in Check_GetArrayLength (env=0x13d980, array=0x4410db68) at dalvik/vm/CheckJni.c:1854
#5  0x8263afbc in GetArrayLength (aMimeType=...)
    at /home/vladimir/proj/android/android-ndk-r3-crystax/build/platforms/android-5/arch-arm/usr/include/jni.h:852
#6  nsMIMEInfoAndroid::GetMimeInfoForMimeType (aMimeType=...)
    at ../../../mozilla-central/uriloader/exthandler/android/nsMIMEInfoAndroid.cpp:74
#7  0x8263a658 in nsOSHelperAppService::GetMIMEInfoFromOS (this=<value optimized out>, aMIMEType=..., 
    aFileExt=..., aFound=0x46ebf634)
    at ../../../mozilla-central/uriloader/exthandler/android/nsOSHelperAppService.cpp:56
#8  0x82635aac in nsExternalHelperAppService::GetTypeFromExtension (this=<value optimized out>, 
    aFileExt=..., aContentType=...)
    at ../../../mozilla-central/uriloader/exthandler/nsExternalHelperAppService.cpp:2607
#9  0x82635486 in nsExternalHelperAppService::GetTypeFromFile (this=0x48c56850, 
    aFile=<value optimized out>, aContentType=...)
    at ../../../mozilla-central/uriloader/exthandler/nsExternalHelperAppService.cpp:2743
#10 0x821e63c2 in nsFileChannel::MakeFileInputStream (this=<value optimized out>, file=0x49e19f20, 
    stream=<value optimized out>, contentType=<value optimized out>)
    at ../../../../mozilla-central/netwerk/protocol/file/nsFileChannel.cpp:301
#11 0x821e668e in nsFileChannel::OpenContentStream (this=0x49aeb340, async=<value optimized out>, 
    result=<value optimized out>, channel=<value optimized out>)
    at ../../../../mozilla-central/netwerk/protocol/file/nsFileChannel.cpp:370
#12 0x821ade46 in nsBaseChannel::BeginPumpingData (this=0x49aeb340)
    at ../../../../mozilla-central/netwerk/base/src/nsBaseChannel.cpp:228

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 450799 [details] [diff] [review]
patch

Looking at this some more, nsMIMEInfoAndroid::GetMimeInfoForMimeType is the only method that actually has to do JNI goop inside nsMIMEInfoAndroid.

Can you just make the bridge return (or fill a passed-in) nsTArray of strings?  That way JNI stuff doesn't get spread troughout the code.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Once I hacked around the above:

W/dalvikvm( 2745): JNI WARNING: received null jstring (Check_GetStringChars)
I/dalvikvm( 2745): "GeckoThread" prio=5 tid=43 NATIVE
I/dalvikvm( 2745):   | group="main" sCount=0 dsCount=0 s=N obj=0x4410ad50 self=0x11b8b0
I/dalvikvm( 2745):   | sysTid=2766 nice=0 sched=0/0 cgrp=default handle=1222936
I/dalvikvm( 2745):   at dalvik.system.NativeStart.run(Native Method)
I/dalvikvm( 2745):   at dalvik.system.NativeStart.run(Native Method)


I believe that the mime type that's being given is an empty string, e.g. length == 0.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Here's the stack for the above.  I take back the empty string -- wFileExt is "js".  jstrType needs a null check before calling GetStringChars on it in GetMimeTypeFromExtension.

#0  0xad0356e4 in dvmAbort () at dalvik/vm/Init.c:1650
#1  0xad0272b6 in abortMaybe () at dalvik/vm/CheckJni.c:295
#2  0xad02a07e in checkString (env=0x12d018, jstr=0x0, func=0xad069268 "Check_GetStringChars") at dalvik/vm/CheckJni.c:565
#3  0xad02eeee in Check_GetStringChars (env=0x12d018, string=0x0, isCopy=0x0) at dalvik/vm/CheckJni.c:1762
#4  0x8270efb0 in GetStringChars (this=0x46614040, aFileExt=<value optimized out>, aMimeType=<value optimized out>)
    at /home/vladimir/proj/android/android-ndk-r3-crystax/build/platforms/android-5/arch-arm/usr/include/jni.h:834
#5  mozilla::AndroidBridge::GetMimeTypeFromExtension (this=0x46614040, aFileExt=<value optimized out>, aMimeType=<value optimized out>)
    at ../../../../mozilla-central/widget/src/android/AndroidBridge.cpp:249
#6  0x8263af82 in nsMIMEInfoAndroid::GetMimeInfoForFileExt (aFileExt=...) at ../../../mozilla-central/uriloader/exthandler/android/nsMIMEInfoAndroid.cpp:106
#7  0x8263a660 in nsOSHelperAppService::GetMIMEInfoFromOS (this=<value optimized out>, aMIMEType=<value optimized out>, aFileExt=..., aFound=0x46dbf634)
    at ../../../mozilla-central/uriloader/exthandler/android/nsOSHelperAppService.cpp:58
#8  0x82635aac in nsExternalHelperAppService::GetTypeFromExtension (this=<value optimized out>, aFileExt=..., aContentType=...)
    at ../../../mozilla-central/uriloader/exthandler/nsExternalHelperAppService.cpp:2607

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451094 [details] [diff] [review]
patch

Looks fine, though:

>+nsresult nsAndroidHandlerApp::LaunchWithURI(nsIURI *aURI, nsIInterfaceRequestor *aWindowContext)
>+{
>+  nsCAutoString uristr;
>+  aURI->GetAsciiSpec(uristr);
>+  return NS_OK;
>+}

^ ???

> void
>+AndroidBridge::GetHandlersForMimeType(const char *aMimeType, nsStringArray* aStringArray)
>+{
>+    AutoLocalJNIFrame jniFrame;
>+    NS_ConvertUTF8toUTF16 wMimeType(aMimeType);
>+    jstring jstr = mJNIEnv->NewString(wMimeType.get(), wMimeType.Length());
>+    jobject obj = mJNIEnv->CallStaticObjectMethod(mGeckoAppShellClass, 
>+                                                  jGetHandlersForMimeType, 
>+                                                  jstr);
>+    jobjectArray arr = static_cast<jobjectArray>(obj);
>+    jsize len = mJNIEnv->GetArrayLength(arr);
>+    for (jsize i = 0; i < len; i+=2) {
>+        jstring jstr = static_cast<jstring>(mJNIEnv->GetObjectArrayElement(arr, i));
>+        nsJNIString jniStr(jstr);
>+        aStringArray->AppendString(jniStr);
>+    } 
>+}

Might want to add some defense here, e.g. null-checking the return value array.

>+nsJNIString::nsJNIString(jstring jstr)
>+{
>+    const jchar* jCharPtr = JNI()->GetStringChars(jstr, false);
>+    nsresult rv;
>+    Assign(jCharPtr);
>+    JNI()->ReleaseStringChars(jstr, jCharPtr);
>+
>+}

This needs to null-check jstr, and use SetIsVoid(PR_TRUE) if it's null (instead of crashing :-)

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/0d15abb54498

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #24)
> pushed http://hg.mozilla.org/mozilla-central/rev/0d15abb54498

... but without the review comment fixes :(