Closed Bug 709250 Opened 13 years ago Closed 12 years ago

Error downloading favicon: java.net.MalformedURLException: java.lang.NullPointerException: Cannot find "!/"

Categories

(Firefox for Android Graveyard :: General, defect, P3)

ARM
Android
defect

Tracking

(firefox11 affected, firefox12 affected, firefox13 affected, firefox14 verified, firefox15 verified, blocking-fennec1.0 +, fennec11+)

VERIFIED FIXED
Tracking Status
firefox11 --- affected
firefox12 --- affected
firefox13 --- affected
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +
fennec 11+ ---

People

(Reporter: aaronmt, Assigned: wesj)

References

Details

Attachments

(3 files, 1 obsolete file)

D/GeckoFavicons( 4513): Downloading favicon for URL = about:home with favicon URL = jar:jar:file:///data/app/org.mozilla.fennec-2.apk!/omni.ja!/chrome/chrome/content/branding/favicon32.png

D/GeckoFavicons( 4513): Error downloading favicon: java.net.MalformedURLException: java.lang.NullPointerException: Cannot find "!/"

--
Samsung Nexus S (Android 2.3.6)
20111209105120
http://hg.mozilla.org/integration/mozilla-inbound/rev/8b622aa1c57f
jar:jar:file:///data/app/org.mozilla.fennec-2.apk!/omni.ja!/chrome/chrome/content/branding/favicon32.png

* jar:jar:
* omni.ja! instead of omni.jar 

What's the regression range? The changeset you mention is OSX only
I think this might be related to bug 695843
We use this code to convert a chrome:// URI to a jar: URI so we can access resources in the APK using Java:

let registry = Cc['@mozilla.org/chrome/chrome-registry;1'].getService(Ci["nsIChromeRegistry"]);
return registry.convertChromeURL(Services.io.newURI(aURI, null, null)).spec;

The returned URI seems wrong now
The returned URI is right, since omni.jar is now named omni.ja (bug 701875) and is now nested (bug 708525 ; thus the jar:jar: url)

