1472476 - The first session restore after bug 1453751 causes restored tabs to lose their icons until the pages are loaded

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Dave Townsend [:mossop]]

We store the icon URL in session store data and previous to bug 1453751 that was normally a remote http url. After bug 1453751 we no longer accept setting remote http urls on tabs so the first restore will log errors like "Attempt to set a remote URL https://bugzilla.mozilla.org/extensions/BMO/web/images/favicon.ico as a tab icon.".

Comment 1

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

Comment 2

[Dave Townsend [:mossop]]

What do you think here Marco? The only options I can think of:

1. Do nothing. It happens for one session restore when upgrading from pre bug 1453751 to post. That means one startup for most users.
2. Re-allow setting remote urls on tabs for a while, including the code that sets the content principal. I was planning on ripping out the iconloadingprincipal support from the platform but we'd have to delay that in this case.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Dave Townsend [:mossop] from comment #2)
> 1. Do nothing. It happens for one session restore when upgrading from pre
> bug 1453751 to post. That means one startup for most users.

To me it sounds like an easy call, I'd just do nothing. Upgrades are a critical path and users are (sadly but understandably) used by OS and other apps to restart things after an update, especially if they notice strange behaviors.
The only important downside I notice here is for pinned tabs, I suppose those will be unrecognizeable on that startup... But it will work again after the first refresh right? It sounds more like an annoyance than a critical problem.

Comment 4

[Dave Townsend [:mossop]]

(In reply to Marco Bonardo [::mak] from comment #3)
> (In reply to Dave Townsend [:mossop] from comment #2)
> > 1. Do nothing. It happens for one session restore when upgrading from pre
> > bug 1453751 to post. That means one startup for most users.
> 
> To me it sounds like an easy call, I'd just do nothing. Upgrades are a
> critical path and users are (sadly but understandably) used by OS and other
> apps to restart things after an update, especially if they notice strange
> behaviors.
> The only important downside I notice here is for pinned tabs, I suppose
> those will be unrecognizeable on that startup... But it will work again
> after the first refresh right? It sounds more like an annoyance than a
> critical problem.

If I recall right pinned tabs reload automatically so they should pick up the right favicon then, this problem is only for tabs that are in the dormant state until you select them and they reload.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #2)
> What do you think here Marco? The only options I can think of:
> 
> 1. Do nothing. It happens for one session restore when upgrading from pre
> bug 1453751 to post. That means one startup for most users.

It may mean many more startups for users with session restore enabled and lots of open tabs that they may not select in every session.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> It may mean many more startups for users with session restore enabled and lots of open tabs that they may not select in every session.

Indeed, that is my situation.  I basically lost favicons on most of my tabs and won't be getting them back until I actually touch those tabs... in the meantime, _finding_ the tabs I want to touch has gotten much harder.  :(

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Dave, do you have an update on this? Thanks

Comment 8

[Dave Townsend [:mossop]]

(In reply to Pascal Chevrel:pascalc from comment #7)
> Dave, do you have an update on this? Thanks

I'm currently not working on it. Someone needs to make a decision on whether we need to fix this or not. I'm not sure who that is.

Comment 9

[Dave Townsend [:mossop]]

Attachment: Bug 1472476: Allow restoring sessions from before favicons were only allowed to be local schemes. r=mak

Re-introduces support for setting remote icons provided a loading principal is
passed. Removes some now defunkt code from sessionstore.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 8999266 [details]
Bug 1472476: Allow restoring sessions from before favicons were only allowed to be local schemes. r=mak

Drew Willcoxon :adw has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a6ea725be3f
Allow restoring sessions from before favicons were only allowed to be local schemes. r=adw

Comment 12

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 5a6ea725be3f (Bug 1472476) for ESlint failure on /sessionstore/TabState.jsm:15:1

Backout: https://hg.mozilla.org/integration/autoland/rev/e9b2abafd25e0cfe41cff40c763d2f0b12848d3f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5a6ea725be3fd22b2036380d801f076efbbc2e8f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=194874570

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=194874570&repo=autoland&lineNumber=265

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd65bc99f083
Allow restoring sessions from before favicons were only allowed to be local schemes. r=adw

Comment 14

[Dorel Luca [:dluca]]

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