tabs.onUpdated doesn't always report favIconUrl (when restoring discarded tabs)
Categories
(WebExtensions :: Frontend, defect, P5)
Tracking
(Not tracked)
People
(Reporter: n.gollenstede, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
|
||
Updated•7 years ago
|
|
||
Updated•7 years ago
|
When I navigate from a tab that has a favicon to one that doesn’t have one, the changeInfo obtained by adding a listener on browser.tabs.onUpdated doesn’t have the field faviconUrl.
After some investigation, it seems that the favicon is not reported as changed when this message appears in the browser console:
[Exception... "Favicon at "http://example.com/favicon.ico" failed to load: Not Found." nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource:///modules/FaviconLoader.jsm :: onStopRequest :: line 186" data: no]
The issue may lie here, with how the error is managed:
https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/browser/modules/FaviconLoader.jsm#461
|
||
Comment 5•6 years ago
|
||
This is a low priority for us, and comment 1 is expected behavior. Because the workaround is to just get the icon off the tab object during any update call, I'm going to move this to P5.
I did look into this a bit further, it would be easy for a contribution to fix this up a bit. It's probably not a good-first-bug, but a good-second-or-third-bug for a contributor.
Basically, in the case that the favicon fails to load, the load promise is rejected, Link:SetFailedIcon is sent instead of Link:SetIcon. That results in clearPendingIcon[1] being called, which does NOT call tabAttrModified[2], thus we do not get a notification in webextensions.
I'm assuming in this case a generic tab icon is shown.
We could possibly move the set/clearPendingIcon calls into tabbrowser.js and add a call to tabAttrModified for those. Or we could listen directly for Link:SetFailedIcon (and perhaps Link:SetIcon) in ext-tabs.js to handle the update call for the favicon.
[1] https://searchfox.org/mozilla-central/rev/11712bd3ce7454923e5931fa92eaf9c01ef35a0a/browser/base/content/browser.js#3753
[2] https://searchfox.org/mozilla-central/rev/11712bd3ce7454923e5931fa92eaf9c01ef35a0a/browser/base/content/tabbrowser.js#839
|
|
||
Updated•6 years ago
|
I just tested it and setIconFromLink is not even called, it stops in FaviconLoader.jsm. I’m not sure what to do about it though because it looks like changing its behavior could have large consequences on the code that depends on it.
I tried to look at the code but I don’t really understand what’s going on. I didn’t found where the problems happen and am stuck right now.
It seems that now I’m notified on every onUpdated about favIconUrl, even when it didn’t change.
Also, when there is no favIconUrl, it says { favIconUrl: undefined }, but it may be more correct to say { favIconUrl: null }, even though tabs.Tab doesn’t seem to have any nullable field yet. The behavior isn’t defined in the docs (I think it should) and I don’t know how other browsers handle this case (when tab changes from having a favicon to having none).
Also I guess it may be linked/happening at the same place in the code than bug 1417721.
When a tab «lose» its favicon, Google Chrome have the same behavior as described in my first comment here, so maybe it should stay that way and be documented explicitly (i.e. saying somewhere in the docs that extension developers should check for favIconUrl in tab because changeInfo doesn’t report favIconUrl removal).
I opened an issue on mdn/content, to document the current behavior. This issue can be closed afterwards if Firefox keeps the current (surprising but consistent with Chrome) behavior.
|
||
Comment 10•4 years ago
|
||
The test coverage for favicons and tabs.onUpdated is very minimal; bug 1688388 has some tests but didn't land because of intermittent failures (there are many comments about this in the patch at https://phabricator.services.mozilla.com/D98471, search for "faviconurl").
|
|
||
Updated•3 years ago
|
|
||
Comment 11•3 years ago
|
||
The severity field for this bug is relatively low, S3. However, the bug has 141 votes.
:robwu, could you consider increasing the bug severity?
For more information, please visit auto_nag documentation.
|
||
Comment 12•3 years ago
|
||
The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.
|
||
Comment 13•1 month ago
|
||
Hum now when a tab loses its favicon, I get { favIconUrl: null }:
- I don’t know when it changed and it broke handling when a tab loses its favicon in my extension
- it doesn’t behave like Chrome anymore, and there’s no update here, so I wonder if that’s intended
- I’m not sure a field in the
changeInfois supposed to be null since it doesn’t match the type in https://www.npmjs.com/package/@types/firefox-webext-browser, if it’s intended then the author of the package need to be informed, and MDN updated as well
|
|
||
Comment 14•1 month ago
|
||
Could share a test case? What is the expected value, and what does Chrome do? Bonus points if you use mozregression to identify the change in Firefox that caused the change, if you believe it to be a recent change.
If undefined is preferred over null, appending ?? undefined at https://searchfox.org/firefox-main/rev/e7f79d7d80b01b4d4b4d961124ef960ba9ab30a7/browser/components/extensions/parent/ext-browser.js#807 would solve the issue.
|
|
||
Comment 15•1 month ago
|
||
Firefox 147.0.3: log when navigating from a new tab (which has a favicon) to example.com (which hasn’t one).
repro:
browser.tabs.onUpdated.addListener((tabId, changeInfo, tab) => {
console.log("changeInfo: ", JSON.stringify(changeInfo));
console.log("tab: ", JSON.stringify(tab));
console.log();
});
|
|
||
Comment 16•1 month ago
|
||
Log for Chromium 144.0.7559.132 with the same steps and code.
|
|
||
Comment 17•1 month ago
|
||
I personally don’t really care about the behavior as long as the changeInfo gives me, in some logical and documented and stable way, the info that the tab now has no favicon, and I can’t find another similar case in the WebExtensions API that I know of, but it seems to me the current behavior of Firefox is practical and makes a lot of sense.
But in the past, I edited MDN to describe the weird behavior of older Firefox versions, and I’d like to be able to describe when the behavior changed (if I know it won’t change again).
Description
•