Last Comment Bug 670341 - about:permissions queries hosts for favicons
: about:permissions queries hosts for favicons
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: :Margaret Leibovic
:
: Jared Wein [:jaws] (please needinfo? me)
Mentors:
Depends on:
Blocks: 657961 675945
  Show dependency treegraph
 
Reported: 2011-07-09 04:33 PDT by Stefan
Modified: 2011-09-30 08:49 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.32 KB, patch)
2011-08-02 12:11 PDT, :Margaret Leibovic
mak77: feedback+
Details | Diff | Splinter Review
patch v2 (2.16 KB, patch)
2011-08-02 13:39 PDT, :Margaret Leibovic
mak77: review+
Details | Diff | Splinter Review

Description Stefan 2011-07-09 04:33:03 PDT
User Agent:  
Build ID: 20110706164304

Steps to reproduce:

0. Use a profile which is use, not a fresh one.
1. Open new tab with web console (shift-ctrl-k)
2. about:permissions


Actual results:

Network access occurs. Fx tries to fetch some favicons


Expected results:

No network access occurs. Favicons shall be gathered only when a host is intentionally visited.
Comment 1 Stefan 2011-07-09 04:37:10 PDT
It is certainly not "nice" if favicons are grabbed when the Permission Manager is entered (Bug 658615).
Comment 2 Thomas Ahlblom 2011-07-09 06:12:09 PDT
Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a2) Gecko/20110709 Firefox/7.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110709 Firefox/8.0a1
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-09 18:47:04 PDT

*** This bug has been marked as a duplicate of bug 658615 ***
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-07-09 18:48:59 PDT
Actually, this is asking for the *opposite* of Bug 658615.
Comment 5 :Margaret Leibovic 2011-08-02 10:08:06 PDT
This seems to be a problem with the favicon service API. What we really want is an API method that just returns locally stored favicon data if we have it, and does not try to connect to the host otherwise. It looks like this method exists in nsIFaviconService, but not mozIAsyncFavicons, so perhaps we need to add a new async method to do this?

I'm cc'ing Marco, since he knows more about the favicon service.
Comment 6 Marco Bonardo [::mak] 2011-08-02 11:20:47 PDT
can you just use a moz-anno:favicon:pageurl src for the icon?
Comment 7 :Margaret Leibovic 2011-08-02 12:11:11 PDT
Created attachment 550154 [details] [diff] [review]
patch

(In reply to comment #6)
> can you just use a moz-anno:favicon:pageurl src for the icon?

I wasn't aware that this was a way to get what I wanted. With this patch I'm not seeing any more network activity, but I'm still seeing favicons, so it solves the problem!
Comment 8 Marco Bonardo [::mak] 2011-08-02 12:40:30 PDT
Comment on attachment 550154 [details] [diff] [review]
patch

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

per IRC discussion, this is fine, but adds 2 queries.  You should fire the https request before the http one, and ignore an eventual second callback if both exist.

::: browser/components/preferences/aboutPermissions.js
@@ +94,5 @@
>    getFavicon: function Site_getFavicon(aCallback) {
>      function faviconDataCallback(aURI, aDataLen, aData, aMimeType) {
>        try {
> +        // Avoid making a network connection to fetch the favicon
> +        aCallback("moz-anno:favicon:" + aURI.spec);

you can use gFaviconService.getFaviconLinkForIcon(aURI) to get automatically a moz-anno:favicon nsIURI (it just does the string concat practically)

@@ +102,5 @@
>      }
>  
>      // Try to find favicion for both URIs. Callback will only be called if a
>      // favicon URI is found, so this means we'll always prefer the https favicon.
>      gFaviconService.getFaviconURLForPage(this.httpURI, faviconDataCallback);

gFaviconService should also be QI to Ci.mozIAsyncFavicons (you can .getService().QueryInterface() to get both)
Comment 9 Marco Bonardo [::mak] 2011-08-02 13:01:50 PDT
nevermind QI, it's not needed, gavin pointed out we implement classinfo there, I honestly didn't recall :(
Comment 10 :Margaret Leibovic 2011-08-02 13:39:31 PDT
Created attachment 550186 [details] [diff] [review]
patch v2
Comment 11 Marco Bonardo [::mak] 2011-08-03 03:16:26 PDT
Comment on attachment 550186 [details] [diff] [review]
patch v2

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

::: browser/components/preferences/aboutPermissions.js
@@ +102,2 @@
>        try {
> +        // Get favicon data instead of fetching the favicon over the network.

nit: this comment looks a bit ambiguous, may get some love.
Comment 13 Tim Taubert [:ttaubert] 2011-08-04 05:22:47 PDT
http://hg.mozilla.org/mozilla-central/rev/7a907ffaa45b
Comment 14 Daniel Desira 2011-09-30 08:49:24 PDT
verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0

Note You need to log in before you can comment on or make changes to this bug.