1375223 - Remove Async.querySpinningly

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Edouard Oger [:eoger]]

History operations can be done asynchronously using History.jsm now.
We should use this API and kill Async.querySpinningly.

Comment 1

[Lina Butler [:lina]]

Tiago is working on moving the history queries into Places in bug 1336518. \o/

Comment 2

[Lina Butler [:lina]]

There are still some uses of `querySpinningly` in test code that we can replace with `PlacesUtils.promiseDBConnection`. For example, this code:

    let connection = PlacesUtils.history.QueryInterface(
      Ci.nsPIPlacesDatabase).DBConnection;

    let statement = connection.createAsyncStatement(
      "SELECT id FROM moz_places WHERE guid = :guid");

    stmt.params.guid = fxguid;
    let result = Async.querySpinningly(stmt, ["id"]);
    do_check_eq(result[0].id, fxid);

Becomes:

    let db = await PlacesUtils.promiseDBConnection();

    let result = await db.execute(
      "SELECT id FROM moz_places WHERE guid = :guid",
      { guid: fxguid });

    do_check_eq(result[0].getResultByName("id"), fxid);

https://searchfox.org/mozilla-central/source/services/common/tests/unit/test_async_querySpinningly.js can be removed entirely.

Comment 3

[Luciano I]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #2)
> There are still some uses of `querySpinningly` in test code that we can
> replace with `PlacesUtils.promiseDBConnection`. For example, this code:
> 
>     let connection = PlacesUtils.history.QueryInterface(
>       Ci.nsPIPlacesDatabase).DBConnection;
> 
>     let statement = connection.createAsyncStatement(
>       "SELECT id FROM moz_places WHERE guid = :guid");
> 
>     stmt.params.guid = fxguid;
>     let result = Async.querySpinningly(stmt, ["id"]);
>     do_check_eq(result[0].id, fxid);
> 
> Becomes:
> 
>     let db = await PlacesUtils.promiseDBConnection();
> 
>     let result = await db.execute(
>       "SELECT id FROM moz_places WHERE guid = :guid",
>       { guid: fxguid });
> 
>     do_check_eq(result[0].getResultByName("id"), fxid);
> 
> https://searchfox.org/mozilla-central/source/services/common/tests/unit/
> test_async_querySpinningly.js can be removed entirely.

I started work in this bug and i encountered one thing to point out and one problem.

In the first case (thing to point out):
http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_places_guid_downgrade.js#120
the function is never execute, so i don't understand why we need to maintain or something is wrong in another place.

And second: (problem)
http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_telemetry.js#571-572
if i remove lines 571 and 572 and replace them for:


     await PlacesUtils.promiseDBConnection();
     await db.execute("select bar from foo");


here is the output:
0:33.10 TEST_STATUS: Thread-1 test_sql_error FAIL [test_sql_error : 588] {"name":"unexpectederror","error":"Error: Error(s) encountered during statement execution: no such table: foo"} deepEqual {"name":"sqlerror","code":1}/home/lucho/src/mozilla-central/obj-x86_64-pc-linux-gnu/_tests/xpcshell/services/sync/tests/unit/test_telemetry.js:test_sql_error:588


i have no idea how to deal with that error

Comment 4

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

Attachment: fix-telemetry-test-sql-errors.patch

