FormHistory.jsm migration and DB creation should be off the main thread

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: Gavin, Assigned: mconley)

Tracking

(Blocks: 3 bugs, {main-thread-io})

Trunk
mozilla60
main-thread-io
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [qf:f60][qf:p1])

Attachments

(15 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
Grisha
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
59 bytes, text/x-review-board-request
mak
: review+
Details
4.18 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
nalexander
: review+
Details
See data in bug 878677 - essentially all of the work under dbInit() in FormHistory.jsm still occurs synchronously on the main thread. We should get rid of that, if possible.
Keywords: main-thread-io
Blocks: 887887
We should just use openAsyncDatabase and then fix any consumer to be async.
Migration and schema may be funny to handle, though Task.jsm may help handling errors.
The even better alternative may be to switch to Sqlite.jsm, though that may require slightly more refactoring.
Blocks: 950073
Whiteboard: p=0
No longer blocks: 950073
Flags: firefox-backlog+
Whiteboard: p=0 → p=13
Points: --- → 13
Whiteboard: p=13
(Assignee)

Updated

a year ago
Whiteboard: [qf:p1]
(Assignee)

Comment 2

a year ago
Any objections if I try to drive this to completion, mak?
Flags: needinfo?(mak77)
(Assignee)

Comment 3

a year ago
(Tentatively assigning self while I wait for mak to get back to me)
Assignee: nobody → mconley
Status: NEW → ASSIGNED
absolutely not, compared to comment 1, we should just use Sqlite.jsm that does the right thing.
Flags: needinfo?(mak77)
(Assignee)

Comment 5

a year ago
Okay, here's my approach plan that I'm starting with:

First, I'll add a second db connection construct to FormHistory.jsm which uses Sqlite.jsm, in parallel with the Services.storage stuff. The construct should return a Promise that resolves to a fully migrated database connection, and calling it should be "idempotent".

This should include unit tests to ensure that the setup occurs correctly, and handles things like:
  * Database corruption
  * Migration
  * The base case (no database)
  * The common case (up to date database)

Then, in a new commit, I'll start migrating individual queries over. I'll start with FormHistory.count, and work my way across until I've got them all. I'll probably write each one as a separate commit.

Then I'll start getting rid of the dead code, and the old Services.storage stuff. That'll be it's own commit as well.

I expect all of this to land at the same time - but I'm breaking it up this way to make reviewing easier.

Wish me luck!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8933796 [details]
Bug 888784 - Add a new Sqlite.jsm based database connector to FormHistory.jsm.

https://reviewboard.mozilla.org/r/204688/#review211014

::: toolkit/components/satchel/FormHistory.jsm:902
(Diff revision 1)
> +  async _establishConn(attemptNum = 0) {
> +    log(`Establishing database connection - attempt # ${attemptNum}`);
> +    let conn;
> +    try {
> +      conn = await Sqlite.openConnection({ path: this.path });
> +    } catch (e) {

Please add a note to check the error reason, once bug Bug 1407778 will be fixed, we should probably not throw away the user data if the db is not corrupt but fails to open for other reasons (like no disk space or a fancy antivirus locking it)

::: toolkit/components/satchel/FormHistory.jsm:915
(Diff revision 1)
> +        await conn.close();
> +      }
> +
> +      log("Establishing connection failed too many times. Giving up.");
> +
> +      throw e;

nit: I'd probably remove a newline or two from here to compact a little bit

::: toolkit/components/satchel/FormHistory.jsm:919
(Diff revision 1)
> +
> +      throw e;
> +    }
> +
> +    try {
> +      let dbVersion = parseInt(await conn.getSchemaVersion(), 10);

Ah hum, looks like getSchemaVersion documentation lies since it states @return Promise<int>.
It also doesn't make sense to return a string, it escapes me why we do that...
Would you mind filing a bug to fix that in Toolkit / Storage (should be trivial so feel free to fix it if you wish) and fixing the callpoints using parseInt? (I can only find HomeProvider.jsm doing that)

::: toolkit/components/satchel/FormHistory.jsm:960
(Diff revision 1)
> +                                   Cr.NS_ERROR_FILE_CORRUPTED);
> +      }
> +
> +      let tablesExist = await conn.tableExists("moz_formhistory");
> +
> +      if (!tablesExist) {

You could probably save a read here by just checking dbVersion == 0. I don't think we still support a case where schema may be larger than zero while moz_formhistory doesn't exist.
Attachment #8933796 - Flags: review?(mak77) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8933797 [details]
Bug 888784 - Make FormHistory.count use Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204690/#review211074

::: toolkit/components/satchel/FormHistory.jsm:1088
(Diff revision 1)
> +      defaultHandlers.handleError = handlers.handleError;
> +    }
> +
> +    if (handlers.handleCompletion) {
> +      defaultHandlers.handleCompletion = handlers.handleCompletion;
> +    }

nit: please remove the newlines between these 3 ifs, to compact this a little bit.

