869209 - order provider menus by frecency

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Currently providers are unordered in the menus.  We want them to appear by frecency, most frecent at top.  Moving the patch for that over from bug 818675.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: use frecency to order provider lists

This was f+'d by felipe in bug 818675, but we want review from someone with more knowledge of the db backend.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 747116 [details] [diff] [review]
use frecency to order provider lists

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

as a side note, in future you may evaluate to convert all social to use SQLite.jsm, opening your own db connection to Places.
That will give you cleaner syntax, more concurrency and some automatic management of memory.
Btw, for now it's fine as-is.

::: toolkit/components/social/SocialService.jsm
@@ +84,5 @@
> +    SocialServiceInternal.providerArray.forEach(function (p) {
> +      p.frecency = 0;
> +      rev_providers[p._rev_host] = p;
> +      rev_hosts.push(p._rev_host);
> +    }.bind(this));

you should consider using arrow functions :)

@@ +87,5 @@
> +      rev_hosts.push(p._rev_host);
> +    }.bind(this));
> +
> +    // XXX setting stmt.params.hostlist = [...] failed, we just create the
> +    // string ourselves

yes, you can't bind an array, though you can simply .join(",") an array of host strings...

@@ +98,5 @@
> +      "SELECT rev_host, MAX(frecency) as frecency " +
> +      "FROM moz_places " +
> +      "WHERE rev_host IN ("+hostlist+") " +
> +      "AND visit_count > 0 " +
> +      "GROUP BY rev_host "

you may select from moz_hosts, notice that in moz_hosts the hosts are not reversed and they are "normalized" in the sense "www." is removed (so you should search for facebook.com, not www.facebook.com)
That should be faster since the frecency and hosts are already grouped.
you can also ORDER BY frecency DESC in the query instead of doing that manually in handleCompletion.

So, in the end, the code may be much simpler.

@@ +123,5 @@
> +          return b.frecency - a.frecency;
> +        }));
> +      }
> +    });
> +    stmt.finalize();

the perfect thing would be to
let stmt =
try { executeAsync }
finally { stmt.finalize() }

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: use frecency to order provider lists

Adding an explicit call for getOrderedProviderList.  That should help indicate use that will result in db hits.

implemented feedback, but keeping post-sort since I need to sort all providers, some may not be in the query result.

added xpcshell test.

Comment 4

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=11f886611760

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 747682 [details] [diff] [review]
use frecency to order provider lists

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

I don't see anything bad :)

::: toolkit/components/social/SocialService.jsm
@@ +104,5 @@
> +
> +    // cannot bind an array to stmt.params so we have to build the string
> +    let stmt = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +                                 .DBConnection.createAsyncStatement(
> +      "SELECT host, frecency FROM moz_hosts WHERE host IN (\""+hosts.join('","')+"\") "

a comprehension should be a bit more readable, I was initially guessing about those apices inside the IN :)

[ '"' + host + '"' for each (hosts in hosts) ].join(",")

PS: I assume you verified these hosts are valid somewhere, if they can be installed from the Web a SQL injection attack may be possible here.

@@ +128,5 @@
> +          let providerList = SocialServiceInternal.providerArray;
> +          aCallback(providerList.sort(function(a, b) {
> +            // reverse sort
> +            return b.frecency - a.frecency;
> +          }));

providerList.sort((a, b) => b.frecency - a.frecency);

