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
Updated•7 years ago
|
Updated•6 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•5 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•5 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•3 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•2 years ago
|
Comment 11•2 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•2 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.
Description
•