::: toolkit/components/satchel/FormHistory.jsm:1147
(Diff revision 1)
> -          );
> -        }
> -      },
> -    };
>  
> -    stmt.executeAsync(handlers);
> +    this.db.then(async conn => {

So, I'm actually wondering if we should go a little bit further. I'd like if long term we could remove the 3-callbacks API in FormHistory and move to a much simpler resolve/reject strategy.

As such, I wonder if we could already start by presenting a double API here, so both fulfill the callbacks, but also return a promise.
Count seems to perfectly support this, since it can only resolve to an integer or reject.
We can then convert consumers later in time.

::: toolkit/components/satchel/FormHistory.jsm:1152
(Diff revision 1)
> -    stmt.executeAsync(handlers);
> +    this.db.then(async conn => {
> +      try {
> +        await conn.executeCached(query, aSearchData, row => {
> +          let count = row.getResultByName("numEntries");
> +          handlers.handleResult(count);
> +        });

In this case we have only one result, so you can just
let rows = await conn.executeCached()...
let count = rows[0].getResultByName("numEntries");
The row handler is usually used when there are multiple results and we care about shortening the callback time for the first results.
Attachment #8933797 - Flags: review?(mak77) → review+

Comment 17

a year ago
mozreview-review
Comment on attachment 8933798 [details]
Bug 888784 - Make FormHistory.search use Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204692/#review211086

::: toolkit/components/satchel/FormHistory.jsm:1108
(Diff revision 1)
> +      query += " WHERE " + queryTerms;
> +    }
>  
> -    let handlers = {
> -      handleResult(aResultSet) {
> -        for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
> +    let handlers = this._prepareHandlers(aHandlers);
> +
> +    this.db.then(async conn => {

Search is a little bit more complex to support than count with promises, but I think it could work similarly to Sqlite.execute, thus have an onRow handler that returns results fater, and return a promise that either resolves to an array of results (eventually empty) or rejects.
the onRow handler could be in alternative to aHandlers, so if it's a function is onRow, if it's an object is old callbacks.
Now, I'm not pretending you do all that work here, so if you think some of these changes are requiring too much time, feel free to follow-up.
Attachment #8933798 - Flags: review?(mak77) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8933799 [details]
Bug 888784 - Remove FormHistory.getSchemaVersion and update some tests that used it to use add_task.

https://reviewboard.mozilla.org/r/204694/#review211096

::: toolkit/components/satchel/FormHistory.jsm:1398
(Diff revision 1)
>      return pending;
>    },
>  
> -  get schemaVersion() {
> -    return dbConnection.schemaVersion;
> +  getSchemaVersion() {
> +    return this.db.then(conn => conn.getSchemaVersion());
>    },

Since this is a test-only thing, why don't we just add an head utils to open a short-lived Sqlite.jsm connection to formhistory.sqlite and read the value, and remove this pointless API?

::: toolkit/components/satchel/test/unit/head_satchel.js:55
(Diff revision 1)
>        }
>      },
>    });
>  }
>  
> +function promiseSearchEntries(terms, params) {

This would probably be much simpler with the suggested changes to directly return a promise.

::: toolkit/components/satchel/test/unit/test_async_expire.js:57
(Diff revision 1)
>    let now = 1000 * Date.now();
> -  let updateLastUsed = function updateLastUsedFn(results, age) {
> +  let updateLastUsed = (results, age) => {
>      let lastUsed = now - age * 24 * PR_HOURS;
>  
>      let changes = [];
>      for (let r = 0; r < results.length; r++) {

looks like it could be converted to for...of

::: toolkit/components/satchel/test/unit/test_history_api.js:63
(Diff revision 1)
> -                         },
> -                       });
> -  });
> -}
> -
>  function promiseCountEntries(name, value, checkFn) {

can we replace the promiseCountEntries in head_satchel with this one, by providing a default checkFn?
Attachment #8933799 - Flags: review?(mak77)

Updated

a year ago
Whiteboard: [qf:p1] → [qf:i60][qf:p1]

Comment 19

a year ago
mozreview-review
Comment on attachment 8933800 [details]
Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204696/#review211214

::: toolkit/components/satchel/FormHistory.jsm:627
(Diff revision 1)
>  }
>  
>  /**
> + * @typedef {Object} InsertQueryData
> + * @property {Object} A change requested by FormHistory.
> + * @property {String} The insert query string.

shouldn't these have names?

::: toolkit/components/satchel/FormHistory.jsm:765
(Diff revision 1)
> +          let queryParams = {
> +            lastUsed: now,
> +            guid: change.guid,
> +          };
> +
> +          queries.push(conn.executeCached(query, queryParams));

nit: You can probably avoid the query and queryParams temp vars and just pass to executeCached

::: toolkit/components/satchel/FormHistory.jsm:796
(Diff revision 1)
> -      stmts.push(stmt);
>      }
>    }
>  
> -  for (let stmt of stmts) {
> -    stmt.bindParameters(bindingArrays.get(stmt));
> +  try {
> +    await Promise.all(queries);

The previous implementation was wrapping all statements in a transaction, this doesn't (see executeTransaction).
I think the best solution would be to collect all sql and their params, and executeTransaction them here, rather than running all of the above code in it (even if the transaction is deferred and thus active only after the first write, so the problem may be limited and we could accept it if refactoring code is more complex).

::: toolkit/components/satchel/FormHistory.jsm:805
(Diff revision 1)
> -    handleCompletion(aReason) {
> -      if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) {
> -        for (let [notification, param] of notifications) {
> +  for (let [notification, param] of notifications) {
> -          // We're either sending a GUID or nothing at all.
> +    // We're either sending a GUID or nothing at all.
> -          sendNotification(notification, param);
> +    sendNotification(notification, param);
> -        }
> +  }

There is probably a small difference where before we were sending the notifications and then notifying an error, now we may do the opposite. But should we notify on error? if the transaction fails it is rollbacked and notifying any change would be wrong.
Attachment #8933800 - Flags: review?(mak77)

Comment 20

a year ago
mozreview-review
Comment on attachment 8933801 [details]
Bug 888784 - Get rid of FormHistory.shutdown.

https://reviewboard.mozilla.org/r/204698/#review211378
Attachment #8933801 - Flags: review?(mak77) → review+

Comment 21

a year ago
mozreview-review
Comment on attachment 8933802 [details]
Bug 888784 - Make FormHistory.getAutoCompleteResults use Sqlite.jsm backend.

https://reviewboard.mozilla.org/r/204700/#review211414
Attachment #8933802 - Flags: review?(mak77) → review+
Note, I'm not sure the failure in browser/components/search/test/browser_426329.js that I see on Try is not caused by this... it sounds like some timing skew problem?

Comment 23

a year ago
mozreview-review
Comment on attachment 8933803 [details]
Bug 888784 - Make FormHistory.expireOldEntries use new Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204702/#review211416

While looking at this patch, I saw that the "formhistory-expire-now" notification is completely useless, the only use is in test_async_expire.js, that could well just do formHistoryStartup.QueryInterface(Ci.nsIObserver).observe(null, "idle-daily", "");
If you have a minute to spare, it would save an addObserver call on startup.
Attachment #8933803 - Flags: review?(mak77) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8933804 [details]
Bug 888784 - Get rid of dead mozIStorageService code in FormHistory.jsm.

https://reviewboard.mozilla.org/r/204704/#review211418
Attachment #8933804 - Flags: review?(mak77) → review+
(Assignee)

Updated

a year ago
Blocks: 1423729
(Assignee)

Comment 25

a year ago
mozreview-review-reply
Comment on attachment 8933796 [details]
Bug 888784 - Add a new Sqlite.jsm based database connector to FormHistory.jsm.

https://reviewboard.mozilla.org/r/204688/#review211014

> Please add a note to check the error reason, once bug Bug 1407778 will be fixed, we should probably not throw away the user data if the db is not corrupt but fails to open for other reasons (like no disk space or a fancy antivirus locking it)

Filed bug 1423729 to come back to this once bug 1407778 lands. Also added a comment referring to the bug.

> Ah hum, looks like getSchemaVersion documentation lies since it states @return Promise<int>.
> It also doesn't make sense to return a string, it escapes me why we do that...
> Would you mind filing a bug to fix that in Toolkit / Storage (should be trivial so feel free to fix it if you wish) and fixing the callpoints using parseInt? (I can only find HomeProvider.jsm doing that)

I filed bug 1423732 for getSchemaVersion returning a string instead of an integer.
(Assignee)

Comment 26

a year ago
mozreview-review-reply
Comment on attachment 8933800 [details]
Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204696/#review211214

> The previous implementation was wrapping all statements in a transaction, this doesn't (see executeTransaction).
> I think the best solution would be to collect all sql and their params, and executeTransaction them here, rather than running all of the above code in it (even if the transaction is deferred and thus active only after the first write, so the problem may be limited and we could accept it if refactoring code is more complex).

> The previous implementation was wrapping all statements in a transaction

Are you certain? The only transaction I see is in the migration... where is the transaction starting and ending?
ni? for comment 26
Flags: needinfo?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

a year ago
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #26)
> Are you certain? The only transaction I see is in the migration... where is
> the transaction starting and ending?

conn.executeStatemants, when there is more than 1 writing statement, wraps everything in a transaction:
https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/storage/mozStorageAsyncStatementExecution.cpp#525
Flags: needinfo?(mak77)

Comment 38

a year ago
mozreview-review
Comment on attachment 8933799 [details]
Bug 888784 - Remove FormHistory.getSchemaVersion and update some tests that used it to use add_task.

https://reviewboard.mozilla.org/r/204694/#review213064

::: toolkit/components/satchel/FormHistory.jsm:1116
(Diff revisions 1 - 2)
> +    }
> +
> +    let allResults = [];
> +
> +    return new Promise((resolve, reject) => {
> -    this.db.then(async conn => {
> +      this.db.then(async conn => {

changing search to an async function *may* make this code a bit cleaner, and if this.db throws, nothing is catching it currently? async would reject naturally, you could just return/re-throw.

::: toolkit/components/satchel/FormHistory.jsm:1157
(Diff revisions 1 - 2)
>      }
>  
>      let handlers = this._prepareHandlers(aHandlers);
>  
> +    return new Promise((resolve, reject) => {
> -    this.db.then(async conn => {
> +      this.db.then(async conn => {

as well as count could just be an async function
Attachment #8933799 - Flags: review?(mak77) → review+
(Assignee)

Comment 39

a year ago
mozreview-review-reply
Comment on attachment 8933800 [details]
Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204696/#review211214

> There is probably a small difference where before we were sending the notifications and then notifying an error, now we may do the opposite. But should we notify on error? if the transaction fails it is rollbacked and notifying any change would be wrong.

Okay, I'll not send notifications on rollback.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hey Standard8, so I'm getting some shutdown leaks from this change, and I remember you having to deal with leaks when moving some Places stuff over to async interfaces / transactions.

The shutdown leak is caused by the "Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend." patch in the series when running browser/components/search/test/browser_private_search_perwindowpb.js:

LOG ERROR | TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_private_search_perwindowpb.js | leaked 1 window(s) until shutdown [url = about:blank] [log…]
LOG ERROR | TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_private_search_perwindowpb.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/browser.xul] 

I'll note that if I remove the conn.executeTransaction that's wrapping the executeCached calls inside updateFormHistoryWrite, the leak goes away.

Any tips on how I can debug this?
Flags: needinfo?(standard8)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #68)
> Hey Standard8, so I'm getting some shutdown leaks from this change, and I
> remember you having to deal with leaks when moving some Places stuff over to
> async interfaces / transactions.
...
> I'll note that if I remove the conn.executeTransaction that's wrapping the
> executeCached calls inside updateFormHistoryWrite, the leak goes away.
> 
> Any tips on how I can debug this?

Unfortunately not really. I don't think we have any good tools for debugging shutdown leaks :-( If we do, then I don't know about them...

For the places stuff, the only things I've come across are:

- Passing arrays from our content code to jsms seems to sometimes cause issues with leaks, if the jsm keeps hold of the array (Marco may know more about this).

- I've occasionally seen things with, I think, the bookmarks toolbar where we've created nodes and then haven't removed them before the test completed, and closed the toolbar (unless it was a window), and hence it then decided to leak.
Flags: needinfo?(standard8)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #68)
> Any tips on how I can debug this?

In a similar test I had to wait for "last-pb-context-exited" after closing the pb window, and that somehow helped. You could try. This is not an explanation of the problem clearly, just a possible workaround.
(In reply to Marco Bonardo [::mak] from comment #70)
> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and
> needinfos from comment #68)
> > Any tips on how I can debug this?
> 
> In a similar test I had to wait for "last-pb-context-exited" after closing
> the pb window, and that somehow helped. You could try. This is not an
> explanation of the problem clearly, just a possible workaround.

Thanks for the suggestion! Unfortunately, waiting for last-pb-context-exited doesn't eliminate the shutdown leak. Neither does waiting for a long period of time.

Also note that this failure is permanent and not intermittent.

I wonder if we have a real leak here somehow with the transaction stuff... :/
It's possible since we are crossing a jsm boundary, we had leaks in the past when the jsm was storing an array from another global, though this far nobody had problems with transactions and leaks (We use them everywhere in Places).
On the other side, b-c tests involving PB windows are known to be leaky in general, it's far from the first time this happens and thus I suspect the problem is still with the PB window :(

If you can reproduce the leak locally with a single test run (or a short list of tests), that'd help and maybe I could also see if I can figure out anything of interest to help.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Fixed the leak - had to move the executeTransaction async function out into its own function... I think the aPreparedHandlers object (which contains functions from those leaking DOM scopes) was being kept alive somehow in the function closure when it was inline in updateFormHistoryWrite. I'm really not certain how this helps prevent that though, since the lines after my new function call also use aPreparedHandlers.... but this at least gets rid of the leak locally for me.

Pushing to try now.

Comment 84

a year ago
mozreview-review
Comment on attachment 8933800 [details]
Bug 888784 - Make FormHistory.update use Sqlite.jsm database backend.

https://reviewboard.mozilla.org/r/204696/#review217058

LGTM. We may want to file a bug to convert consumers of the modernized interfaces (using promises instead of 3 callbacks) and remove the old iface where possible. I think a couple method still rely on the callbacks interface and could be handled apart in other bugs too.

::: toolkit/components/satchel/FormHistory.jsm:784
(Diff revision 6)
>            change.guid = generateGUID();
>          }
> -        stmt = makeAddStatement(change, now, bindingArrays);
> -        notifications.push(["formhistory-add", change.guid]);
> +
> +        let { query, updatedChange } = prepareInsertQuery(change, now);
> +
> +        queries.push({ query, params: updatedChange });

nit: this empty newline can likely be removed
Attachment #8933800 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 90

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80a207ed79ec
Add a new Sqlite.jsm based database connector to FormHistory.jsm. r=mak
https://hg.mozilla.org/integration/autoland/rev/51fb50c1ea68
Make FormHistory.count use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/10c472d10264
Make FormHistory.search use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/18d185fa362e
Remove FormHistory.getSchemaVersion and update some tests that used it to use add_task. r=mak
https://hg.mozilla.org/integration/autoland/rev/73ad820d09ec
Make FormHistory.update use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/46fb8f82f2bf
Get rid of FormHistory.shutdown. r=mak
https://hg.mozilla.org/integration/autoland/rev/67c58cb32ac9
Make FormHistory.getAutoCompleteResults use Sqlite.jsm backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/d92599272745
Make FormHistory.expireOldEntries use new Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/98722ab8c2f6
Get rid of dead mozIStorageService code in FormHistory.jsm. r=mak
(Assignee)

Updated

a year ago
Blocks: 1429108
Backed out for failing mochitest-bc browser_426329.js on Linux and on Android chrome's test_ext_browsingData_formdata.html and robocop's testFormHistory:

https://hg.mozilla.org/integration/autoland/rev/9fc7e71752fd1e9764e900eec7c0b7de43d5f8db

Please take a look: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d9afc5417979939d64843b12458ada9f64445f4f&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Mochitest plain (Windows + ...?): https://treeherder.mozilla.org/logviewer.html#?job_id=155080982&repo=autoland
TEST-UNEXPECTED-FAIL | toolkit/components/satchel/test/test_form_submission_cap2.html | checking for initially empty storage

browser-chrome (Linux + OS X + ...?): https://treeherder.mozilla.org/logviewer.html#?job_id=155077460&repo=autoland
TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_426329.js | Checking length of expected menu - Got 2, expected 1

Android c2: https://treeherder.mozilla.org/logviewer.html#?job_id=155072645&repo=autoland
TEST-UNEXPECTED-FAIL | mobile/android/components/extensions/test/mochitest/test_ext_browsingData_formdata.html | reference form entry should be deleted. - got 1, expected +0

robocop: https://treeherder.mozilla.org/logviewer.html#?job_id=155063694&repo=autoland
TEST-UNEXPECTED-FAIL | testFormHistory | Exception caught - java.lang.NullPointerException
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)
These additional patches fix all but the robocop failure, which I'm currently at a loss to explain or understand. :(

Hey nalexander - we chatted in #mobile the other day about how I might need to wait for the FormHistory database to be created from the Gecko-side of things before the testFormHistory test proceeds. Do you have a sense of how I can verify this? I tried creating something similar to blockForGeckoReady() called blockForFormHistoryReady() that waited for a FormHistory:Ready message from browser.js once the database was fully set-up but that didn't really seem to help.

I'm kind of at a loss to understand where things are falling over, to be honest. Are you able to provide any guidance on how I've broken things? Or do you know who I could ask for guidance?
Flags: needinfo?(mconley) → needinfo?(nalexander)
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #97)
> These additional patches fix all but the robocop failure, which I'm
> currently at a loss to explain or understand. :(

> Hey nalexander - we chatted in #mobile the other day about how I might need
> to wait for the FormHistory database to be created from the Gecko-side of
> things before the testFormHistory test proceeds. Do you have a sense of how
> I can verify this? I tried creating something similar to
> blockForGeckoReady() called blockForFormHistoryReady() that waited for a
> FormHistory:Ready message from browser.js once the database was fully set-up
> but that didn't really seem to help.

Can you post these patches?  This approach should work, and I expect you're seeing some timing issue around blockForFormHistoryReady -- that approach is flaky.

I vaguely remember we had some code for FormHistory initialization in Fennec -- but now I think that was logins: https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoMessageReceiver.java#15

Observe that there's no response message there, so it doesn't immediately handle your case.  But that approach would start Gecko and get a message in, including during the running of tests (I think).
 
> I'm kind of at a loss to understand where things are falling over, to be
> honest. Are you able to provide any guidance on how I've broken things? Or
> do you know who I could ask for guidance?

I know about as much about this as anyone right now :(  Maybe mcomella knows more, I'll NI him just in case.
Flags: needinfo?(nalexander) → needinfo?(michael.l.comella)
(In reply to Nick Alexander :nalexander from comment #98)
> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and
> needinfos from comment #97)
> > These additional patches fix all but the robocop failure, which I'm
> > currently at a loss to explain or understand. :(
> 
> > Hey nalexander - we chatted in #mobile the other day about how I might need
> > to wait for the FormHistory database to be created from the Gecko-side of
> > things before the testFormHistory test proceeds. Do you have a sense of how
> > I can verify this? I tried creating something similar to
> > blockForGeckoReady() called blockForFormHistoryReady() that waited for a
> > FormHistory:Ready message from browser.js once the database was fully set-up
> > but that didn't really seem to help.
> 
> Can you post these patches?  This approach should work, and I expect you're
> seeing some timing issue around blockForFormHistoryReady -- that approach is
> flaky.
> 
> I vaguely remember we had some code for FormHistory initialization in Fennec
> -- but now I think that was logins:
> https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/GeckoMessageReceiver.java#15

Looks like https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/FormHistoryProvider.java#127 needs to use GeckoMessageReceiver as well.  But that still won't wait for the message to be processed, which is the real issue with this async-ification.

Comment 100

a year ago
mozreview-review
Comment on attachment 8941984 [details]
Bug 888784 - Make FormHistory update transaction immediate.

https://reviewboard.mozilla.org/r/212182/#review218184

Is this strictly necessary for some failure you debugged?
The reason the old API used "immediate" transactions is because on a classic Storage connection your can execute both sync and async statements, and you don't want a sync transaction to contend with an async transaction when both are requiring a deferred transaction. Another case where it can be useful, is when you have multiple connections writing to the same database and you want to impose some kind of ordering.
In your case, since you are using Sqlite.jsm you are using a pure-async connection (where every operation is naturally serialized), and you have a single connection writing to the db, so there should be no benefit in using immediate.
Attachment #8941984 - Flags: review?(mak77) → review-

Comment 101

a year ago
mozreview-review
Comment on attachment 8941988 [details]
Bug 888784 - Use Sqlite.shutdown instead of AsyncShutdown to close FormHistory database connection.

https://reviewboard.mozilla.org/r/212190/#review218188
Attachment #8941988 - Flags: review?(mak77) → review+

Comment 102

a year ago
mozreview-review
Comment on attachment 8941987 [details]
Bug 888784 - Make satchel tests wait for FormHistory to be clear when shutting down.

https://reviewboard.mozilla.org/r/212188/#review218194
Attachment #8941987 - Flags: review?(mak77) → review+
(Assignee)

Comment 103

a year ago
mozreview-review-reply
Comment on attachment 8941984 [details]
Bug 888784 - Make FormHistory update transaction immediate.

https://reviewboard.mozilla.org/r/212182/#review218184

> Is this strictly necessary for some failure you debugged?

Actually, no - this is just something I noticed while I was debugging. I didn't realize that we'd made immediate mode unnecessary with the switch to Sqlite.jsm. I'll drop this patch from the series. Thanks for explaining!
Here's the attempt I made at making the testFormHistory.java robocop test pass... I was trying to wait for browser.js to report that the FormHistory database had been successfully created.
attachment 8942218 [details] [diff] [review] shows my attempt. See anything obvious that I'm missing? Full disclosure: I have no idea what I'm doing.
Flags: needinfo?(nalexander)

Comment 106

a year ago
mozreview-review
Comment on attachment 8941986 [details]
Bug 888784 - Prevent duplicate fieldname/value pairs from being inserted in FormHistory.

https://reviewboard.mozilla.org/r/212186/#review218212

Not thrilled by this, there should be protection at the schema level... but I'd also not like to have a large index just for it, so for now it will do.

::: toolkit/components/satchel/FormHistory.jsm:354
(Diff revision 1)
> +    }
> +
> +    return false;
> +  },
> +
> +  clear(fieldnamesAndValues) {

do you plan to expand usage in the future?
Off-hand currently looks like you would just need a no-argument clear() that just clears the Map rather than a selective one (and then you could remove "adds"). But maybe I'm misreading it.
Attachment #8941986 - Flags: review?(mak77) → review+
(Assignee)

Comment 107

a year ago
mozreview-review-reply
Comment on attachment 8941986 [details]
Bug 888784 - Prevent duplicate fieldname/value pairs from being inserted in FormHistory.

https://reviewboard.mozilla.org/r/212186/#review218212

> Not thrilled by this, there should be protection at the schema level...

I agree - though I'm worried there are probably already violations of this constraint out in the wild too, since this appears to be a pretty trivial case to enter by just calling FormHistory.update() in quick succession.

> do you plan to expand usage in the future?
> Off-hand currently looks like you would just need a no-argument clear() that just clears the Map rather than a selective one (and then you could remove "adds"). But maybe I'm misreading it.

> Off-hand currently looks like you would just need a no-argument clear() that just clears the Map rather than a selective one (and then you could remove "adds"). But maybe I'm misreading it.

I'm keeping it around because while waiting for the transaction to run, it's possible that other FormHistory.update() calls will come in that attempt to add to the same fieldname with different values, and I don't want to blow away the list of in-progress adds to a fieldname after the first transaction is finally committed.

So I'm going to drop this for now.

Thanks for the review!
> Maybe mcomella knows more, I'll NI him just in case.

I briefly looked into this. The following code doesn't seem right:

+            Actions.EventExpecter formHistoryReadyExpector =
+                    mActions.expectGlobalEvent(Actions.EventType.GECKO, "FormHistory:Ready");
+            if (!GeckoThread.isRunning()) {
+                formHistoryReadyExpector.blockForEvent(GECKO_READY_WAIT_MS, true);
+            }
+            formHistoryReadyExpector.unregisterListener();

I think blockForGeckoReady does this because `if (GeckoThread.isRunning())`, it's mission is already completed and it can return without blocking.

For FormHistory, I'd guess we'd just want to block for the event, no matter what the GeckoThread state is (assuming it correctly handles blocking before Gecko starts). I'd be concerned about missing the event though, if the listener isn't set soon enough.

---

Otherwise, I don't really know anything about FormHistory. One general suggestion I might make is to run the test with a debugger (can you do that for robocop?) and you can inspect the state. You should be able to attach a JS debugger too to get the full picture.

Here are some links for myself if I need to look at this again:
- The failing line: https://hg.mozilla.org/integration/autoland/file/d9afc5417979939d64843b12458ada9f64445f4f/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testFormHistory.java#l52
- The mentioned IRC conversation: https://mozilla.logbot.info/mobile/20180110#c14110256
Flags: needinfo?(michael.l.comella)

Comment 109

a year ago
mozreview-review
Comment on attachment 8941985 [details]
Bug 888784 - Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished.

https://reviewboard.mozilla.org/r/212184/#review218268

While this looks fine for `formdata`, you're changing the general case behaviour. A different commit message would be appropriate, I think, as well as double checking that this won't regress some buggy code upstream by accident.

::: mobile/android/modules/Sanitizer.jsm:67
(Diff revision 1)
> +
> +        if (canClearResult) {
> +          await item.clear(options);
> +        }
> +      };
> +      return maybeDoClear();

This is change is a bit subtle, and I think your commit message is wrong.

If I read this right, you're actually fixing a... bug? that affects the general case: if `canClear` is a function, `_clear` wouldn't return anything (making it impossible to correctly order execution based on the work in `item.clear`); otherwise, it would return a Promise.

From a brief search, users of Sanitizer.clearItem are assuming it returns a promise, which wasn't always the case prior to this change.

I'm guessing your change will subtly change how calls like Sanitizer.clearItem("cache") behave. Can you double check that we're still doing the right thing?
Attachment #8941985 - Flags: review?(gkruglov) → review+
(Assignee)

Comment 110

a year ago
mozreview-review-reply
Comment on attachment 8941985 [details]
Bug 888784 - Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished.

https://reviewboard.mozilla.org/r/212184/#review218268

> This is change is a bit subtle, and I think your commit message is wrong.
> 
> If I read this right, you're actually fixing a... bug? that affects the general case: if `canClear` is a function, `_clear` wouldn't return anything (making it impossible to correctly order execution based on the work in `item.clear`); otherwise, it would return a Promise.
> 
> From a brief search, users of Sanitizer.clearItem are assuming it returns a promise, which wasn't always the case prior to this change.
> 
> I'm guessing your change will subtly change how calls like Sanitizer.clearItem("cache") behave. Can you double check that we're still doing the right thing?

You're right - I've got my bug-blinders on, and didn't remember that I'm changing the clear behaviour for _all_ data types. I'll do a check around the tree to make sure this is going to be okay.
(Assignee)

Comment 111

a year ago
mozreview-review-reply
Comment on attachment 8941985 [details]
Bug 888784 - Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished.

https://reviewboard.mozilla.org/r/212184/#review218268

> You're right - I've got my bug-blinders on, and didn't remember that I'm changing the clear behaviour for _all_ data types. I'll do a check around the tree to make sure this is going to be okay.

It looks like Sanitizer.jsm switched over to using a Promise-API back in bug 1001309. From the looks of [the patch that added the support], Sanitizer.sanitize was modified to be asynchronous too (it would already send a message to the Java-side of things when completed, but it was also modified to take a callback).

A cursory check around the codebase, and it seems as if the other consumers of Fennec's Sanitizer.jsm expect the Promise API.

> I'm guessing your change will subtly change how calls like Sanitizer.clearItem("cache") behave.

Are you sure? `clearItem("cache")` will skip the branch I changed because the canClear `property` returns a boolean and not a function.

Or am I misunderstanding?
ni? for comment 111
Flags: needinfo?(gkruglov)

Comment 113

a year ago
mozreview-review
Comment on attachment 8941985 [details]
Bug 888784 - Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished.

https://reviewboard.mozilla.org/r/212184/#review218342

::: mobile/android/modules/Sanitizer.jsm:67
(Diff revision 1)
> +
> +        if (canClearResult) {
> +          await item.clear(options);
> +        }
> +      };
> +      return maybeDoClear();

Ah, I picked a bad example. 'syncedTabs' is the only one that'll hit your change.

Updated

a year ago
Flags: needinfo?(gkruglov)
(Assignee)

Comment 114

a year ago
mozreview-review-reply
Comment on attachment 8941985 [details]
Bug 888784 - Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished.

https://reviewboard.mozilla.org/r/212184/#review218342

> Ah, I picked a bad example. 'syncedTabs' is the only one that'll hit your change.

Ah, I see - I assume you're referring to what the Promise resolves to? Because it looks like the syncedTabs thing always returned a Promise, via https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/mobile/android/modules/geckoview/Messaging.jsm#128-138

You're right though - the Promise resolves to the response from the Java side of things. I'll modify my patch so that we resolve with the resolving value of the item.clear() Promise, or undefined if we don't end up calling item.clear (because we determined that we can't clear).

Does that work for you?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8941984 - Attachment is obsolete: true
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #114)
> Does that work for you?

Yup, thanks for double checking!

Comment 121

a year ago
mozreview-review
Comment on attachment 8942408 [details]
Bug 888784 - Modify testFormHistory Fennec test to wait until FormHistory database is created.

https://reviewboard.mozilla.org/r/212714/#review218598

With the re-ordering below -- or a counter for why I'm incorrect -- this looks good.  If it's green in try (run `robocop-*`) it's good for me!

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/OldBaseTest.java:100
(Diff revision 1)
>      }
>  
> +    protected void blockForFormHistoryReady() {
> +        try {
> +            EventDispatcher.getInstance().dispatch("FormHistory:Init", null);
> +            Actions.EventExpecter formHistoryReadyExpector =

It's not clear from comments (and maybe not clear from usage), but you need to create this _before_ you dispatch the `:Init` message.

`expectGlobalEvent` starts listening immediately, and as written there's a race window whereby you might miss the `:Ready` message.  If you do this before `:Init`, you'll catch that message no matter what, and safely `block` for it.

These names and this usage pattern is confusing; `block` will "see" messages that were queued after `expect` but *before* you started blocking.
Attachment #8942408 - Flags: review?(nalexander) → review+
(Assignee)

Comment 122

a year ago
mozreview-review-reply
Comment on attachment 8942408 [details]
Bug 888784 - Modify testFormHistory Fennec test to wait until FormHistory database is created.

https://reviewboard.mozilla.org/r/212714/#review218598

> It's not clear from comments (and maybe not clear from usage), but you need to create this _before_ you dispatch the `:Init` message.
> 
> `expectGlobalEvent` starts listening immediately, and as written there's a race window whereby you might miss the `:Ready` message.  If you do this before `:Init`, you'll catch that message no matter what, and safely `block` for it.
> 
> These names and this usage pattern is confusing; `block` will "see" messages that were queued after `expect` but *before* you started blocking.

Ah, okay - that makes sense. Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Pushed to try to see the final verdict here. I'm going to wait until after soft freeze before I consider landing this, anyways.
Flags: needinfo?(nalexander)

Comment 139

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2682e434b20
Add a new Sqlite.jsm based database connector to FormHistory.jsm. r=mak
https://hg.mozilla.org/integration/autoland/rev/291e111da6ef
Make FormHistory.count use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/5f88a6ef1aac
Make FormHistory.search use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/53cb7d7c71da
Remove FormHistory.getSchemaVersion and update some tests that used it to use add_task. r=mak
https://hg.mozilla.org/integration/autoland/rev/dc3b1653e70c
Make FormHistory.update use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/f33b9cc53b30
Get rid of FormHistory.shutdown. r=mak
https://hg.mozilla.org/integration/autoland/rev/a799d6cd42e4
Make FormHistory.getAutoCompleteResults use Sqlite.jsm backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/9e70d1f4489e
Make FormHistory.expireOldEntries use new Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/8a44ddb5f2c2
Get rid of dead mozIStorageService code in FormHistory.jsm. r=mak
https://hg.mozilla.org/integration/autoland/rev/5044e2244d17
Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/456d0f3e4e34
Prevent duplicate fieldname/value pairs from being inserted in FormHistory. r=mak
https://hg.mozilla.org/integration/autoland/rev/4f7816c12e0b
Make satchel tests wait for FormHistory to be clear when shutting down. r=mak
https://hg.mozilla.org/integration/autoland/rev/4f1b033d3cc0
Use Sqlite.shutdown instead of AsyncShutdown to close FormHistory database connection. r=mak
https://hg.mozilla.org/integration/autoland/rev/8c3fc3cb35b7
Modify testFormHistory Fennec test to wait until FormHistory database is created. r=nalexander
Thanks, NarcisB.

Following Standard8's advice here - I'm modifying the FormHistory.update patch to set // eslint-disable-next-line complexity on updateFormHistoryWrite, and have filed bug 1432208 to do the refactor.
Flags: needinfo?(mconley)
(Assignee)

Updated

a year ago
See Also: → bug 1432208
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 156

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0515e1964c93
Add a new Sqlite.jsm based database connector to FormHistory.jsm. r=mak
https://hg.mozilla.org/integration/autoland/rev/5dc6712167d9
Make FormHistory.count use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/741f048ffa80
Make FormHistory.search use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/343e228b79cb
Remove FormHistory.getSchemaVersion and update some tests that used it to use add_task. r=mak
https://hg.mozilla.org/integration/autoland/rev/b3d54042dc65
Make FormHistory.update use Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/cb495c23a3dc
Get rid of FormHistory.shutdown. r=mak
https://hg.mozilla.org/integration/autoland/rev/749c9c9e93fc
Make FormHistory.getAutoCompleteResults use Sqlite.jsm backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/0555c44a9034
Make FormHistory.expireOldEntries use new Sqlite.jsm database backend. r=mak
https://hg.mozilla.org/integration/autoland/rev/c40d452ccef9
Get rid of dead mozIStorageService code in FormHistory.jsm. r=mak
https://hg.mozilla.org/integration/autoland/rev/3b128657b9eb
Make sure Fennec's Sanitizer.jsm resolves a clearing Promise only after a data clearing attempt has finished. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/9e83248b0485
Prevent duplicate fieldname/value pairs from being inserted in FormHistory. r=mak
https://hg.mozilla.org/integration/autoland/rev/2b2c3a2923a9
Make satchel tests wait for FormHistory to be clear when shutting down. r=mak
https://hg.mozilla.org/integration/autoland/rev/73f44bb6788e
Use Sqlite.shutdown instead of AsyncShutdown to close FormHistory database connection. r=mak
https://hg.mozilla.org/integration/autoland/rev/f13e81b2bad0
Modify testFormHistory Fennec test to wait until FormHistory database is created. r=nalexander
Blocks: 987745
You need to log in before you can comment on or make changes to this bug.