708525 - about: pages do not load favicons

Status: VERIFIED FIXED

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

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

The favicon loading fails because we try to turn "about:foo" into a Java URL, which fails, and we are casting the faviconURL connection to HTTP, whcih fails for a "jar:" URL.

This patch only tries to turn the page URL into a Java URL if we need it for the fallback favicon.ico, which about: pages can't use anyway.

The patch also uses a simpler URLConnection as long as possible, only casting to an HttpURLConnection when we need to disconnect. That is not supported in URLConnection itself.

Tested and about:home now loads a favicon. All http favicons still work too, including "favicon.ico" fallbacks.

Comment 1

[Lucas Rocha (:lucasr)]

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

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

r+ with nits fixed and questions answered.

::: mobile/android/base/Favicons.java
@@ +298,5 @@
>              ByteArrayInputStream byteStream = null;
>              BitmapDrawable image = null;
>  
>              try {
> +                urlConnection = faviconUrl.openConnection();

You're generalizing this code to handle different types of connections such as what? Just checking.

@@ +313,5 @@
>                  }
>              } catch (Exception e) {
>                  Log.d(LOGTAG, "Error downloading favicon: " + e);
>              } finally {
> +                if (urlConnection != null && urlConnection instanceof HttpURLConnection) {

Is it enough to only cover HttpURLConnection here? What about, say, HttpsURLConnection?

@@ +316,5 @@
>              } finally {
> +                if (urlConnection != null && urlConnection instanceof HttpURLConnection) {
> +                    HttpURLConnection httpConnection = (HttpURLConnection) urlConnection;
> +                    httpConnection.disconnect();
> +                }

Add empty line here.

@@ +355,5 @@
> +                        pageUrl = new URL(mPageUrl);
> +                    } catch (MalformedURLException e) {
> +                        Log.d(LOGTAG, "The provided URL is not valid: " + e);
> +                        return null;
> +                    }

Add empty line here.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Lucas Rocha (:lucasr) from comment #1)

> ::: mobile/android/base/Favicons.java

> >              ByteArrayInputStream byteStream = null;
> >              BitmapDrawable image = null;
> >  
> >              try {
> > +                urlConnection = faviconUrl.openConnection();
> 
> You're generalizing this code to handle different types of connections such
> as what? Just checking.

JarURLConnection - which is what's returned for jar: URLs, like the ones about pages end up using.

> >              } catch (Exception e) {
> >                  Log.d(LOGTAG, "Error downloading favicon: " + e);
> >              } finally {
> > +                if (urlConnection != null && urlConnection instanceof HttpURLConnection) {
> 
> Is it enough to only cover HttpURLConnection here? What about, say,
> HttpsURLConnection?

We were casting the urlConnection to a HttpURLConnection previously. HttpsURLConnection derives from HttpConnection, which has the disconnect method.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da040523f045

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/da040523f045

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Note: This is broken again because of bug 709250

Comment 6

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-02-24)
20120224031039
http://hg.mozilla.org/mozilla-central/rev/cd120efbe4c6


Still reproducible on:

Firefox 12.0a2 (2012-02-24)
20120224042008
http://hg.mozilla.org/releases/mozilla-aurora/rev/643e4dd65350

Firefox 11.0 (2012-02-24)
20120223235138
http://hg.mozilla.org/releases/mozilla-beta/rev/0e30f13e9012

--
Device: Motorola Droid 2
OS: Android 2.3.3

Comment 7

[Cristian Nicolae (:xti)]

I missed a tab. Sorry for the comment #6. I will check this again when bug 709250 will be fixed

Comment 8

[Cristian Nicolae (:xti)]

Since there is no favicon for any about: page due to bug 709250, I will reopen this bug.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Please don't reopen a bug that has been fixed. The new breakage is new. Bug 709250 covers the new breakage.

Comment 10

[Paul Feher]

I'm closing this bug based on the previously comment. The new breakage is covered by Bug 709250.