1225543 - Bookmark to about:blank shows favicon from random pages

Status: NEW

(Firefox :: Bookmarks & History, defect, P3)

Description

[Jonathan Allard]

Attachment: Screen Shot 2015-11-17 at 13.11.50.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20151117004023

Steps to reproduce:

1. Add bookmark to `about:blank` on the Bookmarks toolbar
2. Use Firefox for a while


Actual results:

The bookmark favicon will be of another site/page


Expected results:

The favicon should either be blank or generic

Comment 1

[Jonathan Allard]

I don't remember a previous version of Firefox that didn't have this bug, it's been happening as long as I've had such a bookmark. I'd be curious to know why it happens, as it is seemingly random.

Comment 2

[Alice0775 White]

I can also reproduce on Windows7

This seems to be e10s only bug.

Reproducible: 100%

STR:
1. Add bookmark to `about:blank` on the Bookmarks toolbar
2. Open web page ( e.g., https://developer.mozilla.org/en-US/Add-ons/Code_snippets )
3. Open about:config or about:buildconfig etc.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=473627b1b002&tochange=22d78dbf4b3a

Regressed by: 22d78dbf4b3a	Bill McCloskey — Bug 1056443 - Use correct URL in location bar when switching processes (r=ttaubert)

Comment 4

[Tracy Walker [:tracy]]

Kicking back into e10s triage.  Yes, this bug is e10s specific. But I believe this isn't a very common use case.  How many people actualy bookmark about:blank (it is not "Starable"). In addition, only power users would be accessing about:config. I could not get the favicon to change by visiting real sites.

Comment 6

[atlanto]

Having same problem here.

STR: in my case
1. Open about:config and bookmark it
2. Go to any web page with favicon
3. Go back

Comment 7

[atlanto]

Investigation code:
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -892,6 +892,8 @@ var SessionStoreInternal = {
         if ("image" in tabData) {
           // Use the serialized contentPrincipal with the new icon load.
           let loadingPrincipal = Utils.deserializePrincipal(tabData.iconLoadingPrincipal);
+          console.log("setIcon [" + tabData.image + "] for ["
+            + win.gBrowser.currentURI.spec + "] @SessionStore:restoreHistoryComplete");
           win.gBrowser.setIcon(tab, tabData.image, loadingPrincipal);
           TabStateCache.update(browser, { image: null, iconLoadingPrincipal: null });
         }

--- a/browser/components/sessionstore/TabState.jsm
+++ b/browser/components/sessionstore/TabState.jsm
@@ -18,6 +18,8 @@ XPCOMUtils.defineLazyModuleGetter(this, 
   "resource:///modules/sessionstore/TabAttributes.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Utils",
   "resource://gre/modules/sessionstore/Utils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "console",
+  "resource://gre/modules/Console.jsm");
 
 /**
  * Module that contains tab state collection methods.
@@ -119,10 +121,15 @@ var TabStateInternal = {
     // be read from the tab/browser every time we collect data.
 
     // Store the tab icon.
+    let cached = true;
     if (!("image" in tabData)) {
       let tabbrowser = tab.ownerGlobal.gBrowser;
       tabData.image = tabbrowser.getIcon(tab);
+      cached = false;
     }
+    console.log((cached ? "cached" : "getIcon") + " [" + tabData.image + "] for ["
+      + tabData.entries[tabData.index - 1].url + "] on ["
+      + browser.currentURI.spec + "] @_collectBaseTabData");
 
     // Store the serialized contentPrincipal of this tab to use for the icon.
     if (!("iconLoadingPrincipal" in tabData)) {

Investigation steps:
1. Start firefox with a new profile.
2. Open local file.
3. Go to web page.
4. Make sure icons in menu>History.
5. Go back.
6. Look icons in menu>History.
Actual result:
The local file have a favicon as same as the web page.
Expected result:
The local file have the globe icon instead of any favicon.

Since I'm not an expert, I could imagine at least two or more workarounds instantly.
A. Check url of tab and browser in TabState.jsm. (I don't think it is reasonable, though)
B. Remove >if (!("image" in tabData))< (I believe it will not degrade performance because getIcon is not heavy, but I'm not understanding the reason why if this if-statement should not be removed)
C. Check if the favicon should load before invoking setIcon in SessionStore.jsm. 8-|
D. Remove >win.gBrowser.setIcon(tab, tabData.image, loadingPrincipal);<  (correct favicon appears without this statement/mechanism)
and
E. Don't update the cache with the last image and the changed index @SessionStore.navigateAndRestore>restoreTab>TabStateCache.update. :-/

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 9

[Marco Bonardo [:mak]]

This is not super problematic for about pages, but we also have some reports about icons being misassigned, and we should ensure this is not a possible source for that issue.
It would be better to make this session restore path more robust, eventually ensuring it won't store favicons in the Places database (as ideally they should already be there).