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.

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.

Any objections if I try to drive this to completion, mak?

(Tentatively assigning self while I wait for mak to get back to me)

absolutely not, compared to comment 1, we should just use Sqlite.jsm that does the right thing.

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 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.

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.

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.

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?

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.

Comment on attachment 8933801 [details] Bug 888784 - Get rid of FormHistory.shutdown. https://reviewboard.mozilla.org/r/204698/#review211378

Comment on attachment 8933802 [details] Bug 888784 - Make FormHistory.getAutoCompleteResults use Sqlite.jsm backend. https://reviewboard.mozilla.org/r/204700/#review211414

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 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.

Comment on attachment 8933804 [details] Bug 888784 - Get rid of dead mozIStorageService code in FormHistory.jsm. https://reviewboard.mozilla.org/r/204704/#review211418

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.

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

(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

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

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.

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?

(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.

(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.

For reference, this is what helped in the past (re last-pb-context-exited): https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js#61

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 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

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

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

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?

(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.

(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 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.

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

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

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.

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.

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

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?

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.

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

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.

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?

(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 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.

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!

Pushed to try to see the final verdict here. I'm going to wait until after soft freeze before I consider landing this, anyways.

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

Backed out for ESlint failure on /FormHistory.jsm https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a50c869e1eada65ef60353028d554617ca810495 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=157736210&repo=autoland&lineNumber=249 Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8c3fc3cb35b7c626220763c92aca700d2f33a208

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.

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

https://hg.mozilla.org/mozilla-central/rev/0515e1964c93 https://hg.mozilla.org/mozilla-central/rev/5dc6712167d9 https://hg.mozilla.org/mozilla-central/rev/741f048ffa80 https://hg.mozilla.org/mozilla-central/rev/343e228b79cb https://hg.mozilla.org/mozilla-central/rev/b3d54042dc65 https://hg.mozilla.org/mozilla-central/rev/cb495c23a3dc https://hg.mozilla.org/mozilla-central/rev/749c9c9e93fc https://hg.mozilla.org/mozilla-central/rev/0555c44a9034 https://hg.mozilla.org/mozilla-central/rev/c40d452ccef9 https://hg.mozilla.org/mozilla-central/rev/3b128657b9eb https://hg.mozilla.org/mozilla-central/rev/9e83248b0485 https://hg.mozilla.org/mozilla-central/rev/2b2c3a2923a9 https://hg.mozilla.org/mozilla-central/rev/73f44bb6788e https://hg.mozilla.org/mozilla-central/rev/f13e81b2bad0