Closed Bug 869209 Opened 7 years ago Closed 6 years ago

order provider menus by frecency

Categories

(Firefox Graveyard :: SocialAPI, defect)

22 Branch
x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
This was f+'d by felipe in bug 818675, but we want review from someone with more knowledge of the db backend.
Assignee: nobody → mixedpuppy
Attachment #747116 - Flags: feedback?(mak77)
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() }
Attachment #747116 - Flags: feedback?(mak77)
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.
Attachment #747116 - Attachment is obsolete: true
Attachment #747682 - Flags: review?(mak77)
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()
Attachment #747682 - Flags: review?(mak77) → review+
as a side note, this is synchronous and does main-thread IO, you should evaluate making the process async in future.
(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?
(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?
Attached patch ordered providers (obsolete) — Splinter Review
feedback implemented

https://tbpl.mozilla.org/?tree=Try&rev=0d1d6719b827
Attachment #747682 - Attachment is obsolete: true
Attachment #748156 - Flags: review+
Can we delay landing this until we have an async solution? I don't see the need to expedite this fix.
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?
Flags: needinfo?(mak77)
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
Nono, it's fine. Not sure what i was thinking about, maybe got confused with the test.
Flags: needinfo?(mak77)
http://hg.mozilla.org/integration/mozilla-inbound/rev/390d49fe8290 - Ryan VanderMeulen - Backed out changeset 080912624060 (bug 869209) for Android xpcshell failures.
try with build change to prevent building on android

https://tbpl.mozilla.org/?tree=Try&rev=f4f36e5b9430
try with feedback from gavin on config changes

https://tbpl.mozilla.org/?tree=Try&rev=3c47c1881603
includes fix for xpcshell tests on android, by removing socialapi toolkit from android (none of it would work anyway)
Attachment #748156 - Attachment is obsolete: true
Attachment #748474 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39f9c42b2668
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
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.
Flags: sec-review?
(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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(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
Flags: sec-review? → sec-review?(amuntner)
I actually don't understand the need for security review request on this, jaws, can you explain the concern?
Flags: needinfo?(jaws)
I think we can drop the need for the secreview. I added it because the sql injection comment wasn't clear to me.
Flags: needinfo?(jaws)
Depends on: 872323
Flags: sec-review?(amuntner)
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.