Open Bug 1450384 Opened 2 years ago Updated 13 days 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.

You need to log in before you can comment on or make changes to this bug.