1163888 - No favicon for "host" autocomplete entries

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

Best viewed in the screenshot at in attachment 8603949 [details]. In the entry labeled "Unified=on: enter", note how "Visit arstechnica.com" doesn't display the favicon for the site even though the entry below (an article on arstechnica from history) does.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0006-Bug-1163888-ensure-autocomplete-host-matches-display.patch

There are 2 things conspiring against us here:

* Entries from the "hosts" table aren't complete URLs, just host names. Thus, when we execute |NetUtil.newURI(untrimmedHost)| we are passing "arstechnica.com" which fails with NS_ERROR_MALFORMED_URI. I fixed this by passing the hostname to Services.uriFixup.getFixupURIInfo() with no flags. This gives us a valid URL (but I'm not sure the flags are correct - or even if that's the correct way to get the full URL)

* The code naively assumes a path of /favicon.ico is correct, which is often wrong. In the arstechnica case, the favicon is https://cdn.arstechnica.net/favicon.ico. Obtaining the correct favicon for the host requires calling PlacesUtils.promiseFaviconLinkUrl() - which returns a promise - but none of the _process*Row() functions are setup to handle promises. So I also changed _onResultRow() so the returned "match" objects can be promises and this._addMatch() is called when it resolves.

This patch works for me. It has no tests and I'm not particularly confident with the promise changes, but worth getting feedback!

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8604440 [details] [diff] [review]
0006-Bug-1163888-ensure-autocomplete-host-matches-display.patch

Review of attachment 8604440 [details] [diff] [review]:
-----------------------------------------------------------------

the part about building a complete host looks good and we could land it.

but the promise part looks a little bit hack-ish, and I fear it might be fragile to future changes. It might also end up adding matches in unexpected order, if I read it correctly.
The best solution would probably be to add a JOIN with moz_favicons in the queries, but that might slowdown the queries. Could be worth a try though.

For example, you can try this query in Sqlite Manager to check which hosts have a favicon:

SELECT host, f.url FROM moz_hosts
LEFT JOIN moz_places h ON h.url = IFNULL(prefix, "http://") || host || "/"
LEFT JOIN moz_favicons f ON h.favicon_id = f.id
WHERE f.url NOTNULL

From what I see on my database the guess we do with /favicon.ico is very bad.

Using a similar query, we could basically add a subquery in the host/url queries where iconurl is supposed to be fetched (const QUERYINDEX_ICONURL       = 3; so the 4th column in the query)

 (SELECT f.url FROM moz_favicons f
  JOIN moz_places h ON h.favicon_id = f.id
  WHERE h.url = IFNULL(prefix, "http://") || host || "/")

Then it would only be matter of teaching _processHostRow to fetch QUERYINDEX_ICONURL, like _processRow does.

changing urlQuery should be easier since there we already have h.url, a simple LEFT JOIN with moz_favicons should do.

The reason I didn't join originally was to save some resources, but since doesn't look like we have cheaper solutions, we should probably do it properly.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

as a side note, since that might look surprising, '||' is the append-string magic wand in SQLite.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8604440 [details] [diff] [review]
0006-Bug-1163888-ensure-autocomplete-host-matches-display.patch

(In reply to Marco Bonardo [::mak] from comment #2)
> but the promise part looks a little bit hack-ish, and I fear it might be
> fragile to future changes. It might also end up adding matches in unexpected
> order, if I read it correctly.

Well, lots of this world is already promise based - this code called from sqlite completion callbacks already, right?

> The best solution would probably be to add a JOIN with moz_favicons

Hmm - so even though we already have async functions (one promise-based, the other callback-based) that perfectly solves our problem, we can't use it here and instead must add many complex lines of sub-queries with SQL and related support code etc?

So yeah, count me out :)

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mark Hammond [:markh] from comment #4)
> Hmm - so even though we already have async functions (one promise-based, the
> other callback-based) that perfectly solves our problem, we can't use it
> here and instead must add many complex lines of sub-queries with SQL and
> related support code etc?

The problem is that we must retain certainty of the order results are added, that would require, the promise solution has an high risk of adding races where that might not be true anymore.
The only viable fix would be to make the onRow handler of Sqlite.jsm run tasks instead of simple callbacks, but then we'd have risk that some consumer yields infinitely blocking the connection.

The other fact is that promiseFaviconLinkUrl is much slower than directly querying, since it has to run a similar query, but with more overhead.
The 2 things are basically doing the same, but one is more performant.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Dave, what's the right way to nominate something for triage for the team backlog? Is this OK?

Comment 7

[Dave Townsend [:mossop]]

I think so but I'll defer to Shell

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'll take this bug as a side project, while it's not blocking unified complete, it's polish work for it, and the solution is basically already expressed in the previous comments. I just need to figure out a testing story.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7969c82a42df

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7969c82a42df