What this means is that the java code doesn't handle the situation.
Is it a native ui issue only or does it affect xul too ?
(In reply to Mike Hommey [:glandium] from comment #4)
> The returned URI is right, since omni.jar is now named omni.ja (bug 701875)
> and is now nested (bug 708525 ; thus the jar:jar: url)

bug 695843, not bug 708525.
(In reply to Mark Finkle (:mfinkle) from comment #3)
> We use this code to convert a chrome:// URI to a jar: URI so we can access
> resources in the APK using Java:
> 
> let registry =
> Cc['@mozilla.org/chrome/chrome-registry;1'].
> getService(Ci["nsIChromeRegistry"]);
> return registry.convertChromeURL(Services.io.newURI(aURI, null, null)).spec;
> 
> The returned URI seems wrong now

Note that the code following this to convert resource:// uris will have the same problem.

If you want to "backout" bug 695843 until something is figured for this bug, you can just back out the packager.mk bits of part 10 of that bug (so, just https://hg.mozilla.org/mozilla-central/diff/792a9ba2aa26/toolkit/mozapps/installer/packager.mk ), or make these parts native fennec only (since xul fennec seems to be unaffected).
Assignee: nobody → mark.finkle
Priority: -- → P3
tracking-fennec: --- → 11+
Attached file Log.
Just got this on a current m-c build on my S2. Fennec died, so startup crasher?
(In reply to Richard Newman [:rnewman] from comment #8)
> Created attachment 587576 [details]
> Log.
> 
> Just got this on a current m-c build on my S2. Fennec died, so startup
> crasher?

Should not be a crasher. The exception is caught, but logged.

I tried coming up with a way to convert the chrome: URI into a data: URI and sending that across. It was complicated and slow.

I think we could try to fix this by adding support for nested JARs to the Java side. If that doesn't work, we could actually add the same PNGs to the Java resources and just map them to the chrome URIs. More duplication, I know :(
Confirming, yes, it's not a crasher; but still reproducible.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Assignee: mark.finkle → lucasr.at.mozilla
Attached patch Patch (obsolete) — Splinter Review
This implements a nested jar reader for Gecko. It applies on top of my bookmarks patch in bug 728224 and removes the code there that inserted a default set of favicons for about pages.

There is some delay while we load these on the initial start.
Assignee: lucasr.at.mozilla → wjohnston
Attachment #607353 - Flags: review?(mark.finkle)
Comment on attachment 607353 [details] [diff] [review]
Patch


>diff --git a/mobile/android/base/Favicons.java b/mobile/android/base/Favicons.java

>             } catch (Exception e) {
>-                Log.d(LOGTAG, "Error downloading favicon: " + e);
>-                Bitmap bitmap = getFallback(mContext, mPageUrl);
>-                return new BitmapDrawable(bitmap);
>+                if (mFaviconUrl.startsWith("jar:jar:")) {
>+                    InputStream stream = GeckoJarReader.GetStream(mFaviconUrl);
>+                    if (stream != null) {
>+                        image = new BitmapDrawable(stream);
>+                    } else {
>+                        Log.d(LOGTAG, "Error getting favicon from jar: " + e);
>+                    }
>+                } else {
>+                    Log.d(LOGTAG, "Error downloading favicon: " + e);
>+                }

You could try to see if the URL starts with "jar:jar:" before we throw an exception. It might be a little faster if we can skip any work that happens before the exception.

>diff --git a/mobile/android/base/GeckoJarReader.java b/mobile/android/base/GeckoJarReader.java
>+        Stack<String> jarurl = parseUrl(aUrl, null);

You could create two "parseUrl" methods. One that only takes a URL and one that takes a URL and a Stack. It would be a bit nicer for callers.

r+, with the nits addressed

Also, sending to Lucas for a double check on the GeckoJarParser code.

Lastly, can you make a quick test too? Just to test the GeckoJarParser code. Use one of the known resources in the JAR.
Attachment #607353 - Flags: review?(mark.finkle)
Attachment #607353 - Flags: review?(lucasr.at.mozilla)
Attachment #607353 - Flags: review+
Comment on attachment 607353 [details] [diff] [review]
Patch

Review of attachment 607353 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I'd like to see a cleaned up version (with a separate patch removing the favicon fallback code?) before giving r+.

::: mobile/android/base/AwesomeBarTabs.java
@@ +156,5 @@
>              byte[] b = cursor.getBlob(faviconIndex);
>              ImageView favicon = (ImageView) view;
>  
>              if (b == null) {
> +                favicon.setImageBitmap(null);

This looks unrelated to your patch to add nested jar support. Do it in a separate patch?

::: mobile/android/base/Favicons.java
@@ +295,5 @@
>                      byteStream = new ByteArrayInputStream(bytes);
>                      image = (BitmapDrawable) Drawable.createFromStream(byteStream, "src");
>                  }
>              } catch (Exception e) {
> +                if (mFaviconUrl.startsWith("jar:jar:")) {

You're checking for jar:jar prefix in two places. Doing it only in GeckoJarReader should be enough, right? You're already covering the case when it returns null here.

@@ +300,5 @@
> +                    InputStream stream = GeckoJarReader.GetStream(mFaviconUrl);
> +                    if (stream != null) {
> +                        image = new BitmapDrawable(stream);
> +                    } else {
> +                        Log.d(LOGTAG, "Error getting favicon from jar: " + e);

"from nested jar"?

::: mobile/android/base/GeckoJarReader.java
@@ +9,5 @@
> +import java.util.jar.JarFile;
> +import java.util.Stack;
> +import java.util.zip.ZipFile;
> +import java.util.zip.ZipInputStream;
> +import java.util.zip.ZipEntry;

Hmm, shouldn't you use JarFile, JarInputStream, JarEntry instead of Zip*. Not sure if makes any difference but it feels like the natural thing to do I guess.

@@ +22,5 @@
> +/* Reads out of a multiple level deep jar file such as
> + *  jar:jar:file:///data/app/org.mozilla.fennec.apk!/omni.ja!/chrome/chrome/content/branding/favicon32.png
> + */
> +public class GeckoJarReader {
> +    static private String LOGTAG = "GeckoJarReader";

nit: add empty line here.

@@ +23,5 @@
> + *  jar:jar:file:///data/app/org.mozilla.fennec.apk!/omni.ja!/chrome/chrome/content/branding/favicon32.png
> + */
> +public class GeckoJarReader {
> +    static private String LOGTAG = "GeckoJarReader";
> +    static public InputStream GetStream(String aUrl) {

getStream() for consistency. We don't usually do the 'a' prefix in our Java code. url would be more consistent but no big deal.

@@ +29,5 @@
> +            Log.i(LOGTAG, "Invalid url " + aUrl);
> +            return null;
> +        }
> +
> +        Stack<String> jarurl = parseUrl(aUrl, null);

nit: jarUrl for consistency. Actually it should be more like jarUrls, right?

@@ +34,5 @@
> +        ZipInputStream zis = null;
> +
> +        try {
> +            // Load the initial jar file as a zip
> +            URL fileurl = new URL(jarurl.pop());

nit: fileUrl for consistency.

@@ +42,5 @@
> +
> +            // loop through children jar files until we reach the innermost one
> +            while (jarurl.peek() != null) {
> +                String fileName = jarurl.pop();
> +                Log.i(LOGTAG, "Opening " + fileName);

nit: add empty line here.

@@ +63,5 @@
> +                    Log.i(LOGTAG, "No Entry for " + fileName);
> +                    return null;
> +                }
> +            }
> +        } catch (java.util.EmptyStackException ex) {

Maybe log just for debugging purposes?

@@ +64,5 @@
> +                    return null;
> +                }
> +            }
> +        } catch (java.util.EmptyStackException ex) {
> +        } catch (java.io.IOException ex) {

import java.io.IOException and use IOException here.

@@ +80,5 @@
> +            entry = zipstream.getNextEntry();
> +            while(entry != null && !entry.getName().equals(entryName)) {
> +                entry = zipstream.getNextEntry();
> +            }
> +        } catch (java.io.IOException ex) {

Import java.io.IOException and use IOException here.

@@ +82,5 @@
> +                entry = zipstream.getNextEntry();
> +            }
> +        } catch (java.io.IOException ex) {
> +            Log.i(LOGTAG, "Exception " + ex);
> +        }

nit: add empty line here.

@@ +95,5 @@
> +     *    file:///data/app/org.mozilla.fennec.apk
> +     *    omni.ja
> +     *    chrome/chrome/content/branding/favicon32.png
> +     */
> +    static private Stack<String> parseUrl(String aUrl, Stack<String> aParent) {

I agree with mfinkle here. Add a parseUrl() that only gets a url and default aParent to new Stack<String>(). aParent feels like a wrong name for this argument. Isn't it more like parts or elements or something?

@@ +101,5 @@
> +            aParent = new Stack<String>();
> +        }
> +
> +        if (aUrl.startsWith("jar:")) {
> +            int jarend = aUrl.lastIndexOf("!");

nit: jarEnd for consistency.

@@ +102,5 @@
> +        }
> +
> +        if (aUrl.startsWith("jar:")) {
> +            int jarend = aUrl.lastIndexOf("!");
> +            String substr = aUrl.substring(4, jarend);

nit: subStr for consistency. Can you safely asume jarEnd will be a valid index here?
Attachment #607353 - Flags: review?(lucasr.at.mozilla) → review-
Attached patch PatchSplinter Review
I... did things a bit different than the comments. Happy to change most of them, but wanted to argue a bit first.

> Looks good but I'd like to see a cleaned up version (with a separate patch
> removing the favicon fallback code?) before giving r+.

Favicon bits were put in in bug 728224, so I removed them there.

> You're checking for jar:jar prefix in two places. Doing it only in
> GeckoJarReader should be enough, right? You're already covering the case
> when it returns null here.

I don't think this Jar reader really cares if we're using jar:jar, so I decided to remove this check in there, but left it in Favicons.java. A startsWith("jar:") check might be better there though.

> Hmm, shouldn't you use JarFile, JarInputStream, JarEntry instead of Zip*.
> Not sure if makes any difference but it feels like the natural thing to do I
> guess.

Yeah, the only difference is that JARFile extends ZIPFile by adding some metadata stuff. I figured using ZIP was probably cleaner in terms of speed and memory, and we don't need that info (yet).

> I agree with mfinkle here. Add a parseUrl() that only gets a url and default
> aParent to new Stack<String>(). aParent feels like a wrong name for this
> argument. Isn't it more like parts or elements or something?

Done, and changed this to "results".

Test coming...
Attachment #607353 - Attachment is obsolete: true
Attachment #608128 - Flags: review?(lucasr.at.mozilla)
Attached patch testSplinter Review
Pretty simple tests. Tries to open a jar:jar:file:///....fennec.apk!/omni.ja!/something url and checks that an InputStream is returned (does not check the contents of the stream). Also checks with an invalid jar and an invalid path inside the jar to make sure those return null.
Attachment #608129 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 608128 [details] [diff] [review]
Patch

Review of attachment 608128 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

::: mobile/android/base/Favicons.java
@@ +296,5 @@
>                      byteStream = new ByteArrayInputStream(bytes);
>                      image = (BitmapDrawable) Drawable.createFromStream(byteStream, "src");
>                  }
>              } catch (Exception e) {
> +                if (mFaviconUrl.startsWith("jar:jar:")) {

nit: add a comment explaining that if image download fails it might be because we're dealing with a nested jar file.

::: mobile/android/base/GeckoJarReader.java
@@ +23,5 @@
> +    static private String LOGTAG = "GeckoJarReader";
> +
> +    static public InputStream getStream(String url) {
> +        Stack<String> jarUrls = parseUrl(url);
> +        ZipInputStream inputstream = null;

inputStream for consistency.

@@ +43,5 @@
> +                    entry = zip.getEntry(fileName);
> +                }
> +
> +                // if there is nothing else on the stack, this will throw and break us out of the loop
> +                jarUrls.peek();

nit: add empty line here.

@@ +61,5 @@
> +        } catch (IOException ex) {
> +            Log.e(LOGTAG, "Exception ", ex);
> +        } catch (Exception ex) {
> +            Log.e(LOGTAG, "Exception ", ex);
> +        }

nit: add empty line here.

@@ +67,5 @@
> +    }
> +
> +    /* Searches through a ZipInputStream for an entry with a given name */
> +    static private ZipEntry getEntryFromStream(ZipInputStream zipstream, String entryName) {
> +        ZipEntry entry = null;

nit: add empty line here.

@@ +75,5 @@
> +                entry = zipstream.getNextEntry();
> +            }
> +        } catch (IOException ex) {
> +            Log.i(LOGTAG, "Exception " + ex);
> +        }

nit: add empty line here.

@@ +95,5 @@
> +
> +    static private Stack<String> parseUrl(String url, Stack<String> results) {
> +        if (results == null) {
> +            results = new Stack<String>();
> +        }

nit: add empty line here.
Attachment #608128 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 608129 [details] [diff] [review]
test

Review of attachment 608129 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: mobile/android/base/tests/testJarReader.java.in
@@ +17,5 @@
> +/**
> + * A basic jar reader test. Tests reading a png from fennec's apk, as well
> + * as loading some invalid jar urls.
> + */
> +public class testJarReader extends BaseTest {

Not in your patch but I wonder if we should be inheriting from BaseTest when you test doesn't mess with UI at all. For content provider tests I inherited from AndroidTestCase. I think we should probably have a separate infra for UI-less unit test like this one. Could you please file a bug to start discussion on that?

@@ +38,5 @@
> +
> +            // Test looking for an non-existant file in a jar
> +            url = "jar:file://" + getActivity().getApplication().getPackageResourcePath() + "2!/omni.ja";
> +            stream = (InputStream)getStreamMethod.invoke(null, "jar:" + url + "!/chrome/chrome/content/branding/nonexistant_file.png");
> +            mAsserter.is(stream, null, "JarReader returned null for non-existant file in jar");

Maybe also test when the jar url has a weird "!!"? It should fail in that case I guess?
Attachment #608129 - Flags: review?(lucasr.at.mozilla) → review+
Depends on: 738421
Blocks: 739215
Status: NEW → ASSIGNED
This was re-enabled in bug 738421
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
It seems that this error doesn't occur anymore on the latest Nightly and Aurora builds.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-28)
Firefox 14.0a2 (2012-05-28)

Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: