some favicons are missing on about:debugging / tabs

ASSIGNED
Assigned to

Status

()

Firefox
Developer Tools: about:debugging
P2
normal
ASSIGNED
2 years ago
10 days ago

People

(Reporter: Sören Hentzschel, Assigned: jdescottes)

Tracking

(Blocks: 5 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8752335 [details]
about-debugging-tabs.png

Please have a look at the screenshot. Some favicons are missing (Google Docs, WordPress admin area).

Comment 1

2 years ago
Hi Sören, thank you for catching this so quickly! I believe this is because we use a hardcoded "/favicon.ico" file, instead of letting the website properly define its favicon via meta tags or a manifest file.

However, we definitely shouldn't show broken images like the ones in your screenshot, and instead show at least the default globe icon. Alex, could you please have a look at this?
Flags: needinfo?(poirot.alex)
Priority: -- → P1

Comment 2

2 years ago
I have some bandwidth to take the bug. 

I'd plan to use PlacesUtils.promiseFaviconLinkUrl to collect the correct favicon, and replace hardcode default icon path with `PlacesUtils.favicons.defaultFavicon.spec`

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1680

Is that the right approach?
Assignee: nobody → gasolin
Flags: needinfo?(janx)

Comment 3

2 years ago
Created attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review commit: https://reviewboard.mozilla.org/r/57538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57538/

Comment 4

2 years ago
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Icon shows fine in WIP. Current implementation will trigger tabs render several times (setState), need some suggestion if it matters.
Flags: needinfo?(janx)
Attachment #8759544 - Flags: feedback?(janx)
Flags: needinfo?(poirot.alex)

Comment 5

2 years ago
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Thanks a lot for addressing this gasolin!

Alex would be the best person to review such a change.

That said, I tried your WIP patch and it works for me! (Favicons that previously looked broken are now displayed successfully). My two biggest worries are:

- Your approach doesn't use our devtools actors, so it probably won't work correctly for remote debugging devices when we enable them (e.g. maybe Firefox won't be able to retrieve the favicon of a web app on Fennec).

- The fetched icons are low-resolution, even though we display them at a larger size in about:debugging. Maybe there is a way to ask Firefox for a high-resolution favicon for a given website?
Attachment #8759544 - Flags: feedback?(janx) → review?(poirot.alex)
Attachment #8759544 - Flags: review?(poirot.alex)
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

https://reviewboard.mozilla.org/r/57538/#review54720

::: devtools/client/aboutdebugging/components/tabs/panel.js:52
(Diff revision 1)
>        tabs.forEach(tab => {
>          // FIXME Also try to fetch low-res favicon. But we should use actor
>          // support for this to get the high-res one (bug 1061654).
>          let url = new URL(tab.url);
>          if (url.protocol.startsWith("http")) {
> -          let prePath = url.origin;
> +          let promise = PlacesUtils.promiseFaviconLinkUrl(url);

Thanks for looking into this.

As Jan said, it would ideally be on the actor side.

Like in TabActor's form:
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1075

Any attribute put on `response` object is going to be available here, on `tab` object.

Unfortunately form() is synchronous, which doesn't fit the PlaceUtils helper synchronousness.
But we already had to work around that limitation.
See `update()`, where you can introduce some async computation required in `form()`:
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1056

Also, may be you could use the favicon xpcom (mozIAsyncFavicons) directly to prevent loading placeutils.jsm.

It would be great to add a test against the icon over here:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_tabs.js?force=1#39
newTabTarget is an instance of this react component:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/addons/target.js

Comment 7

2 years ago
Mak do you know if there's an iconservice to get high-res icon?
Flags: needinfo?(mak77)
(In reply to Fred Lin [:gasolin] from comment #7)
> Mak do you know if there's an iconservice to get high-res icon?

I will start working on bug 492172 soon (Likely before the end of June) since that's also a request for Activity Stream.  For now we don't have hi-res icons.
Flags: needinfo?(mak77)

Comment 9

2 years ago
Gasolin, Alex suggested in comment 6 to change the devtools TabActor to return a favicon URL.

It is important to use an iconservice through a devtools actor, which lives inside a target device, because when we'll be able to connect about:debugging to Fennec phones (e.g. via a USB cable), the actor will be able to use the device's iconservice instead of the iconservice from desktop Firefox. (Maybe the desktop's iconservice will not know the icon for a tab that's open on the device.)

Regarding which icon service to use, I guess you can use either PlacesUtils.promiseFaviconLinkUrl in the TabActor like you did in your previous patch, or use AsyncFavicons[0] directly as Alex suggested.

[0] https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/places/favicon.js#19

Regarding high-resolution icons, from Marco's comment 8, it looks like we don't know how to get these yet, so we'll have to keep the low-resolution icons in about:debugging for now until bug 492172 is fixed.

Thanks again for taking on this bug! Please let us know if you have any questions.
Flags: needinfo?(gasolin)

Comment 10

2 years ago
Thanks for explanation, I'm just back from the vacation and I'd like learn more about the actor in this workweek. See you soon :)
Status: NEW → ASSIGNED
Flags: needinfo?(gasolin)

Comment 11

2 years ago
after tracing code, I plan to move icon retrieving logics to the `onListTabs` function in actor/root.js 

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/root.js#250

by add extra `then` clause in `onListTabs` function, we don't need change form() to promise in webbrowser.js 


The code will looks like

-   return reply;
- });
+   return promise.resolve(reply);
+ }).then(reply => { // find proper icon
+   // Filter out closed tabs (represented as `null`).
+   let tabs = reply.tabs.filter(tab => !!tab);
+   ...
+   reply.tabs = tabs;
+   return reply;
+ });

I may pipe another `then` clause to deal with the promise.all iconservice results

ex: 

+   return promise.resolve(reply);
+ }).then(reply => { // find proper icon
+   ...
+   return new promise.resolve([reply, promise.all(tabPromises)]);
+ }).then((...) => {
+   return reply;
+ });