(In reply to Luciano I from comment #3)
> In the first case (thing to point out):
> http://searchfox.org/mozilla-central/source/services/sync/tests/unit/
> test_places_guid_downgrade.js#120
> the function is never execute, so i don't understand why we need to maintain
> or something is wrong in another place.

Yes, thanks for bringing that up - it is a mistake. I filed bug 1387303 to fix that test. If you wait a day or so (possibly less than 24 hours) and rebase your patch, you should find that test fixed (meaning it will still need to change in your patch, but hopefully those changes will be much clearer and easier to implement)

> And second: (problem)
> http://searchfox.org/mozilla-central/source/services/sync/tests/unit/
> test_telemetry.js#571-572
> if i remove lines 571 and 572 and replace them for:
> 
> 
>      await PlacesUtils.promiseDBConnection();
>      await db.execute("select bar from foo");
> 
> 
> here is the output:
> 0:33.10 TEST_STATUS: Thread-1 test_sql_error FAIL [test_sql_error : 588]
> {"name":"unexpectederror","error":"Error: Error(s) encountered during
> statement execution: no such table: foo"} deepEqual
> {"name":"sqlerror","code":1}/home/lucho/src/mozilla-central/obj-x86_64-pc-
> linux-gnu/_tests/xpcshell/services/sync/tests/unit/test_telemetry.js:
> test_sql_error:588
> 
> 
> i have no idea how to deal with that error

This is quite subtle - by changing how we execute these queries we also change what errors are thrown, and I think this actually uncovers yet another bug.

So we need to do 2 things here:
* change telemetry.js to handle both types of SQL errors that can be thrown and report them both in the same way.
* ensure test_telemetry.js causes both kinds of errors to be thrown so we have test coverage for both.

I'm attaching a patch for this issue, which you should be able to roll up into your patch and have things work.

Thanks!

Comment 5

[Luciano I]

(In reply to Mark Hammond [:markh] from comment #4)

> So we need to do 2 things here:
> * change telemetry.js to handle both types of SQL errors that can be thrown
> and report them both in the same way.

i'm not sure if we need to change something
because if you replace  in http://searchfox.org/mozilla-central/source/services/sync/tests/unit/test_telemetry.js#579
{"name":"sqlerror","code":1}
for
{"name":"unexpectederror","error":"Error: Error(s) encountered during > statement execution: no such table: foo"}
you'll get all test passed because this will handle the error:
http://searchfox.org/mozilla-central/source/services/sync/modules/telemetry.js#717-720

and i think we can remove this code because is no longer need it:
http://searchfox.org/mozilla-central/source/services/sync/modules/telemetry.js#684-691

> * ensure test_telemetry.js causes both kinds of errors to be thrown so we
> have test coverage for both.

what i have no idea, is how to cause that sqlerror again.

Comment 6

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

(In reply to Luciano I from comment #5)
> i'm not sure if we need to change something
> because if you replace  in
> http://searchfox.org/mozilla-central/source/services/sync/tests/unit/
> test_telemetry.js#579
> {"name":"sqlerror","code":1}
> for
> {"name":"unexpectederror","error":"Error: Error(s) encountered during >
> statement execution: no such table: foo"}
> you'll get all test passed because this will handle the error:
> http://searchfox.org/mozilla-central/source/services/sync/modules/telemetry.
> js#717-720

Yes - but that would be wrong - the entire point of the test is to ensure that a SQL error reports a sqlerror in telemetry.

> and i think we can remove this code because is no longer need it:
> http://searchfox.org/mozilla-central/source/services/sync/modules/telemetry.
> js#684-691

What makes you say that?

> > * ensure test_telemetry.js causes both kinds of errors to be thrown so we
> > have test coverage for both.
> 
> what i have no idea, is how to cause that sqlerror again.

I believe the patch I uploaded does everything you need for the 2 issues you raised.

Comment 7

[Luciano I]

I will wait for the patch and give you an update later!
Thanks for the feedback!

Comment 8

[Luciano I]

Attachment: Bug 1375223 - Remove Async.querySpinningly.

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

Comment 9

[Lina Butler [:lina]]

Comment on attachment 8894121 [details]
Bug 1375223 - Remove Async.querySpinningly.

https://reviewboard.mozilla.org/r/165190/#review173604

Thanks so much for doing this, and I'm really sorry for the late review. r- for TPS breakage, but you're on the right track. Nice work!

::: services/common/tests/unit/test_async_querySpinningly.js:2
(Diff revision 1)
> -/* Any copyright is dedicated to the Public Domain.
> - * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*
> +if i remove this file i get:

If you remove this line from `xpcshell.ini`, you can delete the file: https://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/services/common/tests/unit/xpcshell.ini#46

::: services/sync/tests/unit/test_history_store.js:170
(Diff revision 1)
>    do_check_eq(queryres[0].time, TIMESTAMP3);
>  });
>  
>  add_task(async function test_invalid_records() {
>    _("Make sure we handle invalid URLs in places databases gracefully.");
> -  let connection = PlacesUtils.history
> +  

Nit: Please remove all new whitespace from the lines highlighted in red. I've configured my editor to trim whitespace automatically on save, because I always forget to do it manually. :-) You might find that helpful, too.

::: services/sync/tests/unit/test_history_store.js:180
(Diff revision 1)
> -  + "VALUES ('invalid-uri', hash('invalid-uri'), 'Invalid URI', '.', 1, " + TIMESTAMP3 + ")"
> +    + "VALUES ('invalid-uri', hash('invalid-uri'), 'Invalid URI', '.', 1, " + TIMESTAMP3 + ")"
> -  );
> +    );
> -  Async.querySpinningly(stmt);
> -  stmt.finalize();
> -  // Add the corresponding visit to retain database coherence.
> -  stmt = connection.createAsyncStatement(
> +  });
> +  
> +  await PlacesUtils.withConnectionWrapper("test_invalid_record", async function(db) {
> +    await db.execute(

Please remove the second `withConnectionWrapper` call here, move the second `db.execute` call into the function above, and wrap them in `db.executeTransaction` like this:

    await PlacesUtils.withConnectionWrapper("test_invalid_record", async function(db) {
      await db.executeTransaction(async function() {
        await db.execute(
          "INSERT INTO moz_places "
          ...
        );
        await db.execute(
          "INSERT INTO moz_historyvisits "
          ...
        );
      });

::: services/sync/tests/unit/test_telemetry.js:572
(Diff revision 1)
>    let server = serverForFoo(engine);
>    await SyncTestingInfrastructure(server);
> -  engine._sync = function() {
> +  engine._sync = async function() {
>      // Just grab a DB connection and issue a bogus SQL statement synchronously.
> -    let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection;
> -    Async.querySpinningly(db.createAsyncStatement("select bar from foo"));
> +    /*
> +    This not work but i have no clue how to handle.

Hmm. `Sqlite.jsm` rejects all its promises with vanilla `Error`s (for example, https://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/toolkit/modules/Sqlite.jsm#818-819), so we can't tell the difference between SQL error and other errors.

I think it's probably not worth reporting as a separate error. With `Async.querySpinningly` gone, all our queries now go through `PlacesSyncUtils`, and those methods have their own tests.

Please remove this test completely, and https://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/services/sync/modules/telemetry.js#701-703, too.

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:84
(Diff revision 1)
>     * corresponding to the visits in the history database for the
>     * given uri
>     */
> -  _getVisits: function HistStore__getVisits(uri) {
> -    this._visitStm.params.url = uri;
> -    return Async.querySpinningly(this._visitStm, ["date", "type"]);
> +  _getVisits: async function HistStore__getVisits(uri) {
> +    let db = await this._db();
> +    return await db.execute(

This isn't quite right. `querySpinningly` returns an array of objects with column names mapping to values (`[{ type: 1, date: 123456 }]`), but `execute` returns an array of `mozIStorageRow` objects that access results via `getResultByName`. This will break TPS.

Tiago added `PlacesSyncUtils.history.fetchVisitsForURL` in bug 1336518. I think you can remove `_getVisits` entirely, and use `PlacesSyncUtils.history.fetchVisitsForURL` everywhere.

Second, since this is an async function, you'll need to change https://searchfox.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/modules/history.jsm#28,145 from `HistoryEntry._getVisits(uri)` to `Async.promiseSpinningly(PlacesSyncUtils.history.fetchVisitsForURL(uri))`.

You'll also want to add `Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");` to the top of this file, so that `PlacesSyncUtils` is available.

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8894121 [details]
Bug 1375223 - Remove Async.querySpinningly.

https://reviewboard.mozilla.org/r/165190/#review173624

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:48
(Diff revision 1)
>    /**
>     * _db
>     *
>     * Returns the DBConnection object for the history service.
>     */
> -  get _db() {
> +  get _db() {    

Also, since we can use `fetchInfoForURL` now, let's remove the `_db` getter completely.

Comment 11

[Luciano I]

Attachment: Bug 1375223 - Remove Async.querySpinningly.

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

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8899062 [details]
Bug 1375223 - Remove Async.querySpinningly.

https://reviewboard.mozilla.org/r/170410/#review175946

Nice work, almost there! For the next patch, please roll your two commits into one. You can use the `histedit` extension to do that: https://www.mercurial-scm.org/wiki/HisteditExtension

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:20
(Diff revision 1)
>  Cu.import("resource://gre/modules/PlacesUtils.jsm");
> +Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
>  Cu.import("resource://tps/logger.jsm");
>  Cu.import("resource://services-common/async.js");
>  
>  var DumpHistory = function TPS_History__DumpHistory() {

Please change this to `async function DumpHistory`, and update https://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/services/sync/tps/extensions/tps/resource/tps.jsm#439 to `await DumpHistory()`.

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:29
(Diff revision 1)
>    root.containerOpen = true;
>    Logger.logInfo("\n\ndumping history\n", true);
>    for (var i = 0; i < root.childCount; i++) {
>      let node = root.getChild(i);
>      let uri = node.uri;
> -    let curvisits = HistoryEntry._getVisits(uri);
> +    let curvisits = PlacesSyncUtils.history.fetchVisitsForURL(uri);

As in `Find`, please add an `await` here, too.

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:98
(Diff revision 1)
>     * @param item An object representing one or more visits to a specific uri
>     * @param usSinceEpoch The number of microseconds from Epoch to
>     *        the time the current Crossweave run was started
>     * @return true if all the visits for the uri are found, otherwise false
>     */
>    Find(item, usSinceEpoch) {

Since we'll be using `await` in the function body, please change this to `async Find`, and change https://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/services/sync/tps/extensions/tps/resource/tps.jsm#425,429 to use `await HistoryEntry.Find(...)` and `!await HistoryEntry.Find(...)`.

::: services/sync/tps/extensions/tps/resource/modules/history.jsm:102
(Diff revision 1)
>     */
>    Find(item, usSinceEpoch) {
>      Logger.AssertTrue("visits" in item && "uri" in item,
>        "History entry in test file must have both 'visits' " +
>        "and 'uri' properties");
> -    let curvisits = this._getVisits(item.uri);
> +    let curvisits = PlacesSyncUtils.history.fetchVisitsForURL(uri);

`fetchVisitsForURL` is an async function, so please change this to `await PlacesSyncUtils.history.fetchVisitsForURL`.

Comment 13

[Lina Butler [:lina]]

Hi Luciano! Are you still planning to work on this? (Totally OK if not).

Comment 14

[Edouard Oger [:eoger]]

Attachment: Bug 1375223 - Remove Async.querySpinningly.

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

Comment 15

[Lina Butler [:lina]]

Comment on attachment 8924020 [details]
Bug 1375223 - Remove Async.querySpinningly.

https://reviewboard.mozilla.org/r/195234/#review200872

Sorry, I forgot to hit r+ yesterday. This LGTM, thanks!

Comment 16

[Lina Butler [:lina]]

(If you could keep Luciano's name on the commit, or credit them for the original patch in the commit message, that would be awesome).

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8924020 [details]
Bug 1375223 - Remove Async.querySpinningly.

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

Comment 18

[Edouard Oger [:eoger]]

Done, thanks!

Comment 19

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 39581873590b -d 8dd4ccaed43e: rebasing 431791:39581873590b "Bug 1375223 - Remove Async.querySpinningly. r=kitcambridge" (tip)
local [dest] changed services/common/tests/unit/test_async_querySpinningly.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 20

[Edouard Oger [:eoger]]

Comment on attachment 8924020 [details]
Bug 1375223 - Remove Async.querySpinningly.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0155160ae50e
Remove Async.querySpinningly. r=kitcambridge

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0155160ae50e