::: toolkit/components/social/test/xpcshell/test_SocialService.js
@@ +181,5 @@
> +  for (let p of providers) {
> +    n++;
> +    values[p.domain] =  n;
> +    DBConnection.executeSimpleSQL("INSERT INTO moz_hosts (id, host, frecency, typed, prefix) "+
> +                                  "VALUES (" + n + ", \"" + p.domain + "\", " + n +", 1, \"test.\")");

you could rather add visits through a history API, than adding data through a direct query, it's more future-proof (see promiseAddVisits)

@@ +187,5 @@
> +  providers = yield SocialService.getOrderedProviderList(next);
> +  for (let p of providers) {
> +    do_check_true(p.frecency != 0);
> +    do_check_eq(values[p.domain], p.frecency);
> +    DBConnection.executeSimpleSQL("DELETE FROM moz_hosts where host = \"" + p.domain + "\"");

and then this could just be a promiseClearHistory()

Comment 6

[Marco Bonardo [:mak]]

as a side note, this is synchronous and does main-thread IO, you should evaluate making the process async in future.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #6)
> as a side note, this is synchronous and does main-thread IO, you should
> evaluate making the process async in future.

It's really not acceptable to be adding new sources of main-thread-IO. Are there really no alternatives here?

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Marco Bonardo [:mak] from comment #6)
> as a side note, this is synchronous and does main-thread IO, you should
> evaluate making the process async in future.

I thought I was safe using async functions on the db.  What should I be doing?

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: ordered providers

feedback implemented

https://tbpl.mozilla.org/?tree=Try&rev=0d1d6719b827

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Can we delay landing this until we have an async solution? I don't see the need to expedite this fix.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

The patch is using executeAsync. Does that do main-thread IO? O.o

Only the test code does sync IO to simplify test setup.. maybe that was what Marco mentioned?

Comment 12

[Shane Caraveo (:mixedpuppy)]

Seems the last try is getting oranges that I think are fixed with the patch in bug 862314, another push including that patch to see if bc is fixed.

https://tbpl.mozilla.org/?tree=Try&rev=196756937de2

Comment 13

[Marco Bonardo [:mak]]

Nono, it's fine. Not sure what i was thinking about, maybe got confused with the test.

Comment 14

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/080912624060

Comment 15

[Shane Caraveo (:mixedpuppy)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/390d49fe8290 - Ryan VanderMeulen - Backed out changeset 080912624060 (bug 869209) for Android xpcshell failures.

Comment 16

[Shane Caraveo (:mixedpuppy)]

try with build change to prevent building on android

https://tbpl.mozilla.org/?tree=Try&rev=f4f36e5b9430

Comment 17

[Shane Caraveo (:mixedpuppy)]

try with feedback from gavin on config changes

https://tbpl.mozilla.org/?tree=Try&rev=3c47c1881603

Comment 18

[Shane Caraveo (:mixedpuppy)]

Attachment: ordered providers

includes fix for xpcshell tests on android, by removing socialapi toolkit from android (none of it would work anyway)

Comment 19

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39f9c42b2668

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39f9c42b2668

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

From Marco's review:

> +    // cannot bind an array to stmt.params so we have to build the string
> +    let stmt = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> +                                 .DBConnection.createAsyncStatement(
> +      "SELECT host, frecency FROM moz_hosts WHERE host IN (\""+hosts.join('","')+"\") "

a comprehension should be a bit more readable, I was initially guessing about those apices inside the IN :)

[ '"' + host + '"' for each (hosts in hosts) ].join(",")

PS: I assume you verified these hosts are valid somewhere, if they can be installed from the Web a SQL injection attack may be possible here.

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #21)
> From Marco's review:
> 
> > +    // cannot bind an array to stmt.params so we have to build the string
> > +    let stmt = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> > +                                 .DBConnection.createAsyncStatement(
> > +      "SELECT host, frecency FROM moz_hosts WHERE host IN (\""+hosts.join('","')+"\") "
> 
> a comprehension should be a bit more readable, I was initially guessing
> about those apices inside the IN :)
> 
> [ '"' + host + '"' for each (hosts in hosts) ].join(",")
> 
> PS: I assume you verified these hosts are valid somewhere, if they can be
> installed from the Web a SQL injection attack may be possible here.

The hosts are retrieved from the manifest, which is set within firefox code using nsIURI, or in the case of an install from AMO, would be set by code in AMO during addition to AMO.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> in the case of an install from AMO, would be set by code in
> AMO during addition to AMO.

Can you explain this more? Where does AMO get these values?

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #23)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> > in the case of an install from AMO, would be set by code in
> > AMO during addition to AMO.
> 
> Can you explain this more? Where does AMO get these values?

Not really, it's not implemented.  I imagine the sumbital process would define a manifest without origin, and either via script or manual review we would add origin, in the same way we do in firefox.

Comment 25

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] from comment #21)
> From Marco's review:
> 
> > +    // cannot bind an array to stmt.params so we have to build the string
> > +    let stmt = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase)
> > +                                 .DBConnection.createAsyncStatement(
> > +      "SELECT host, frecency FROM moz_hosts WHERE host IN (\""+hosts.join('","')+"\") "
> 
> a comprehension should be a bit more readable, I was initially guessing
> about those apices inside the IN :)
> 
> [ '"' + host + '"' for each (hosts in hosts) ].join(",")

btw, that was already changed

Comment 26

[Shane Caraveo (:mixedpuppy)]

I actually don't understand the need for security review request on this, jaws, can you explain the concern?

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

I think we can drop the need for the secreview. I added it because the sql injection comment wasn't clear to me.