Is that make sense to you?
Flags: needinfo?(janx)
Hi Gasolin, thanks for following up on this!

Moving the icon retrieving logic to `onListTabs` is an interesting idea, but I'm not sure we want to delay the tabs list response just to fetch all the icons: fetching them all might take some time, and maybe some callers of `onListTabs` don't actually need icons, so they shouldn't have to wait for them.

Alex, what do you think of Gasolin's idea in comment 11?
Flags: needinfo?(janx) → needinfo?(poirot.alex)
Yes. RootActor should not contain such logic. This actor is meant to be generic and focus on providing access to other actors.

Fetching and exposing icons should be part of the TabActor, from webbrowser.js.
Now, we have this limitation on form() which is supposed to be synchronous.
I mentioned the update() method, which I thought was called in every case, but it looks like it is only called when we fetch an already created actor.
  https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#369
But may be that's easier to convert form() to be async for tab actors? Would it require a big refactoring? Or if that require changing many callsites, may be we could let form() as-is and expose another method returning a promise and which would add additional data on the form like the icon?

Otherwise I don't have a strong opinion about fetching the icons when doing listTabs.
Here I mostly care about not polluting RootActor with tab things.

One way to mitigate all concerns could be to expose a getIcon request on TabActor and call it for every tab.
Another option could be to pass options to listTabs, in order to define how much data you would like to fetch, but that may require tweaking various callsites.
Flags: needinfo?(poirot.alex)

Comment 14

2 years ago
Hmm, I'll try to assign the icon service promise for tab.icon in webbrowser.js:form, then resolve the promise in client. Therefore it won't block the tabList retrieval process.

Comment 15

2 years ago
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57538/diff/1-2/
Attachment #8759544 - Attachment description: Bug 1272774 - show favicons on about:debugging / tabs via PlacesUtils.promiseFaviconLinkUrl; → Bug 1272774 - show favicons on about:debugging via favicons service;

Comment 16

2 years ago
In WIP I moved favicons related code to devtools/server/actors/webbrowser.js and do the lazy icon retrieving via response.iconPromise


Though in WIP I always encounter `favicons not found` error (same issue when I use PlaceUtils) in webbrowser.js 

Do I need any special settings?
Flags: needinfo?(janx)
Alex, is this the approach you were expecting from comment 6 and comment 13? Please have a look at the attached WIP patch.
Flags: needinfo?(janx) → needinfo?(poirot.alex)
https://reviewboard.mozilla.org/r/57538/#review58112

It looks like you have issues figuring your way into our codebase.
Here is a working solution:

