1360158 - Bookmarked favicons blinking on next bookmarks visit after landing patches from bug #977177

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Open this URL - about:support
2. Bookmark it in "Bookmarks Toolbar"
3. Enable "Bookmarks Toolbar"
4. Press newly created bookmark a few times to see that it will blink nearly each time

BTW. - Favicon in opened tab doesn't blink



You can see it also when you open some amount of bookmarks by "Open All in Tabs" feature and stay in "Library" to observe this.



Tested with Mozilla Firefox Nightly 55.0a1 (2017-04-25) (64-bit) [Portable] with clean new fresh profile without any addons (extensions, plugins, themes, etc.)
Works fine with Mozilla Firefox 53 (64-bit) [Portable] with clean new fresh profile without any addons (extensions, plugins, themes, etc.)



"Speedy" Regression window (mozilla-central)
Good:
https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-12-03-02-52-mozilla-central/

Bad:
https://ftp.mozilla.org/pub/firefox/nightly/2017/04/2017-04-13-03-02-27-mozilla-central/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f40e24f40b4c4556944c762d4764eace261297f5&tochange=819a666afddc804b6099ee1b3cff3a0fdf35ec15

Probably caused by:
d73a295b3652	Marco Bonardo — Bug 977177 - Invalidate the page-icon image cache when necessary. r=adw
42df4b3da073	Marco Bonardo — Bug 977177 - Don't expire root domain icons with history, and don't associate pages to them. r=adw
5311e5ac3b4b	Marco Bonardo — Bug 977177 - Add favicons.sqlite to profile related lists. r=adw,jmaher
864e72c60156	Marco Bonardo — Bug 977177 - Split ico files into native frames. r=adw
62f3fc3cb351	Marco Bonardo — bug 977177 - Fallback to the root domain icon. r=adw
60002894a42b	Marco Bonardo — Bug 977177 - Expire old page to icon relations to avoid serving deprecated icons. r=adw
4a0770d810dc	Marco Bonardo — Bug 977177 - Add size ref fragment to icon protocols. r=adw
90d755bcbb92	Marco Bonardo — Bug 977177 - Update favicons API consumers. r=adw
942aa1533e08	Marco Bonardo — Bug 977177 - Move favicons to a separate store. r=adw

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 2

[Marco Bonardo [:mak]]

the problem is that to invalidate the icon and force the view to reload it, we must remove the image attribute and set it back, removing the attribute removes the image for a brief while.
I'm not sure whether there's a non-hacky way around that.

Maybe annoying on slow systems, but not critical for functionality though. I don't think it's worth tracking.

I'm calling it a P2, but I have no idea on how to workaround the problem without dumb hacks (I'd prefer to avoid things like adding a ref to the url and then removing it...)

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Marco Bonardo [::mak] from comment #2)
> Maybe annoying on slow systems, but not critical for functionality though.

Yes, it's only very annoying.
I can reproduce in on my Intel C2D E6550 2,33 @ ~4GHz
and also on AMD Ryzen 5 1600, which are hardly slow systems ;)


(In reply to Marco Bonardo [::mak] from comment #2)
> I don't think it's worth tracking.

I think it is, as it's kinda very visible "performance" change, especially for users with "Bookmark Toolbar" and Bookmarks/History sidebars enabled, same for more then 1 monitor/display users and on one screen with "Library".


(In reply to Marco Bonardo [::mak] from comment #2)
> the problem is that to invalidate the icon and force the view to reload it,
> we must remove the image attribute and set it back, removing the attribute
> removes the image for a brief while.
> I'm not sure whether there's a non-hacky way around that.
> [..]
> I'm calling it a P2, but I have no idea on how to workaround the problem
> without dumb hacks (I'd prefer to avoid things like adding a ref to the url
> and then removing it...)

Can't it be done with the same way, as it was done in Firefox 54 or we used there these nasty hacks which we don't want now?

Comment 4

[Marco Bonardo [:mak]]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #3)
> and also on AMD Ryzen 5 1600, which are hardly slow systems ;)

off-topic: nice choice, I'm on a 1700X.

> (In reply to Marco Bonardo [::mak] from comment #2)
> I think it is, as it's kinda very visible "performance" change, especially
> for users with "Bookmark Toolbar" and Bookmarks/History sidebars enabled,
> same for more then 1 monitor/display users and on one screen with "Library".

What this has to do with performance? changing the ref may even be worse, perf-wise.

The use-case of clicking the bookmark multiple times, just to notice flickering, is not common at all. What the user will perceive is, in the worst case, that he clicked on a bookmark, the page loaded and the bookmark entry was updated. Not completely unexpected, I'd say.
Open all in tabs is a more rarely used feature, and still, it will flicker once for the load.

> Can't it be done with the same way, as it was done in Firefox 54 or we used
> there these nasty hacks which we don't want now?

in 54 the icon url was changing every time, here the icon url is always the same, even if the image changes. So no, we can't do the same way.

Actually, the ref workaround would not even help in the treeview (Library) case. There we can only tell the treeview to invalidate the column.

I don't see any simple technical solutions, as well as I think an update on load is not totally unexpected by the user. This is mostly a polish matter, that will likely require deep changes into the treebody handling (likely not trivial and complex to resource).

Comment 5

[Marco Bonardo [:mak]]

and with that I mean the cost to track and resource this seems to overweight the user benefit.

Comment 6

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to Marco Bonardo [::mak] from comment #4)
> (In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see
> your comment/reply/question/etc.) from comment #3)
> > and also on AMD Ryzen 5 1600, which are hardly slow systems ;)
> 
> off-topic: nice choice, I'm on a 1700X.

off-topic: yours choice is also nice ;)


