2011412 - Don't decode synced tab favicons in the parent

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Tom Schuster]

I did bug 2006037 under the assumption that we are using raw favicon URLs for synced tabs. But when enabling that code in bug 2006015 we ran into crashes, because the page-icon: protocol was being used for the favicons already. So either something changed since I looked into this or I was just hallucinating. (I do believe I actually added the first test for favicon tabs, maybe also with a wrong understanding of the code) If we are actually consistently using the page-icon: protocol already then we can just wait for bug 2003246, which would make the page-icon: protocol safe by default. If the URL comes from the sync server, we might also add some kind of check that it's what we expect?

Mark, would you mind trying to untangle this for me?

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Ah, I forgot about https://searchfox.org/firefox-main/rev/131497bb1b747587b2b21b1abf14f44ecffad805/services/sync/modules/SyncedTabs.sys.mjs#83 - which is trying to use a local favicon when no icon is specified by the remote - and I think the mobile platforms don't supply one. There's also a pref which people can set to avoid using the remote favicon, but there's no UI for that pref.

So the change made to browser-sync.js probably should have been made there instead - sorry about that.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: (secure)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8e3849da0efc
https://hg.mozilla.org/integration/autoland/rev/b39734f474e3
avoid making a moz-remote-image: version of a page-icon: url. r=tschuster

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e62801966f0a
https://hg.mozilla.org/integration/autoland/rev/08a9de1f821f
Revert "Bug 2011412 - avoid making a moz-remote-image: version of a page-icon: url. r=tschuster" for causing failures at test_syncedtabs.js.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

• Push with failures
• Failure log

06:03:21 INFO - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_syncedtabs.js | xpcshell return code: 0
3050	06:03:21 ERROR - Unexpected exception TypeError: can't access property "devicePixelRatio", win is null at resource://services-sync/SyncedTabs.sys.mjs:201
3051	06:03:21 ERROR - getTabClients@resource://services-sync/SyncedTabs.sys.mjs:201:36
3052	06:03:21 ERROR - getTabClients@resource://services-sync/SyncedTabs.sys.mjs:392:27
3053	06:03:21 ERROR - test_noClients@/builds/worker/workspace/build/tests/xpcshell/tests/services/sync/tests/unit/test_syncedtabs.js:127:31
3054	06:03:21 ERROR - async*_run_next_test/<@/builds/worker/workspace/build/tests/xpcshell/head.js:1876:22
3055	06:03:21 ERROR - _run_next_test@/builds/worker/workspace/build/tests/xpcshell/head.js:1876:38
3056	06:03:21 ERROR - run@/builds/worker/workspace/build/tests/xpcshell/head.js:898:9
3057	06:03:21 ERROR - _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:245:6
3058	06:03:21 ERROR - _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:664:5

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/25c1f5db7c45
https://hg.mozilla.org/integration/autoland/rev/1b2c1fae01d0
avoid making a moz-remote-image: version of a page-icon: url. r=tschuster

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/33b6a700327a
https://hg.mozilla.org/integration/autoland/rev/cdc096b45b6b
Revert "Bug 2011412 - avoid making a moz-remote-image: version of a page-icon: url. r=tschuster" for causing xpcshell failures in test_remote_tabs.js.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

• mochitest failures

