Closed Bug 1351084 Opened 3 years ago Closed 3 years ago

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

Categories

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

52 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: 684sigma, Assigned: timhuang)

References

Details

(Keywords: regression, Whiteboard: [domsecurity-active])

Attachments

(1 file)

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.
Has STR: --- → yes
Keywords: regression
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3e153a5acd3597633d67532c4bca294e90ea75aa&tochange=fd0801b220f3b57363b5c7ed2946295433dd125d

Regressed by Bug 1277803.
Blocks: 1277803
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Component: Untriaged → DOM: Security
Ever confirmed: true
Flags: needinfo?(tihuang)
Product: Firefox → Core
Version: 53 Branch → 52 Branch
Thanks for reporting this issue, I will look into this.
Assignee: nobody → tihuang
Flags: needinfo?(tihuang)
Priority: -- → P2
Whiteboard: [domsecurity-active]
Status: NEW → ASSIGNED
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
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?
(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 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.
Attachment #8856394 - Flags: review?(mdeboer) → review+
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce2cbd3fc918
Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal. r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce2cbd3fc918
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
Flags: needinfo?(tihuang)
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?
Flags: needinfo?(tihuang) → needinfo?(mdeboer)
Could you please request beta approval for this patch?  Thanks!
Flags: needinfo?(tihuang)
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
Flags: needinfo?(tihuang)
Attachment #8856394 - Flags: approval-mozilla-beta?
Hi,
Can you help check if this issue is fixed as expected in nightly?
Flags: needinfo?(684sigma)
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
Flags: needinfo?(684sigma)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Comment on attachment 8856394 [details]
Bug 1351084 - Making the TabState.jsm collecting 'iconLoadingPrincipal' from browser.mIconLoadingPrincipal.

session restore fix, verified in nightly, beta54+
Attachment #8856394 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'm still seeing a similar issue on Fx 54b6: https://www.screencast.com/t/Xevnqpzxpa
Thoughts?
Flags: needinfo?(tihuang)
(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.
Flags: needinfo?(tihuang)
(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
Flags: needinfo?(mdeboer)
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 :)
Verified fixed Fx 54b8, Win 10.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.