1351084 - Favicon of about:addons disappears if cache was deleted recently.

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P2)

Description

[684sigma]

I have a problem with Firefox Beta 53. It also happens in Nightly 55. It doesn't happen in Firefox ESR 45.
Sometimes when I start browser, favicon in background about:addons tab is not displayed. If I switch to that tab or even reload it, there's still no favicon, just empty space.
It happens unpredictably, however, I noticed one specific scenario when it happens

1. Open homepage in a new tab. Open about:addons in a new tab. Open http://example.com in a new tab.
2. Open preferences in a new tab, choose When Firefox starts -> Show my windows and tabs from last time.
3. Open Advanced -> Network, clear cache until it says "using 0 bytes". Close preferences.
4. Restart browser. Restart browser.

Result: Favicon in about:addons tab disappears. All other favicons are restored correctly.
Expected: Favicon in about:addons tab should be visible.

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e153a5acd3597633d67532c4bca294e90ea75aa&tochange=fd0801b220f3b57363b5c7ed2946295433dd125d

Regressed by Bug 1277803.

Comment 2

[Tim Huang[:timhuang]]

Thanks for reporting this issue, I will look into this.

Comment 3

[Tim Huang[:timhuang]]

The root cause of this problem is that It will collect a null principal if the content is not loaded when SessionStore tries to collect tab data [1]. And the favicon of about:addons requires the system principal to load. So, the loading will fail.

In order to fix this, the TabState should collect 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal instead of from browser.contentPrincipal. 


[1] http://searchfox.org/mozilla-central/source/browser/components/sessionstore/TabState.jsm#129

Comment 4

[Tim Huang[:timhuang]]

Attachment: Bug 1351084 - Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal.

The browser.contentPrincpal will report a null prinicpal instead of the actual
content principal if the tab is not loaded. So the SessionStore will collect a
wrong principal for the 'iconLoadingPrincipal', and it will use this wrong
principal to load favicon when session restoring.

To fix this problem, this patch makes the TabState.jsm to collect
'iconLoadingPrincipal' from browser.mIconLoadingPrincipal which will be the
correct principal for loading favicon.

Review commit: https://reviewboard.mozilla.org/r/128328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128328/

Comment 5

[Loic]

I read some reports of users complaining about FF52 not displaying favicons after a session restore (like in this bug for about:addons). Could it be the same root cause, Tim?

Comment 6

[Tim Huang[:timhuang]]

(In reply to Loic from comment #5)
> I read some reports of users complaining about FF52 not displaying favicons
> after a session restore (like in this bug for about:addons). Could it be the
> same root cause, Tim?

It could be. If restored tabs are not loaded, then session store will store a null principal for each non-loaded tab and use this null principal to load at next time browser tries to restore sessions. If the website blocks its favicon to be cached, then the favicon won't be loaded when using a null principal.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8856394 [details]
Bug 1351084 - Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal.

https://reviewboard.mozilla.org/r/128328/#review130900

Of course! Makes total sense. Thank you.

Comment 8

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=608cf9597241

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce2cbd3fc918
Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal. r=mikedeboer

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Given the user reports in the wild that are possibly related and the simplicity of the patch, this seems worth consideration for backport? Also, would it be possible to test this?

Comment 12

[Tim Huang[:timhuang]]

I believe that we can test this if we can create a unloaded restored tab. But I don't know how to do it. If it is doable, I will file a bug to add a test case for this. 

Mike, could you give me a hint about this?

Comment 13

[Nathan Froyd [:froydnj]]

Could you please request beta approval for this patch?  Thanks!

Comment 14

[Tim Huang[:timhuang]]

Comment on attachment 8856394 [details]
Bug 1351084 - Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal.

Approval Request Comment
[Feature/Bug causing the regression]: Bug1277803
[User impact if declined]: The users' favicons will not be loaded when restoring user's session.
[Is this code covered by automated tests?]: No 
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes,
  1. Open homepage in a new tab. Open about:addons in a new tab. Open http://example.com in a new tab.
  2. Open preferences in a new tab, choose When Firefox starts -> Show my windows and tabs from last time.
  3. Open Privacy & Security, clear cache until it says "using 0 bytes". Close preferences.
  4. Restart browser twice.

  Expected: Favicon in about:addons tab should be visible. 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not risky
[Why is the change risky/not risky?]: This patch only changes one line, and it only affects the behavior of session restore of favicons. The normal favicon loading will not be affected by this change.
[String changes made/needed]: Nope

Comment 15

[Gerry Chang [:gchang]]

Hi,
Can you help check if this issue is fixed as expected in nightly?

Comment 16

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 17

[Brindusa Tot, Desktop QA]

Reproduced the bug on an older Nightly 55.0a1 from 04.10.2017 on Windows 10 x64

Verified as fixed on latest Nightly 55.0a1, buildID 20170505030252 on Window 10x64, Mac OS X 10.10 and Ubuntu 16.04 x64

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8856394 [details]
Bug 1351084 - Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal.

session restore fix, verified in nightly, beta54+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b6f2961e253

Comment 20

[Paul Silaghi, QA [:pauly]]

I'm still seeing a similar issue on Fx 54b6: https://www.screencast.com/t/Xevnqpzxpa
Thoughts?

Comment 21

[Tim Huang[:timhuang]]

(In reply to Paul Silaghi, QA [:pauly] from comment #20)
> I'm still seeing a similar issue on Fx 54b6:
> https://www.screencast.com/t/Xevnqpzxpa
> Thoughts?

Do you need to clear the cache and restart firefox like what comment 0 says? It seems it is a fresh profile and the favicon of about:addons disappear at the first time you load it. It looks like a different problem to me.

Comment 22

[Paul Silaghi, QA [:pauly]]

(In reply to Tim Huang[:timhuang] from comment #21)
> Do you need to clear the cache and restart firefox like what comment 0 says?
> It seems it is a fresh profile and the favicon of about:addons disappear at
> the first time you load it. 
well, got stuck after step 1 in comment 0.

> It looks like a different problem to me.
filed bug 1363671

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This seems edge-case enough to me that we can wontfix for ESR52. Feel free to change the status back and nominate the patch for approval if you feel otherwise, though, Tim :)

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed Fx 54b8, Win 10.