Open Bug 1428751 Opened 7 years ago Updated 2 years ago

Reddit Enhancement Suite may set the reddit favicon on bookmark entries

Categories

(Toolkit :: Places, defect, P5)

Product:
Toolkit
Component:
Places
Version:
57 Branch
Type:
defect

Tracking

()

People

(Reporter: ossman, Unassigned)

References

Details

(Keywords: steps-wanted, Whiteboard: [fxsearch])

Attachments

(1 file)

screenshot
(deleted), image/png
Details 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171130114058

Steps to reproduce:

Not entirely sure as I've not attempted to fully reproduce this. I failed to reproduce it when omitting the upgrade step though.

1. Add a toolbar bookmark to Firefox 56
2. Upgrade to Firefox 57
3. Click the home button
4. Before the page has fully loaded, click the bookmark from 1.


Actual results:

The bookmark gets the icon from the home page.


Expected results:

The bookmark should get the icon from the page the bookmark points to.
This was broken off from bug 1420150. Some more details from that bug:

In my scenario https://www.reddit.com/ is the homepage and https://intranet.lkpg.cendio.se/ is the bookmark.

So I've had a look in the tables, and it confirms the broken behaviour that is seen:

> sqlite> select * from moz_pages_w_icons inner join moz_icons_to_pages on id = page_id where icon_id = 6140;
> 15541|https://www.reddit.com/|47359719085711|15541|6140
> 19050|https://intranet.lkpg.cendio.se/|47357317736854|19050|6140
> ... (a whole bunch of more reddit URLs)

Every other page for intranet.lkpg.cendio.se has icon id 1. I'm also getting the correct icon for the tab.

There doesn't seem to enough info here to speculate on why it went wrong. But why isn't it resolving itself? When is moz_pages_w_icons updated? It's been broken for months.

I do have one more clue though. There is actually a correct mapping entry as well:

> sqlite> select * from moz_pages_w_icons inner join moz_icons_to_pages on id = page_id where page_url = 'https://intranet.lkpg.cendio.se/';
> 19050|https://intranet.lkpg.cendio.se/|47357317736854|19050|1
> 19050|https://intranet.lkpg.cendio.se/|47357317736854|19050|6140
> 19050|https://intranet.lkpg.cendio.se/|47357317736854|19050|6167
There was a speculation about redirects, and it might be a nice clue. The home page is actually http://www.reddit.com/, which redirects to https://... However there is no redirect for the bookmark.
I've seen this reported various times on reddit, so it's worth investigating.
There may have been a bug in previous versions that assigned the wrong favicon to a page, and maybe we're just keeping on these broken links between pages and icons.
Status: UNCONFIRMED → NEW
Component: Untriaged → Places
Ever confirmed: true
Priority: -- → P2
Product: Firefox → Toolkit
Whiteboard: [fxsearch]
Here's the most recent thread on reddit discussing this issue: https://www.reddit.com/r/firefox/comments/7omw55/help_reddit_is_taking_over_my_favicons_and_i_cant/
Since, I think, FF 55 I've seen sometimes the wrong favicon being assigned to a history entry, but this was very sporadic.

The issue has become more prevalent immediately after upgrading to FF 58. For instance, the Gmail favicon (mail.google.com) has changed to the generic G of www.google.com, both in bookmarks and history. Deleting and re-adding the bookmark does not fix the issue. In another case, no favicon is assigned to a history entry, not even the generic "globe" icon.

Favicons in tab titles are always correct, and have always been, so the issue is specific to bookmarks and history.

The problem does not reproduce in a blank profile, nor does it reproduce if I delete the favicons.sqlite file. However, in both cases, I'm not sure that it may not resurface later (plus, I lose each and every favicon). A "PRAGMA integrity_check" on the file returns ok, so it is not an obvious case of db corruption.

(I am on 32bit Win7).
I have upgraded to FF 58 on another machine on which I synchronise bookmarks. Some of the issues mentioned in previous comment don't reproduce but some do. For instance, if a favicon is associated to a page in favicons.sqlite and its url is still accessible, FF 58 may use that icon in bookmarks and history instead of the favicon referenced by the page. This did not happen in previous versions.
STR for comment 6 above.