From webbrowser.js, add a getIcon method on TabActor.prototype, which do the call to favicons.getFaviconURLForPage. One actor method *can* return a promise if the value can't be fetched immediately.
(but the resolved value can't contain promises, it should be only pure js objects)
  TabActor.prototype.getIcon = function () {
    return new Promise(resolve => {
      ...
      resolve(theIconUrl);
    });
  };
Then, you need to register the new getIcon method in TabActor.prototype.requestTypes.

From panel.js, you should be able to do that:
    let packet = {
      to: tab.actor,
      type: "getIcon"
    };
    return this.props.client.request(packet, response => {
      // response.icon should be the icon
      ...
    });

::: devtools/server/actors/webbrowser.js:1103
(Diff revision 2)
> +      if (url.protocol.startsWith("http")) {
> +        if (!(url instanceof Ci.nsIURI)) {
> +           url = NetUtil.newURI(url);
> +        }
> +
> +        response.iconPromise = new promise(resolve => {

Here you should be using the DOM Promises, with a capital P:
"new Promise"

promise, with a lowercase p is the defer version, used like this:
let defer = promise.defer();
someasyncstuff(function () {
  defer.resolve(resolvedValue);
});
return defer.promise;

::: devtools/server/actors/webbrowser.js:1112
(Diff revision 2)
> +              resolve(uri);
> +            } else {
> +              resolve(favicons.defaultFavicon.spec);
> +            }
> +          });
> +        });

Also, you are aware that this code runs as an actor, are you?
Actors are run in a totally different scope and values returned by the actor are stringified to be pipe through a TCP protocol. Only primitive data type can be returned: pure js objects, strings, numbers, booleans, ...
But no function, nor any non-pure-js-object like promises, DOM nodes, ...

So here, iconPromise won't work. the code in panel.js is not going to receive the promise.
Flags: needinfo?(poirot.alex)

Comment 19

2 years ago
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57538/diff/2-3/

Comment 20

2 years ago
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Thank you Alex, now I know how to request server data from the client side (via client.request).

The WIP can got icon successfully from devtool/server. Will add tests accordingly.
Attachment #8759544 - Flags: feedback?(poirot.alex)
https://reviewboard.mozilla.org/r/57538/#review58916

