1843267 - Favicons defined in disconnected link elements (in document fragments) will be loaded and used as the tab icon.

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[erosman]

Attachment: xhr-favicon.png

XMLHttpRequest from a page, or subsequent to a process initialed from a page e.g. sendMessage to background script to execute XMLHttpRequest, affects the tab's favicon display.

• There shouldn't be a need to request favicon on an XHR request
• Even if requested, there shouldn't be a need to apply it to the tab

Example 1

A local file opened in a tab e.g. file://..../index.html used to have a generic favicon
There is no favicon meta in that page.

Subsequent to a HTTP request from that file to yahoo.com and now the bookmark shows the yahoo favicon.

Example 2

As shown in Somehow userscript interferes with sites' favicon, XMLHttpRequest causes the favicon to change multiple times.

Stack Trace for XHR

load                          resource:///modules/FaviconLoader.sys.mjs:176:20
load                          resource:///modules/FaviconLoader.sys.mjs:563:70
loadIcons                     resource:///modules/FaviconLoader.sys.mjs:641:26
FaviconLoader/this.iconTask<  resource:///modules/FaviconLoader.sys.mjs:617:18
_runTask                      resource://gre/modules/DeferredTask.sys.mjs:347:18
_timerCallback/<              resource://gre/modules/DeferredTask.sys.mjs:318:20
_timerCallback                resource://gre/modules/DeferredTask.sys.mjs:337:9
_startTimer/callback/<        resource://gre/modules/DeferredTask.sys.mjs:185:18

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Firefox::Tabbed Browser' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Pascal Chevrel:pascalc]

It seems to be a regression from what was said on matrix.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Is there any chance you'd be able to hunt down a regression range with mozregression?

Comment 4

[erosman]

Sadly, mozregression GUI doesn't run on my system (Bug 1749215) and I am not good with CLI. Furthermore, I have a slow net connection which complicates downloading multiple Firefox versions for testing.

Comment 5

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

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

For more information, please visit BugBot documentation.

Comment 6

[Daniel Bodea [:danibodea]]

Can you create a detailed guide outlining the sequential steps required to replicate this particular issue?
With that, I will try to investigate for a regression. Thank you.

Comment 7

[erosman]

I can reproduce it as mentioned in Example 2, but I haven't been able to pinpoint the process (too much code involved) to produce a minimal test.
For example, the page gets https://s2.ltrbxd.com/static/img/icons/touch-icon-192x192.257b84e7.png which ends up in the tab favicon but I haven't found yet where that comes from.

I will post as soon as I work out a minimal process to reproduce the issue.

Comment 8

[erosman]

If anyone wants to try the Example 2: Somehow userscript interferes with sites' favicon

• Install FireMonkey
• Install (by going to): https://raw.githubusercontent.com/Purfview/IMDb-Scout-Mod/master/IMDb_Scout_Mod.user.js
• Go to: https://www.imdb.com/title/tt0088247/reference
• Observe favicon change and Network requests for the favicon e.g. https://s2.ltrbxd.com/static/img/icons/touch-icon-192x192.257b84e7.png

Above is not ideal as there are too many processes involved, but it might give someone a clue.

I have been trying but failed to reproduce so far with:

• XHR/fetch from a webpage
• XHR/fetch from the background script of a minimal addon
• XHR/fetch from the background script of a minimal addon subsequent to browser.menus.onClicked.addListener() from the context-menu action
• XHR/fetch from the background script of a minimal addon subsequent to browser.runtime.sendMessage() from the content script

I still need to make and test a message from userScripts to the background script of a minimal addon (which matches the Example 2 environment).

Comment 9

[Daniel Bodea [:danibodea]]

