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

NEW
Unassigned

Status

()

defect
P3
major
2 years ago
22 days ago

People

(Reporter: Virtual, Unassigned)

Tracking

(Blocks 1 bug, 5 keywords)

55 Branch
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox-esr60 wontfix, firefox53 unaffected, firefox54 unaffected, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox67.0.5 wontfix, firefox68 wontfix)

Details

(Whiteboard: [fxsearch])

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
Flags: needinfo?(mak77)
[Tracking Requested - why for this release]: Regression
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...)
Flags: needinfo?(mak77)
Priority: -- → P2
Whiteboard: [fxsearch]
(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?
(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).
Keywords: polish
and with that I mean the cost to track and resource this seems to overweight the user benefit.
(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.
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.
Blocks: PlacesHiresFavicons
No longer blocks: 1356525
Has Regression Range: --- → yes
Has STR: --- → yes
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.
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.
No longer blocks: PlacesHiresFavicons
Blocks: PlacesJank
No longer blocks: PlacesIO

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

Comment 11

2 months ago

Is this still a P2?

Flags: needinfo?(mak77)

moving to backlog

Flags: needinfo?(mak77)
Priority: P2 → P3

[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

You need to log in before you can comment on or make changes to this bug.