::: devtools/client/aboutdebugging/components/tabs/panel.js:51
(Diff revision 3)
> -        // FIXME Also try to fetch low-res favicon. But we should use actor
> -        // support for this to get the high-res one (bug 1061654).
> -        let url = new URL(tab.url);
> -        if (url.protocol.startsWith("http")) {
> -          let prePath = url.origin;
> -          let idx = url.pathname.lastIndexOf("/");
> +        let packet = {
> +          to: tab.actor,
> +          type: "getFavicon"
> +        };
> +
> +        this.props.client.request(packet, response => {

Note that request() also returns a promise which resolves the response object.

It may be better to update state only once all icons are fetched.

::: devtools/server/actors/webbrowser.js:1236
(Diff revision 3)
> +  // support for this to get the high-res one (bug 1061654).
> +  onGetFavicon() {
> +    let url = new URL(this.url);
> +    if (url.protocol.startsWith("http")) {
> +      if (!(url instanceof Ci.nsIURI)) {
> +         url = NetUtil.newURI(url);

No need to import NetUtil for this, you can do:
Services.io.newURI(url, null, null)

::: devtools/server/actors/webbrowser.js:1245
(Diff revision 3)
> +        favicons.getFaviconURLForPage(url, uri => {
> +          if (uri) {
> +            uri = favicons.getFaviconLinkForIcon(uri);
> +            resolve(uri);
> +          } else {
> +            resolve(favicons.defaultFavicon.spec);

it looks like you resolve a nsIURI object in the first resolve() and a string in the second.

::: devtools/server/actors/webbrowser.js:1255
(Diff revision 3)
> +          "from": this.actorID,
> +          "icon": iconNsIURI.spec
> +        };
> +      }).catch(err => {
> +        console.log(err);
> +      });

I'm not sure this catch() is any useful. The error should be prompted anyway.

Comment 22

2 years ago
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57538/diff/3-4/
Attachment #8759544 - Flags: feedback?(poirot.alex)

Comment 23

2 years ago
Addressed issues commented in comment 21, use Promise.all to update icons at once.

The WIP can show icon successfully, though if I switch tab from about:debugging to other web page(ex twitter) then switch back to about:debugging page, the following error occurred in console and the tabs shows `Nothing yet` message.

> error occurred while processing 'getFavicon: ReferenceError: favicons is not defined
> Stack: onGetFavicon/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1239:9
> onGetFavicon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1238:14
> onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1648:15
> ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> > resource://devtools/shared/transport/transport.js:743:7
> Line: 1239, column: 9

Not sure why got `favicons is not defined` while loading normal web page,
Is there a way to detour that?
Flags: needinfo?(poirot.alex)
https://reviewboard.mozilla.org/r/57538/#review59038

::: devtools/server/actors/webbrowser.js:32
(Diff revision 4)
>  loader.lazyRequireGetter(this, "ProcessActorList", "devtools/server/actors/process", true);
>  loader.lazyImporter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
>  loader.lazyImporter(this, "ExtensionContent", "resource://gre/modules/ExtensionContent.jsm");
> -
> +XPCOMUtils.defineLazyServiceGetter(this, "favicons",
> +                                   "@mozilla.org/browser/favicon-service;1",
> +                                   "mozIAsyncFavicons");

The issue you are hitting is that this XPCOM doesn't work in the child process. it works only in the parent process. That's unfortunate :/

This webbrowser.js file is evaluated in the process where the targeted document lives.
For about: pages like about:debugging, about:addons, ... it lives in the parent process and works fine.
But when you open a tab and switch to mozilla.org, the tab is going to live into the content process. The TabActor for this tab is also going to live in the content process.

You might be able to work around that with a patch from bug 1240907 (the second patch which merge BrowserTabActor and RemoteBrowserTabActor). This patch already computes some TabActor fields in the parent process. Look into BrowserTabActor.form(). But again, it forces to put the icon in the form. form() is synchronous, but you can compute async stuff for it in connect() and update() methods which are async.

Given how uterly complex this ends up, it may be more reasonable to keep this code running on the client side... (like your first patch) It would be ok to do so if you can confirm that the favicon service is able to fetch icons for websites we never visited on Firefox.

Sorry for the roadblocks you are hitting, I would never have thought fetching favicons would be that complex!

Comment 25

2 years ago
> if you can confirm that the favicon service is able to fetch icons for websites we never visited on Firefox

Not sure what does that mean?
about:debugging is about to work with remote devices like Firefox for Android.
The actor is going to run on the Android device, while about:debugging runs in your desktop Firefox.
So tabs are going to be open and visited on Android, its favicon service is going to know about these tabs. But about:debugging, on desktop, won't. I have not idea how the favicon service behaves when you task about URL you never visited.
Flags: needinfo?(poirot.alex)
Hum. Does this service works at all?

I get null `uri` when executing that in the browser console:
Favicons.getFaviconDataForPage(Services.io.newURI("https://google.com/", null, null), uri => console.log("uri",uri))

Comment 28

2 years ago
> Hum. Does this service works at all?

Yes, I can still get icon uris with issue described in comment 23 though.


Is there a proper way to do the feature detection on server side? 

> if(!favicons) { // which seems not work
>   return ...
> }

therefore we can ignore the related call in child process?
Since I moved my priority to track#2 tasks,
 I'd reset assignee myself from this issue to not block the progress.

feel free to pick up the WIP patch
Assignee: gasolin → nobody
Status: ASSIGNED → NEW
Benoit, you said you might be interested in picking up this bug.

If that's still the case, please have a look at comment 6 and comment 13, where Alex explains how a TabActor method could provide a high-resolution favicon for a tab (note: it's probably better to have a separate `getIcon()` async actor method, rather than making all `form()` calls wait for icons).

Please feel free to take over Gasolin's WIP patch, or you can also take a different approach if you prefer.
Flags: needinfo?(bchabod)
(In reply to Jan Keromnes [:janx] from comment #30)
> Benoit, you said you might be interested in picking up this bug.
> 
> If that's still the case, please have a look at comment 6 and comment 13,
> where Alex explains how a TabActor method could provide a high-resolution
> favicon for a tab (note: it's probably better to have a separate `getIcon()`
> async actor method, rather than making all `form()` calls wait for icons).
> 
> Please feel free to take over Gasolin's WIP patch, or you can also take a
> different approach if you prefer.

Yes, this bug seems interesting!
I've added it to my backlog, and I'll try to work on it as soon as I can.
I'll keep you guys updated
Flags: needinfo?(bchabod)
Blocks: 1308879
(Reporter)

Comment 32

4 months ago
Any chance this will get fixed? If it's too difficult to fetch the favicons showing no favicons at all would still be better than all the "broken image" images because, well, it looks broken… ;)
(Reporter)

Comment 33

3 months ago
ni? :bigben for comment 32 because one year ago you said:

> and I'll try to work on it as soon as I can. I'll keep you guys updated

;)
Flags: needinfo?(be.chabod)
Sorry Sören, I was planning on fixing this bug during my internship but I ran out of time.
I have other projects now and I won't be working on this or any Firefox-related development in the near future.

Feel free to pick it up or to suggest it to someone!
Flags: needinfo?(be.chabod)
If this is still considered a P1 bug, can we get it moving? It looks like a lot of work has already been done, it would be too bad wasting it.
Fred: what's left here? Can you describe the remaining steps so someone else can pick it up?
Flags: needinfo?(gasolin)
Duplicate of this bug: 1413177
Re-prioritizing as P2. Broken icons look bad but don't prevent using the feature.
Priority: P1 → P2

Comment 38

26 days ago
Based on comment 24, we could continue with the client-side approach in my origin patch
https://reviewboard.mozilla.org/r/57538/diff/1/

And need to make sure tabs icon shows correctly from Android(fennec) (As described in Comment 26)
Flags: needinfo?(gasolin)
Blocks: 1390643
Blocks: 1404663
Blocks: 1390642
Blocks: 1412596
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(In reply to Marco Bonardo [::mak] from comment #8)
> (In reply to Fred Lin [:gasolin] from comment #7)
> > Mak do you know if there's an iconservice to get high-res icon?
> 
> I will start working on bug 492172 soon (Likely before the end of June)
> since that's also a request for Activity Stream.  For now we don't have
> hi-res icons.

Mak: I see that Bug 492172 is closed but I'm not sure which API/service we should use for high resolution icons?
Flags: needinfo?(mak77)
How do you set icons currently?

Either use the idl API on mozIAsyncFavicons (getFaviconXXXForPage) or set the image source to
PlacesUtils.urlWithSizeRef(window, "page-icon:" + page_url, your_preferred_icon_size);
If the icon size you wish is 16px (the UI default), just use "page-icon:page_url".
The page-icon method is usually simpler if your page is chrome privileged.

Note the favicons service for now doesn't have favicons in PB mode.
Flags: needinfo?(mak77)
Thanks for the quick answer.

> How do you set icons currently?

At the moment we just fetch them by guessing the favicon.ico url. But when trying with PlacesUtils.promiseFaviconLinkUrl, I got small resolution icons for some websites, hence my question. 

Just tried using PlacesUtils.urlWithSizeRef, and it seems to work perfectly!
you can also pass a size param to the APIs and they will "try" to fulfill your request. In that case the size you pass must be good for the size you wish to show and the current device pixels (see the implementation of urlWithSizeRef eventually, it's trivial).
promiseFaviconLinkUrl has not been updated yet, I should check the consumer, maybe we could even remove it.
Comment hidden (mozreview-request)
Attachment #8934193 - Flags: review?(poirot.alex)
My import is a bit unclean, will fix that before requesting for review. 

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d31b41969523008b6bb61b786bca67b6fc68662
Comment hidden (mozreview-request)

Comment 46

10 days ago
mozreview-review
Comment on attachment 8934193 [details]
Bug 1272774 - use PlacesUtils.urlWithSizeRef to craft aboutdebugging tab icons;

https://reviewboard.mozilla.org/r/205136/#review211024

This patch fixes the broken icon, but also, it makes all icons be the global/default one when connecting to a remote.
Is that what you intent to do?
The favicon service only works for URLs you already visited, so unless you already visited the website your are debugging on the remote device, the icon will be the default one.

If you want to proceed with that behavior, you can wontfix bug 1061654.
Attachment #8934193 - Flags: review?(poirot.alex)
Good point. We could do this in two steps and first have about:debugging#tabs icons work for local debugging and have a follow-up to address the remote debugging case.

I'll give a shot to an actor-side solution first and see how complex it looks.
You need to log in before you can comment on or make changes to this bug.