I can confirm this issue does reproduce in a very low repro rate (2/15 tries I've see a different icon than the expected one, once in release 115.0.3) even when the imdb scout mod does not work. With this repro rate, I cannot investigate for a regression.

I could not make this mod work for me. When I go into the FireMonkey addon's settings and attempt to manually run the IMDb Scout Mod, it gives out the notification error:
"
IMDb Scout Mod failed to insert
return not in function
"

Do you know why this mod does not work for me?
Which operating system are you using?
Which browser version has reproduced this issue for you?

Thank you for your contribution!

Comment 10

[erosman]

Do you know why this mod does not work for me?

Sorry, I forgot to mention that. As per FireMonkey help:

Quick Fixes

SyntaxError: return not in function

• 💡 Use Wrap code in IIFE
• Greasemonkey, Tampermonkey & Violentmonkey (as well as Node.js), wrap the userscripts in an anonymous IIFE (Immediately Invoked Function Expression) in order to limit its exposure to the page JavaScript. As a result, such JavaScript parse errors do not cause a problem, however that created a situation where developers write JavaScript with syntax errors.

Go to FireMonkey Options > select the script > click the 3-dot menu on top right > click "Wrap code in IIFE" > press Save

Comment 11

[Dão Gottwald [:dao]]

Not sure where this belongs, but FaviconLoader is Firefox::General.

Comment 12

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

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

For more information, please visit BugBot documentation.

Comment 13

[Sam Foster [:sfoster] (he/him) out until 2024-05-26]

I'll take a swing at priority and severity as it looks like this is confirmed, but only reproduces sometimes and in a very particular set of circumstances.

Comment 14

[Pascal Chevrel:pascalc]

FWIW, I think I can reproduce it on https://t2.social

Comment 15

[Daniel Bodea [:danibodea]]

With the information received in comment 13, I've managed to reproduce it more consistently.
Steps:

1. Install https://addons.mozilla.org/en-US/firefox/addon/firemonkey/
2. Install https://raw.githubusercontent.com/Purfview/IMDb-Scout-Mod/master/IMDb_Scout_Mod.user.js
3. Go to FireMonkey Options, select IMDb Scout Mod, click the "3 dots" button, Click the "Wrap code in IIFE" option and then click Save.
4. Load https://www.imdb.com/title/tt0088247/reference
5. Observe the icon displayed on the tab tile.
   Expected: The correct icon is displayed (IMDb, black on yellow)
   Actual: A different icon is being displayed.

I found that Nightly v90.0a1 can not reproduce this issue because the FireMonkey addon is not supported that far back.
Furthermore, I have found that Nightly v100.0a1 would cycle the icons on the page, but usually stop on the correct one (IMDb, black on yellow). The same behavior can be seen in Nightly v119.0a1.
Moreover, the repro rate appears to be quite the same as before: in 2-3/10 tries, when loading the page the icon would change a few times and stop on an incorrect one; while in 7-8/10 tries, the icon would change and stop on the correct one. I will assume that both these situations are reproductions of the reported bug.

In conclusion, the steps above can not be used to investigate any further than branch 100 and The website in comment 14 does not seem to reproduce for me.

Comment 16

[erosman]

I found that Nightly v90.0a1 can not reproduce this issue because the FireMonkey addon is not supported that far back.

Just for information:

Firefox Minimum Version

FireMonkey 2.68 -> Firefox 93 (2021-10-05)
FireMonkey 2.42 -> Firefox 74 (2020-03-10)
FireMonkey 1.25 -> Firefox 68 (2019-07-09)
FireMonkey 1.0 --> Firefox 65 (2019-01-29)

Comment 17

[Dave Townsend [:mossop]]

What's actually going on here is the userscript is loading other remote webpages via xmlhttprequest and then using jQuery to pull data out of them. A side effect of this is that jQuery will parse the webpage into a document fragment that belongs to the content document and when they contain link elements this triggers our listeners for added favicons. The only check we do on new link elements is to ensure they are owned by the document's global, which they are in the case of document fragments.

You can demonstrate this problem quite simply by running the following in the web console of any webpage:

document.createRange().createContextualFragment('<link href="https://searchfox.org/mozilla-central/static/icons/search.png" rel="icon">')

If the webpage has a CSP it will display an error about blocking the load of the resource, if it does not have a CSP (works fine on about:blank) then the tab icon will update.

The fix is quite straightforward, we just need to add a check for link.isConnected. Subsequently adding a disconnected link element to the document does re-trigger the DOMLinkAdded so the icon would get correctly picked up in that case.

I'm not sure when this regressed, it may well have been like this for over 10 years. Certainly long enough ago that it is worthless to pinpoint exactly when it happened.

Comment 18

[Dave Townsend [:mossop]]

This appears related to bug 1083895 but the fix there was specifically about changes to disconnected link elements where here we also need to target added link elements. Given how that was fixed it probably makes more sense (and is much easier to test) to not send the link events on disconnected link elements.

Comment 19

[Dave Townsend [:mossop]]

Attachment: Bug 1843267: Don't dispatch DOMLinkAdded and DOMLinkRemoved for elements disconnected from the DOM. r=emilio!

This change makes these events behave the same as DOMLinkChanged which was changed
in bug 1083895.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab74de20632b
Don't dispatch DOMLinkAdded and DOMLinkRemoved for elements disconnected from the DOM. r=emilio

Comment 21

[Cosmin Sabou [:CosminS]]

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

Comment 22

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

The patch landed in nightly and beta is affected.
:mossop, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Comment 23

[Dave Townsend [:mossop]]

This is a long-standing issue that should impact very few sites so I don't think it is worth uplifting.