[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/favicons/browser_favicon_svg.js | Got green favicon - Both images are identical - 256 == 0
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - resource://testing-common/ImageTestUtils.sys.mjs:assertEqualImage:35
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochitests/content/browser/browser/base/content/test/favicons/browser_favicon_svg.js:testIconImage:43
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochitests/content/browser/browser/base/content/test/favicons/browser_favicon_svg.js:null:69
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - resource://testing-common/BrowserTestUtils.sys.mjs:withNewTab:130
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochitests/content/browser/browser/base/content/test/favicons/browser_favicon_svg.js:null:59
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochikit/content/browser-test.js:handleTask:1302
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1392
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1539
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1282
[task 2026-01-29T01:48:25.536+00:00] 01:48:25     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1063

	01:52:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_synced_tabs_menu.js | image protocol is correct - Got "http:", expected "moz-remote-image:"
5631	01:52:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_synced_tabs_menu.js | image url is correct - Got null, expected "http://example.com/favicon.ico"
6079	01:52:56 ERROR - TEST-EXPECTED-FAIL | browser/components/customizableui/test/browser_synced_tabs_menu.js | changed preference: identity.fxaccounts.remote.root

• xpcshell failures

[task 2026-01-29T01:51:15.603+00:00] 01:51:15     INFO - TEST-PASS | browser/components/urlbar/tests/unit/test_remote_tabs.js | test_maximal - [test_maximal : 1151] result.payload.device at result index 1 - "My Phone" deepEqual "My Phone"
[task 2026-01-29T01:51:15.604+00:00] 01:51:15     INFO - TEST-UNEXPECTED-FAIL | browser/components/urlbar/tests/unit/test_remote_tabs.js | test_maximal - [test_maximal : 1151] result.payload.icon at result index 1 - "cached-favicon:moz-remote-image://?url=http%3A%2F%2Ffavicon%2F&colorScheme=light&width=16&height=16" deepEqual "cached-favicon:http://favicon/"

Comment 9

[Mark Hammond [:markh] [:mhammond]]

The problem I have now is how to handle the URL bar. I chatted with Mak and erroneously told him that cached-favicon:moz-remote-image://?url=... works - because now I test some more I can't reproduce that.

He suggested we really want moz-remote-image:cached-favicon:<favicon_url>. I remain a little confused, because it seems like cached-favicon is always going to want to be wrapped in moz-remote-image, or the icon we get from places is going be decoded in the parent, but this would be the only place which does that. Regardless, I tried that and it doesn't work either.

The only thing that does work is the original cached-favicon:https://... version - but it's not clear those semantics are safe.

ie, I tested, when accounts.firefox.com is a synced tab, each of:

• cached-favicon:moz-remote-image://?url=https%3A%2F%2Faccounts.firefox.com%2Ffavicon.ico&colorScheme=light&width=16&height=16 - what I said worked, but I can't make it work now.
• moz-remote-image://?url=cached-favicon%3Ahttps%3A%2F%2Faccounts.firefox.com%2Ffavicon.ico&width=16&height=16&colorScheme=light - which Mak suggests is more correct and should work, but also doesn't work.
• cached-favicon:https://accounts.firefox.com/favicon.ico - the original, works.

Any advice on what I should do? The patch that is currently on phabricator unpacks the moz-remote-image to get the original URL - so ends up with the last option above which I mentioned works, but isn't clear is safe. If you think it's fine to land it like that and consider this issue in a followup, then would you mind doing another quick review? Otherwise, any advice appreciated.

Comment 10

[Marco Bonardo [:mak]]

Tom is surely better informed about moz-remote-image protocol.
cached-favicon and page-icon protocols are basically just taking the input uri, asynchronously fetching a blob for that uri from the favicons.sqlite database, then serving that blob in their channel.
As such I suggested to Mark that moz-remote-image should be the most external proto, so that the received blob is rendered in the sandbox.
I wonder if the problem is the url encoding or moz-remote-image not handling the multi protocol case correctly. It should be possible to debug in RemoteImageProtocolHandler.cpp and check why the fetch fails, is the destination url wrong, or do we fail somewhere in nsCachedFaviconProtocolHandler.cpp?
As discussed elsewhere, we could also make cached-favicon and page-icon do the sandbox rendering exactly like moz-remote-image, by sharing the code. I mostly wonder if that may have an effect on views showing thousands of icons (e.g. history tree)

Comment 11

[Tom Schuster]

Mak is correct here.

The right order would in theory be moz-remote-image://?url=cached-favicon%..., because we would like the safely render everything. I suspect we fail because we fetch the URLs from the child process, which is probably not support at all by cached-favicon: just like with page-icon:.

I talked about this before and I also think we should make cached-favicon and page-icon safe by default (aka sandboxed). At least for page-icon we should only need to do this for SVGs, because we make raster images save before storing them.

Does that mean for sync all URLs are using cached-favicon: at the moment? If not it might still be an improvement to only use the moz-remote-image: protocol for http/https/data etc. and leave cached-favicon: to some general architectural improvement.

Comment 12

[Marco Bonardo [:mak]]

(In reply to Tom Schuster from comment #11)

I talked about this before and I also think we should make cached-favicon and page-icon safe by default (aka sandboxed). At least for page-icon we should only need to do this for SVGs, because we make raster images save before storing them.

Yes, cached-favicon like page-icon is chrome-only, atm.
A couple thoughts.
First, Places historically didn'tt like to be opened from other than the parent process. Now, from a few months we're using a separate connection to read favicons and visited links (ConcurrentConnection.cpp), but that would still need some work to coordinate with the parent process, as we'll have 2 processes fighting for the db file handles. Ideally it just works, but who knows the unknowns.
Second, we'll at that point accept input from the users, as they can forge URLs. There should be no risk of dataloss as we use a read-only connection, we must be sure to not expose risk of data leak (we bind strings so sql injection is unlikely). We must put limits around these URLs, we don't want a site to be able to rebuild user history querying icons, or to build super large URLs that DOS us, for example.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Does that mean for sync all URLs are using cached-favicon: at the moment?

Nope - SyncedTabs.sys.mjs makes then all moz-remote-image, but when being shown in the URL bar, https://searchfox.org/firefox-main/rev/52e25e8bf7d712501f99b8ba77718ea0edc42bd7/browser/components/urlbar/UrlbarProviderRemoteTabs.sys.mjs#202-204 tries to convert them to cached-favicon.

If not it might still be an improvement to only use the moz-remote-image: protocol for http/https/data etc. and leave cached-favicon: to some general architectural improvement.

Right - so I think you are saying I should just remove that call to getFaviconLinkForIcon() and use the URL I get from SyncedTabs.sys.mjs directly?

Comment 14

[Tom Schuster]

That sounds right.

Comment 15

[Marco Bonardo [:mak]]

this means showing results in the urlbar will fetch from network, potentially disclosing history. Or was that the case before too?

Comment 16

[Mark Hammond [:markh] [:mhammond]]

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

this means showing results in the urlbar will fetch from network, potentially disclosing history. Or was that the case before too?

I don't think that was the case before. While we do fetch them when the accounts menu is opened, I guess it's still bad to fetch them as it's almost impossible to avoid interacting with the URL bar.

From the comments above, it also sounds like supporting moz-remote-image://?url=cached-favicon... is going to be a fair bit of work, so I guess plan C is to just avoid providing any icon for the URL bar and open a followup for this?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7a3abab9f02b
https://hg.mozilla.org/integration/autoland/rev/ebe15552c856
avoid making a moz-remote-image: version of a page-icon: url. r=tschuster,urlbar-reviewers,mak

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 19

[BugBot [:suhaib / :marco]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 20

[BugBot [:suhaib / :marco]]

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

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox149 to wontfix.

For more information, please visit BugBot documentation.

Comment 21

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: This fixes a possible security issue decoding remote icons
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Limited to synced tabs icons
• String changes made/needed: None
• Is Android affected?: no

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Attachment: (secure)

This also disables showing the specific favicon in the address bar, as it's not clear
exactly how to have the child process fetch the favicon. A generic favicon for the
URL will be shown instead.

Original Revision: https://phabricator.services.mozilla.com/D279808

Comment 23

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/d165cac4ad3e
https://hg.mozilla.org/releases/mozilla-beta/rev/0f05e7ecca76