(In reply to Marco Bonardo [::mak] from comment #4)
> > (In reply to Marco Bonardo [::mak] from comment #2)
> > I think it is, as it's kinda very visible "performance" change, especially
> > for users with "Bookmark Toolbar" and Bookmarks/History sidebars enabled,
> > same for more then 1 monitor/display users and on one screen with "Library".
> 
> What this has to do with performance? changing the ref may even be worse,
> perf-wise.

Just only perceptible "performance",
that now we full "refresh" favicon, instead of simple overwrite.

If the fix of this bug will make Firefox slower, I'm kinda in favor of WONTFIX'ing this bug,
because we moved favicons to their own database to get better performance, not same like before or worse with nasty hacks.


(In reply to Marco Bonardo [::mak] from comment #4)
> The use-case of clicking the bookmark multiple times, just to notice
> flickering, is not common at all. What the user will perceive is, in the
> worst case, that he clicked on a bookmark, the page loaded and the bookmark
> entry was updated. Not completely unexpected, I'd say.
> Open all in tabs is a more rarely used feature, and still, it will flicker
> once for the load.
> 
> > Can't it be done with the same way, as it was done in Firefox 54 or we used
> > there these nasty hacks which we don't want now?
> 
> in 54 the icon url was changing every time, here the icon url is always the
> same, even if the image changes. So no, we can't do the same way.
> 
> Actually, the ref workaround would not even help in the treeview (Library)
> case. There we can only tell the treeview to invalidate the column.
> 
> I don't see any simple technical solutions, as well as I think an update on
> load is not totally unexpected by the user. This is mostly a polish matter,
> that will likely require deep changes into the treebody handling (likely not
> trivial and complex to resource).

Just my non programmer talk, as said above,
maybe just simple overwrite will do job, instead of full "refresh"
or maybe if the icon is the same and didn't changed, don't "refresh" it.

Comment 7

[Marco Bonardo [:mak]]

What we could try to do here, is verifying whether we are over-notifying about icon changes. If an icon didn't change, we should not notify a change and then there should not be a ui refresh. It's possible currently we notify too much but that was not visible before.
Changing blockings since this was sort-of expected, while we still want to fix it, it's not an indicator of a broken favicons store nor a cause of wrong favicons.

Comment 8

[Marco Bonardo [:mak]]

side note: we are over notifying here, we don't check if the associations we are about to write are the same as the existing ones, thus we overwrite and renotify. not trivial to fix but feasible probably.

Comment 9

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I'm seeing that Firefox doesn't update database of bookmarks favicons on each websites pages visits, just only for some period of time.
Oddly bookmark favicons are always refreshed on each websites pages visits in Library and Bookmark Toolbar.

Comment 10

[Neha Kochar [:neha]]

Bulk change for all regression bugs with status-firefox67 as 'fix-optional' to be marked 'affected' for status-firefox68.

Comment 11

[Patricia Lawless]

Is this still a P2?

Comment 12

[Marco Bonardo [:mak]]

moving to backlog

Comment 13

[Tim Spurway [:tspurway]]

[bulk change 69 status -> --- b/c to stop re-triaging old regressions every release]

Triage Owners: please do not set release status tracking flags in new releases unless this bug will actually be worked on

Comment 14

[danmattera1]

(In reply to Marco Bonardo [:mak] from comment #4)

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see
your comment/reply/question/etc.) from comment #3)

and also on AMD Ryzen 5 1600, which are hardly slow systems ;)

off-topic: nice choice, I'm on a 1700X.

(In reply to Marco Bonardo [::mak] from comment #2)
I think it is, as it's kinda very visible "performance" change, especially
for users with "Bookmark Toolbar" and Bookmarks/History sidebars enabled,
same for more then 1 monitor/display users and on one screen with "Library".

What this has to do with performance? changing the ref may even be worse,
perf-wise.

The use-case of clicking the bookmark multiple times, just to notice
flickering, is not common at all. What the user will perceive is, in the
worst case, that he clicked on a bookmark, the page loaded and the bookmark
entry was updated. Not completely unexpected, I'd say.
Open all in tabs is a more rarely used feature, and still, it will flicker
once for the load.

Can't it be done with the same way, as it was done in Firefox 54 or we used
there these nasty hacks which we don't want now?

in 54 the icon url was changing every time, here the icon url is always the
same, even if the image changes. So no, we can't do the same way.

Actually, the ref workaround would not even help in the treeview (Library)
case. There we can only tell the treeview to invalidate the column.

I don't see any simple technical solutions, as well as I think an update on
load is not totally unexpected by the user. This is mostly a polish matter,
that will likely require deep changes into the treebody handling (likely not
trivial and complex to resource).

"The use-case of clicking the bookmark multiple times, just to notice flickering, is not common at all."

I see this issue (where the favicons in the bookmarks bar flicker) even when just clicking on a bookmark one time. I don't need multiple clicks to experience it. It's getting increasingly more annoying ever since I started noticing it a while back.

Firefox 90
macOS 10.15.7

example:

https://i.imgur.com/ZaU2d82.mp4

Comment 16

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

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 17

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

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit BugBot documentation.

Comment 18

[Kenneth Totherow]

Attachment: same problem

Thanks from liegehohop1989.