Open Bug 1450384 Opened 7 years ago Updated 1 year ago

tabs.onUpdated doesn't always report favIconUrl (when restoring discarded tabs)

Categories

(WebExtensions :: Frontend, defect, P5)

60 Branch
defect

Tracking

(Not tracked)

UNCONFIRMED

People

(Reporter: n.gollenstede, Unassigned)

References

(Blocks 1 open bug)

Details

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20180322152034 Steps to reproduce: Wen loading a tab that lost its favicon as described in bug #1450382, the favicon will be restored, but the corresponding tab change is not always explicitly reported in `browser.tabs.onUpdated`. Later `tabs.Tab`s have the correct `favIconUrl` set, but sometimes there is no update with the explicit change `{ favIconUrl: '...', }` (but a change `{ favIconUrl: null, }` was fired before. Actual results: No `{ favIconUrl: '...', }` update. The `favIconUrl` has to be inferred from the `tabs.Tab` (3rd argument) of other `.onUpdated` calls for the tab (e.g. with `status` changing). Expected results: Every change of the favicon should fire an explicit update.
Component: Untriaged → WebExtensions: Frontend
OS: Unspecified → All
Product: Firefox → Toolkit
Hardware: Unspecified → All
Depends on: 1450382
Priority: -- → P3
Product: Toolkit → WebExtensions

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]

Any news on this?

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

Priority: P3 → P5

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.

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").

Severity: normal → S3

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.

Flags: needinfo?(rob)

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.

Flags: needinfo?(rob)
You need to log in before you can comment on or make changes to this bug.