Create a blank profile in FF 58 and a page with this code:

<!DOCTYPE html>
<html lang="en">
	<head>
		<meta charset="utf-8">
		<title>Test case</title>
		<link rel="icon" href="https://www.bugzilla.org/img/bugzilla_icon.png" type="image/png">
		<!-- <link rel="shortcut icon" href="https://addons.mozilla.org/favicon.ico?v=1"/> -->
	</head>
	<body></body>
</html>

View the page in the blank profile. The tab title shows the favicon of www.bugzilla.org.
View history. The favicon of the page's history entry is that of www.bugzilla.org.

Now edit the page; comment and uncomment the link tags so as to change the favicon to that of addons.mozilla.org.

View the page again. View history again.

Actual result: the favicon of the page's history entry is still that of www.bugzilla.org.

Expected result: the favicon of the page's history entry should be updated to that of addons.mozilla.org.
(In reply to Francesco from comment #7)
> Now edit the page; comment and uncomment the link tags so as to change the
> favicon to that of addons.mozilla.org.

First, the order you define icons doesn't matter, so having a "later" one in code won't do a thing.
Second, icons are expired after a week (it's actually controlled by the cache expiration pref), so it's not that changing the page will immediately update the icon.

What you are reporting is not this bug, I'm not even sure it's a bug (maybe bug 418144?).

This bug is about a page presenting icon of a totally unrelated host when the page DID NOT define that relation through a <link>. Like the case of the reddit icon appearing for non-reddit hosts.
More information which might help. I use an add-on which makes the home page open when a new tab is created. It is this home page icon (Google) which overwrote some of my bookmarks (Facebook).
interesting, I wonder if others had a similar setup, like using reddit as homepage.
My home page has always been about:blank with no add-ons changing it in any way, and I still got this bug. Even after removing favicons.sqlite a couple of weeks ago one of my favicon-less bookmarks (the site doesn't have favicon.ico or relevant link-tags) got infected again with the reddit icon.
(In reply to Marco Bonardo [::mak] from comment #11)
> interesting, I wonder if others had a similar setup, like using reddit as
> homepage.

Yes, as I mentioned in comment 2, I have reddit as the home page. It also involves a redirect from http to https. I do not have any add-ons related to this though.
Summary: wrong favicon on bookmark entry → wrong favicon on bookmark entry (especially reddit)
Keywords: steps-wanted
While looking into this with Evilpie, he pointed out he's using the Reddit Enhancement Suite add-on, and that add-on manipulates history.

How many of you used or use that add-on?
I do, on both machines where I saw this bug.
I do too.
thanks, this is very useful
Summary: wrong favicon on bookmark entry (especially reddit) → Reddit Enhancement Suite may set the reddit favicon on bookmark entries
Olli, I'm not sure if you're still the right person to ask about Document Navigation, so eventually please forward.

I think the problem here is that the Reddit Enhancement Suite uses history.pushState to replace the page url, and by doing so we end up associating the reddit favicon to whatever page it tries to replace.
The CopyFavicon method in the docshell doesn't make any kind of checks.
I was thinking we may at least check that the host is the same, before copying the favicons across 2 urls in the Docshell. Or maybe we should copy only if the 2 urls differ just by the ref, that seems the original reasoning around Bug 420605.
I don't know the pushState expected behavior regarding icons, but the current one just looks wrong.
Flags: needinfo?(bugs)
pushState shouldn't change the host, but sure, documentURI changes, and one could sort of
hide some real page by using the right pushState.
But can't see how that all would affect cross-domain cases.
Flags: needinfo?(bugs)
I experienced this issue once again a couple days ago. In the bookmark sidebar one of the icons changed to the same icon as my home page icon, this occurred during a crash of Firefox. I'm using this add-on:

https://addons.mozilla.org/en-US/firefox/addon/new-tab-override/
(In reply to Marco Bonardo [::mak] from comment #3)
> There may have been a bug in previous versions that assigned the wrong
> favicon to a page, and maybe we're just keeping on these broken links
> between pages and icons.

presuming my bug 1444675 is the same, the root cause would seem to remain present up to at least FF57, as I've deleted and recreated an affected bookmark (while on FF57), and the issue has occurred again on the new instance of the bookmark some time between then and 2018-03-10 (when I noticed the favicon for the new instance of the bookmark was again replaced)
(In reply to Francesco from comment #5)
> Deleting and re-adding
> the bookmark does not fix the issue.
> 
> The problem does not reproduce ... if
> I delete the favicons.sqlite file.

I'm able to get the correct favicon again if I delete the bookmark, restart firefox, then re-add the bookmark. does that work for you? if not, try again with "Clear history when Firefox closes" checked (also click the "Settings" button next to this option and check all boxes in the dialog that comes up)
(In reply to freallatt from comment #24)
> if not, try again
> with "Clear history when Firefox closes" checked

That will lose all of thistory, that likely most users don't want.
the favicon for a bookmark i often visit first thing after opening the browser is getting replaced (twice so far) with the icon of my home page (the firefox icon used for about:blank)

there are no redirects involved for the bookmark or the home page

i'm not using any public addons other than:
adblock plus
decentraleyes
(In reply to Marco Bonardo [::mak] from comment #25)
> (In reply to freallatt from comment #24)
> > if not, try again
> > with "Clear history when Firefox closes" checked
> 
> That will lose all of thistory, that likely most users don't want.

obviously.

i was just providing information about how i was able to correct the issue ("I'm able to get the correct favicon again if I delete the bookmark, restart firefox, then re-add the bookmark"). i added the additional information about my configuration that may have affected the outcome. I don't think it's helpful that you labeled this information "unhelpful".
I looked at Firefox code, but I couldn't find any hints to misbehaviors, nor I could reproduce the bug yet.

Looking at RES code, this part is the most suspicious, this far:
https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/243ac1fd2d23766a2401444260c1945db9e6182e/lib/modules/orangered.js#L221
Especially if somehow (race condition?) it ends up being applied to the wrong page, there should be some check before replacing icons on the page, like checking it's a reddit page and not something else.
If anyone has contacts with the RES team, it may be nice to check. Maybe it's nothing, but I couldn't find other suspicious behaviors yet.
Attached image screenshot (deleted) — 
Although I'm not sure this may apply to this issue, I can see the reddit icon on the tab of irrelevant site when the reddit icon has badge and the site has no favicon(root favicon is displayed). But bookmark icon is not affected. I cannot reproduce bookmark icon hijacking anyway.

STR:
0. Install RES
1. Go to https://www.reddit.com/ and login (there must be badge on the icon)(back to old reddit if necessary) (screenshot top)
2. Go to http://www.itmedia.co.jp/ (screenshot middle)
3. Go to https://headlines.yahoo.co.jp/list/?m=cnn -> https://www.reddit.com/
4. Back to https://headlines.yahoo.co.jp/list/?m=cnn (screenshot bottom)

If RES option 'Reset Favicon On Leave' is 'on'(by default), RES seems perform adding link tag for the icon while beforeunload event handling. Firefox seems invoke storeIcon and setIcon for new page by receiving 'Link:SetIcon' message.
It looks like messaging lag allows scripting against new page after beforeunload event handling.
(sorry if I'm wrong)
I think you may be onto something, we indeed pass over the browser for the icon load, but by the time we serve the request the browser.currentURI may have changed. It's probably worth passing the original page uri along with other info, regardless.
The recent changes to favicons in bug 1453751 may have solved this on our side, anyway it's likely to be related to comment 29, a bug in the add-on.
Priority: P2 → P3
Depends on: 1488296
This is potentially fixed in Firefox 63b9, please let us know if you still hit this bug after Firefox 63 is released.
Priority: P3 → P5
Having the exact issue on Firefox 63 Stable. I have RES installed.
Same here. Just managed to hit this again on Firefox 63.0.1 on Linux.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: