This probably involves a rewrite of nsIFormHistory2 and nsIFormAutocomplete APIs.

I'm looking at this bug, so I'm wondering if someone can give me a heads up if I'm going in the completely wrong direction. With the rewrite of nsIFormHistory2, is it sufficient to leave the interface as-is, replace all the calls in toolkit/components/satchel/nsFormHistory.js from execute to executeAsync and make sure that all the callers are handling it correctly?

(In reply to Felix Fung (:felix) from comment #2) > With the rewrite of nsIFormHistory2, is it sufficient to leave the interface > as-is, replace all the calls in toolkit/components/satchel/nsFormHistory.js > from execute to executeAsync and make sure that all the callers are handling > it correctly? I don't think you could do this, since we don't necessarily have control of all consumers of the API, and I don't even think there would be a way to make this work if you kept the API the same, since async implies callback functions. I think you would need to implement the API that Paul and Dolske were working on here: https://gist.github.com/2cd878a9fc2dc78fff37. (cc'ing Dolske and Paul, since they know more than me)

Yea, the plan is to just go ahead and add new APIs. We considered keeping the sync stuff in place but I don't think we're going to do that. For the time being though, you could implement the async API alongside the existing API and then you can scrap the sync stuff as you get further along. The consumers in tree are really minimal (IIRC it's just the actual form history controller and sync... actually I think sync just uses the db connection & queries directly...). But I think Margaret's right in that we couldn't user the existing API as is for async anyway.

- Implemented add, modify, remove and search I'm not sure what significance some of the old notifications had and whether or not to keep them around or refactor them out of existence. (e.g. before-removeEntry)

Additional Changes: - Wrote async batch - Made code run I'm not sure about the right way to write batch. It would seem that we want each sql query to have a callback but dbconnection.executeAsync on the connection doesn't really provide that. If we do it otherwise can we still leverage any benefit from having a function like batch?

You may manually begin and commit a transaction and put async statements into it, each one with its own callback. This requires you putting lot of attention in not leaving a transaction open by error. An alternative may consist in using a notifications temp table, and as last steps in executeAsync select * from it and delete from it, so in handleResult you get data from the notifications table and notify. To populate the notifications table you may use temp triggers or run a select into before the actual delete (nsPlacesExpiration does something similar). I also don't think you want to put search in the batch, batches are nice for write operations, useless for reads.

Comment on attachment 566521 [details] [diff] [review] WIP2 - Async draft for nsFormHistory.js Review of attachment 566521 [details] [diff] [review]: ----------------------------------------------------------------- a couple drive-by comments: you MUST .asyncClose() the connection handle. ::: toolkit/components/satchel/nsAsyncFormHistory.js @@ +538,5 @@ > + stmt = this.dbCreateStatement(query, params); > + if (stmt.executeStep()) { > + id = stmt.row.id; > + guid = stmt.row.guid; > + } this can create thread contention, if you use async and sync apis on the same connection. does this really have to be synchronous? @@ +571,5 @@ > + let stmt = this.dbStmts[query]; > + // Memoize the statements > + if (!stmt) { > + this.log("Creating new statement for query: " + query); > + stmt = this.dbConnection.createStatement(query); you should never use createStatement if you run the query asynchronously, always use createAsyncStatement.

Additional Changes: - Batch uses begin/commitTransaction - Fixed some handlers to use handleCompletion instead of handleResult - Replaced getExistingID with waitForExistingID (async call)

Forgot to mention, WIP3: - Cleans up database connection - Calls createAsyncStatement instead of createStatement - Got rid of notification stuff until it is better decided which notifications make sense

A pretty close to feature-complete implementation of the proposed asynchronous form history api here: https://gist.github.com/2cd878a9fc2dc78fff37 Additional Changes: - Used closures to keep synchronize batch statements Some bits of the API are vague/undefined as noted in the comments.

aSearchData format needs to be better specified. Most notably, how would 'removeEntriesByTimeframe' look like?

Additional Changes: - Got rid of XPCOM interface - Added pseudo-selectors (beginTime/endTime) for API

Use the AsyncFormHistory API to 'Clear Recent History'. Additional Note: There is a call to SyncFormHistory.hasEntries which synchronously returns the number of rows in the form history. Is it worthwhile refactoring to make this call Async? If so, we're going to need some other API call because it seems rather wasteful to use search to replicate this behaviour.

Dolske, if your review queue is loaded I'd be happy to do a first-pass, if you wish.

Comment on attachment 567812 [details] [diff] [review] nsIAsyncFormHistory Talked to Dolske, would love if you could do a first pass

Comment on attachment 567816 [details] [diff] [review] nsIAsyncFormHistory - sanitize.js Review of attachment 567816 [details] [diff] [review]: ----------------------------------------------------------------- also mobile has sanitize.js (http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/sanitize.js) you may want to also patch that and get r from mfinkle

Comment on attachment 567812 [details] [diff] [review] nsIAsyncFormHistory Review of attachment 567812 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, didn't have plenty of time today, but there is likely enough to begin cleaning up the thing. Will love to look deeper at next iteration. ::: toolkit/components/satchel/AsyncFormHistory.jsm @@ +12,5 @@ > + * License. > + * > + * The Original Code is mozilla.org code. > + * > + * The Initial Developer of the Original Code is Mozilla Foundation. nit: the Mozilla Foundation @@ +13,5 @@ > + * > + * The Original Code is mozilla.org code. > + * > + * The Initial Developer of the Original Code is Mozilla Foundation. > + * Portions created by the Initial Developer are Copyright (C) 2010 nit: 2011 @@ +16,5 @@ > + * The Initial Developer of the Original Code is Mozilla Foundation. > + * Portions created by the Initial Developer are Copyright (C) 2010 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): nit: you may add yourself to the contributors list, as (original author) @@ +42,5 @@ > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +EXPORTED_SYMBOLS = [ "AsyncFormHistory" ]; > + > +const DB_VERSION = 3; nit: I think it would be clearer as DB_SCHEMA_VERSION @@ +49,5 @@ > +function AsyncFormHistory() { > + this.init(); > +}; > + > +AsyncFormHistory.prototype = { One of the advantages of modules is that you have a single instance of the object and you reuse it across all importers. But this way you are exposing a way to create an instance. So a couple issues: I don't understand how your usage of this module can work in the other patch attached here, at first glance looks like you are doing something like (new asyncformhist.AsyncFormHistory).remove(). I also wonder if was your scope to create a new instance for each implementer. I think you rather want a AsyncFormHistory lazygetter in the module, that creates a singleton instance of your object (called something else), externally you'll just invoke the lazygetter as AsyncFormHistory.remove() and similar. If I misinterpreted something, please let me know. @@ +50,5 @@ > + this.init(); > +}; > + > +AsyncFormHistory.prototype = { > + QueryInterface : XPCOMUtils.generateQI([Ci.nsIAsyncFormHistory, nit: I think I'm not going to discussing code style here, but the ":" labels positioning is fancy compared to the code base... btw, this is stuff to figure out with the module owner. @@ +89,5 @@ > + } > + }, > + dbConnection : null, // The database connection > + dbStmts : null, // Database statements for memoization > + dbFile : null, You don't need to cache dbFile, for a simple reason, once you have a mozIStorageConnection you have a .databaseFile, and later you can easily pass the file handle to the dbInit function. @@ +111,5 @@ > + else > + this._privBrowsingSvc = null; > + } > + return this._privBrowsingSvc; > + }, these service getters may be global defineLazyServiceGetter and defineLazyGetter at the top of the module, would make the object code cleaner @@ +118,5 @@ > + if (!this.debug) > + return; > + dump("FormHistory: " + message + "\n"); > + Services.console.logStringMessage("FormHistory: " + message); > + }, this may be a global function, does not need to be a method of this object. @@ +121,5 @@ > + Services.console.logStringMessage("FormHistory: " + message); > + }, > + > + init : function() { > + let self = this; this is unused @@ +129,5 @@ > + this.updatePrefs(); > + this.dbStmts = {}; > + > + this.messageManager = Cc["@mozilla.org/globalmessagemanager;1"]. > + getService(Ci.nsIChromeFrameMessageManager); this may be a service getter at the top, as well @@ +134,5 @@ > + this.messageManager.loadFrameScript("chrome://satchel/content/formSubmitListener.js", true); > + this.messageManager.addMessageListener("FormHistory:FormSubmitEntries", this); > + > + // so we can shutdown > + Services.obs.addObserver(this, "profile-before-change", false); the comment does not make sense @@ +137,5 @@ > + // so we can shutdown > + Services.obs.addObserver(this, "profile-before-change", false); > + > + try { > + this.dbFile = Services.dirsvc.get("ProfD", Ci.nsIFile).clone(); I don't understand the reason to clone() here, since you don't need to keep both the directory and the database handle @@ +141,5 @@ > + this.dbFile = Services.dirsvc.get("ProfD", Ci.nsIFile).clone(); > + this.dbFile.append("formhistory.sqlite"); > + this.log("Opening database at " + this.dbFile.path); > + > + this.dbInit(); you can pass the file handle to it and avoid to cache it @@ +150,5 @@ > + this.dbCleanup(true); > + this.dbInit(); > + } else { > + throw "Initialization failed"; > + } So, here you may just catch (e if e.result == Cr.NS_ERROR_FILE_CORRUPTED) and let other exceptions go through, rather than rethrowing @@ +155,5 @@ > + } > + }, > + > + shutdown : function () { > + Services.prefs.removeObserver("browser.formfill.", this, false); you may want to remove the profile-before-change observer, doesn't look like this has been designed to be restartable (by a profile-switch, that btw we don't support atm) @@ +167,5 @@ > + let operations = []; > + > + for (let i = 0; i < entries.length; i++) { > + operations.push([ > + Ci.nsIAsyncFormHistory.ADD, nit: to make this more readable, you may at the beginning define something like const ACTION = { ADD: Ci.nsIAsyncFormHistory.ADD , REMOVE: ... } and here just use ACTION.ADD @@ +190,5 @@ > + > + let now = Date.now() * 1000; // microseconds > + params.timesUsed = (aNewData.timesUsed) ? aNewData.timesUsed : 1; > + params.firstUsed = (aNewData.firstUsed) ? aNewData.firstUsed : now; > + params.lastUsed = (aNewData.lastUsed) ? aNewData.lastUsed : now; Why not using the same style for assigning all params? either all params.something = or params = { something:, } @@ +193,5 @@ > + params.firstUsed = (aNewData.firstUsed) ? aNewData.firstUsed : now; > + params.lastUsed = (aNewData.lastUsed) ? aNewData.lastUsed : now; > + > + let stmt = this.dbCreateAsyncStatement(query, params); > + return stmt; the assignment is useless afaict @@ +205,5 @@ > + > + let name = aNewData.fieldname; > + let value = aNewData.value; > + > + if (!(name && value)) { !name || !value may ideally be faster @@ +213,5 @@ > + this._addOrUpdate(0, { newData : aNewData, callbacks: aCallbacks }); > + }, > + > + _addOrUpdate : function (aStage, aParams) { > + // dependent async calls, aStage = 1 depends on aStage = 0 this may better be explained in a proper javadoc on the method... the "stage" concept seems a bit hackish though, may likely be refactored by using separate statementCallbacks... @@ +230,5 @@ > + let guid = aParams['guid']; > + let callbacks = aParams['callbacks']; > + > + if (id != -1) { > + this._doUpdate(id, guid); the assignments seem not that useful, couldn's you just use aParams.id, aParams.guid directly? @@ +245,5 @@ > + let guid = this.generateGUID(); > + let stmt = this._makeAddStatement(guid, aNewData); > + > + stmt.executeAsync({ > + handleResult: function(aResult) {}, as a safety action, you may throw like "Unexpected result" here @@ +263,5 @@ > + }); > + }, > + > + _doUpdate : function (aId, aGuid) { > + let now = Date.now() * 1000; // microseconds since you do multiple times, you may add a microNow (or something like that) getter @@ +341,5 @@ > + } > + > + query += queryTerms.join(", ") + " WHERE guid = :guid"; > + aNewData['guid'] = aGuid; > + stmt = this.dbCreateAsyncStatement(query, aNewData); define stmt on first use @@ +370,5 @@ > + }, > + > + _makeSearchStatement : function (aSearchData) { > + let stmt; > + let query = "SELECT * FROM moz_formhistory"; // XXX select less yeah, * is really not nice nor self documenting @@ +389,5 @@ > + > + let stmt = this._makeSearchStatement(aSearchData); > + stmt.executeAsync({ > + handleResult: function(aResultSet) { > + aResults = []; What is aResults? I don't see its definition, it's not an in param @@ +483,5 @@ > + throw "better exception here: " + op; > + } > + } > + > + this.dbConnection.commitTransaction(); if something in the middle of the transaction throws you are keeping the connection busy forever. And you should handle the rollback case to. Also, since all the operations here are async, you are actually opening a transaction, closing the transaction, then doing the work, that is probably not what you want. You should have a secure final callback that closes the transaction and manage that globally. The only other possibility is putting all statements in an array and use connection.executeAsync, that would likely be better. @@ +499,5 @@ > + queryTerms.push("firstUsed <= :endTime"); > + } else { > + throw "bad field exception"; > + } > + } nit: this looks like a good work for Array.map (getting an array with Object.keys) @@ +517,5 @@ > + } else if (topic == "profile-before-change") { > + this.shutdown(); > + } else { > + this.log("Oops! Unexpected notification: " + topic); > + } switch? @@ +550,5 @@ > + fieldname : name, > + value : value > + }; > + > + stmt = this.dbCreateAsyncStatement(query, params); define var at first use @@ +588,5 @@ > + // Memoize the statements > + if (!stmt) { > + this.log("Creating new statement for query: " + query); > + stmt = this.dbConnection.createAsyncStatement(query); > + this.dbStmts[query] = stmt; this.dbStmts[query] = stmt = this.dbConnection.createAsyncStatement @@ +707,5 @@ > + dbClose : function () { > + // Finalize all statements to free memory, avoid errors later > + for each (let stmt in this.dbStmts) > + stmt.finalize(); > + this.dbStmts = []; = {}

- Addressed the above comments - Used dbConn.executeAsync for batch instead of trying to manage transactions. This means that the batch function no longer has per operation callbacks but just a single batchCallback instead. - Fixed search to behave correctly with no results.

- Make sanitizer use async removal - I don't know if it's worthwhile converting form history calls where blocking makes sense. (e.g. When you open the clear history dialog, we may disable the checkbox to clear form history is there is no history to clear. We perform a search in this case, and it's a lot cleaner for that to be sync.)

- Replaced all usage of nsIFormHistory2 with AsyncFormHistory - This test passes, but leaks

Comment on attachment 572061 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 572061 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/AsyncFormHistory.jsm @@ +14,5 @@ > + * The Original Code is mozilla.org code. > + * > + * The Initial Developer of the Original Code is the Mozilla Foundation. > + * Portions created by the Initial Developer are Copyright (C) 2011 > + * the Initial Developer. All Rights Reserved. Note: you should be using this template instead of copying from another file: http://www.mozilla.org/MPL/boilerplate-1.1/ @@ +62,5 @@ > + QueryInterface : XPCOMUtils.generateQI([Ci.nsIObserver, > + Ci.nsIFrameMessageListener, > + ]), > + > + debug : true, don't check this in set at `true`.

Additional Changes - Fixed typo in modify - Fixed bug in modify where trying to modify GUID would not work - Updated MPL - Search can optionally define what terms it's interested in (e.g. search({ guid : foo }, [ 'fieldname', 'value' ], callbacks) )

I heard the sync team is doing some infra changes to their engines so that it'd be truly async instead of the Async.querySpinningly j0nx that they're doing now. rnewman, I was told you could chip in on how that might change things...

(In reply to Felix Fung (:felix) from comment #24) > I heard the sync team is doing some infra changes to their engines so that > it'd be truly async instead of the Async.querySpinningly j0nx that they're > doing now. rnewman, I was told you could chip in on how that might change > things... Indeed, Felix! I'm traveling right now, so I can give you a proper reply tonight. Is there anything in particular you wanted to know, or should I just give you a general brain dump? This should get you started: https://wiki.mozilla.org/Services/Sync/FxSync/StoreRedesign

Comment on attachment 573623 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 573623 [details] [diff] [review]: ----------------------------------------------------------------- Will we need to switch everything over to this code at one time, or can the sync and async flavors run concurrently? Oh, looks like we're assuming we'll switch over, otherwise both chunks of code will be listening to form submissions. Ok. General issue: you've dropped all the notifications -- sendStringNotification() et al -- which Sync needs so that it can watch what's happening with Form History. Need to add these back, although modified slightly for the new API. TODO: I didn't review the add/update API other than a quick skim. Might want to think about how strictly we should enforce GUID-uniqueness? Also need some tests for this! But overall, let me note that this code is looking very very good! ::: toolkit/components/satchel/AsyncFormHistory.jsm @@ +34,5 @@ > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ > + > +EXPORTED_SYMBOLS = [ "AsyncFormHistory" ]; Thinking from the future, I wonder if we should just call this module+file "FormHistory". I guess we don't do this anywhere else yet (so likely followup fodder), but I wonder if we should try being more robust to accidental API by having a separate public-API-only exported symbol. EG: EXPORTED_SYMBOLS = ["Foo"]; var Foo = { a : function() { return RealFoo.a(); }, b : function() { return RealFoo.b(); }, c : function() { return RealFoo.c(); }, }; var RealFoo { a : function () { ... } b : function () { ... } c : function () { ... } _stuff : ..., _moarStuff : ..., doubleStuff : ..., } [Actually, I suppose there's no real need to wrap things in a RealFoo function, they can all be global functions/properties in the JSMs scope] This way an enterprising extension would be unable to import the module and use Foo._moarStuff, or even redefine what Foo.* things are. ES5 object freezing may or may be be interesting too... Perhaps on just Foo but not RealFoo. @@ +98,5 @@ > + }, > + dbConnection : null, // The database connection > + dbStmts : null, // Database statements for memoization > + > + batching : false, Nit: move up to where debug et al are. @@ +107,5 @@ > + dump("FormHistory: " + message + "\n"); > + Services.console.logStringMessage("FormHistory: " + message); > + }, > + > + init : function() { Note that bug 690903 just changed some of this code, so that the DB is more lazily created. Might want to check bug 698738 and 698738 -- which both just landed -- to see if they're relevant to port too. Oh, and bug 696404 is coming as well. :) @@ +149,5 @@ > + > + receiveMessage: function receiveMessage(message) { > + let entries = message.json; > + let operations = entries.map(function(entry) { > + return [ AsyncFormHistory.ADD, { Hmm, guess this is dropping the "make caller determine if it exists or not" semantics we were thinking of using? @@ +158,5 @@ > + > + this.batch(operations); > + }, > + > + /* ---- nsIFormHistory2 interfaces ---- */ In the words of Chuck Testa... NOPE! This comment can just get nuked. @@ +273,5 @@ > + > + if (queryTerms.length == 0) { > + // remove all entries > + this.log("removeAllEntries"); > + return this.dbCreateAsyncStatement(query); Hmm. We might want to tweak the API here. Maybe... This could be a bit of a footgun, if the caller bungles the call and fails to include the expected aSearchData. Having a separate removeAll() method would make that safer. OTOH, simple APIs ftw, and if code is that badly broken, then that developer fails at programming. @@ +284,5 @@ > + > + remove : function (aSearchData, aCallbacks) { > + let stmt = this._makeRemoveStatement(aSearchData); > + stmt.executeAsync({ > + handleResult: NOOP, Are the NOOPs really needed? I'd sorta hope storage would just handle the case of property == null or an undefined property. Might be a bit easier to read similar to the following: let foo = { handleResult : NOOP, handleError : NOOP, handleCompletion : NOOP }; if (aCallbacks && aCallbacks.onFailure) foo.handleError = function (aErr) { aCallbacks.onFailures (....) }; if (aCallbacks && aCallbacks.onSuccess) foo.handleCompletion = function (aErr) { aCallbacks.onSuccess (....) }; @@ +305,5 @@ > + let query = "UPDATE moz_formhistory SET "; > + let queryTerms = this._makeQueryTerms(aNewData, false); > + > + if (queryTerms.length == 0) { > + throw "better exception here"; Yes, please. :D @@ +314,5 @@ > + > + return this.dbCreateAsyncStatement(query, aNewData); > + }, > + > + modify : function (aGuid, aNewData, aCallbacks) { As with add(), this should do nothing in private browsing mode. I suppose go ahead and check .enabled too, but I wonder if we should just remove that check entirely here. Instead, just have the front-end check if form history is enabled, and if not don't bother calling into here. Hmm. @@ +342,5 @@ > + aSelectTerms = aSelectTerms.filter( > + function (field) { return !(field in aSearchData); }); > + > + if (aSelectTerms.length == 0) { > + throw "can't select nothing"; Instead of an error, maybe it as a "SELECT COUNT(1)" query? I can't recall if that actually faster in some cases, or not. (EG, is your goal is just to show how many entries are in the DB). @@ +359,5 @@ > + } > + return stmt; > + }, > + > + search : function (aSearchData, aSelectTerms, aCallbacks) { Hmm, aSelectTerms wasn't in the API spec, but I can see how it's useful. @@ +361,5 @@ > + }, > + > + search : function (aSearchData, aSelectTerms, aCallbacks) { > + if (!aCallbacks || !aCallbacks.onSuccess) { > + throw "search must have a success handler"; Other functions in the API don't require this... @@ +421,5 @@ > + let guid = this.generateGUID(); > + stmts.push(this._makeAddStatement(guid, aOperation[1])); > + break; > + case AsyncFormHistory.MODIFY: > + stmts.push(this._makeModifyStatement(aOperation[1], aOperation[2])); Same comment re checking .enabled / PB mode as in modify() @@ +460,5 @@ > + > + _makeQueryTerms : function (aQueryData, allowPseudo) { > + let self = this; > + return Object.keys(aQueryData).map(function (field) { > + if (field in self.dbSchema.tables.moz_formhistory) { Do we want to allow callers to use |id| as a searchable field? IIRC, we've avoided that as (1) it's more of an internal identifier / not something callers should be depending on and (2) there's already a GUID @@ +628,5 @@ > + // on. If the columns are borked, something is wrong so blow away > + // the DB and start from scratch. [Future incompatible upgrades > + // should swtich to a different table or file.] > + > + if (!this.dbAreExpectedColumnsPresent()) You forgot to copy this function over. :) @@ +659,5 @@ > + this.dbConnection.schemaVersion = DB_SCHEMA_VERSION; > + this.dbConnection.commitTransaction(); > + this.log("DB migration completed."); > + }, > + Add here: // dbMigrateToVersion1 (bug 463154) -- unsupported as of Firefox 11 // dbMigrateToVersion2 (bug 243136) -- unsupported as of Firefox 11 // dbMigrateToVersion3 (bug 506402) -- unsupported as of Firefox 11 That way it's clear that these were explicitly removed. We should also double check the Firefox 3.6 support plans... Looks like it uses schema v2, so if we want to be paranoid we should keep the v3 migrator a bit longer. @@ +702,5 @@ > + "nsIUUIDGenerator"); > + > +XPCOMUtils.defineLazyServiceGetter(AsyncFormHistory, "_privBrowsingSvc", > + "@mozilla.org/privatebrowsing;1", > + "nsIPrivateBrowsingService"); Move these two up to the top, more visible and less likely to fail if init() should ever end up expecting these to be available. ...oh, you need the object to stick the getters into. Hmm. Well, at least just put these before the init(). If we end up doing the RealFoo thing mentioned above, these should just be setting the getter on the global scope here, and could move to the top.

Comment on attachment 572064 [details] [diff] [review] Use AsyncFormHistory.jsm - sanitize.js Review of attachment 572064 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +272,5 @@ > searchBar.textbox.reset(); > } > > + if (this.range) { > + AsyncFormHistory.remove({ startTime : this.range[0], endTime : this.range[1] }); nit: apart a couple rows where this happens, the common style in this file is to have the colon towards the label as startTime: something, endTime: something. Looks like there is no common code style across our code base though :( @@ +274,5 @@ > > + if (this.range) { > + AsyncFormHistory.remove({ startTime : this.range[0], endTime : this.range[1] }); > + } else { > + AsyncFormHistory.remove({}); Could the object be optional? it's a bit useless to have to create an empty object just to pass it and ignore it. @@ +526,5 @@ > > +XPCOMUtils.defineLazyGetter(this, "AsyncFormHistory", function() { > + Components.utils.import("resource://gre/modules/AsyncFormHistory.jsm"); > + return AsyncFormHistory; > +}); Use defineLazyModuleGetter here nit: I usually prefer having imports at the top of a source file, btw, not really important.

Comment on attachment 572065 [details] [diff] [review] Use AsyncFormHistory.jsm - test/browser_sanitize-timespans.js Review of attachment 572065 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_sanitize-timespans.js @@ +1,5 @@ > // Bug 453440 - Test the timespan-based logic of the sanitizer code > var now_uSec = Date.now() * 1000; > +var hoursSinceMidnight = new Date().getHours(); > +var minutesSinceMidnight = hoursSinceMidnight * 60 + new Date().getMinutes(); > +var s; s is the less clear var I've seen recently :) gSanitizer maybe? @@ +42,4 @@ > // Should test cookies here, but nsICookieManager/nsICookieService > // doesn't let us fake creation times. bug 463127 > + > + setupFormHistory(); // calls test2 make this explicit by adding a callback argument to setupFormHistory @@ +432,5 @@ > + value : "1y", > + firstUsed : lastYear.valueOf() * 1000 > + }] > + ], { > + onSuccess: formHistorySanityCheck I suppose we may want to know if there is a failure, a timeout wouldn't be that easy to debug @@ +447,5 @@ > + ["4hour", "Checking for 4hour form history entry creation", true], > + ["4hour10minutes", "Checking for 4hour10minutes form history entry creation", true], > + ["today", "Checking for today form history entry creation", true], > + ["b4today", "Checking for b4today form history entry creation", true], > + ], test2); as well as, would be better if the callback would be an explicit argument

Comment on attachment 573623 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 573623 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/AsyncFormHistory.jsm @@ +51,5 @@ > +this.__defineGetter__("microNow", function() { > + return Date.now() * 1000; // microseconds > +}); > + > +AsyncFormHistory = { const or let, whatever, but you should declare it @@ +63,5 @@ > + QueryInterface : XPCOMUtils.generateQI([Ci.nsIObserver, > + Ci.nsIFrameMessageListener, > + ]), > + > + debug : true, I suppose this should be flipped before pushing? @@ +75,5 @@ > + "id" : "INTEGER PRIMARY KEY", > + "fieldname" : "TEXT NOT NULL", > + "value" : "TEXT NOT NULL", > + "timesUsed" : "INTEGER", > + "firstUsed" : "INTEGER", is firstUsed really that much important for autocomplete? (I really don't know and not sure I have the time to figure all the connections), could it be dropped? Is it maybe used by Sync? @@ +78,5 @@ > + "timesUsed" : "INTEGER", > + "firstUsed" : "INTEGER", > + "lastUsed" : "INTEGER", > + "guid" : "TEXT" > + }, slightly OT: likely this should not be a SQLite database, the structure is so simple. When building the new API and code consider this is likely going to be remade with levelDB (once we will get support and an async js API for it), that has simpler search capabilities. I suppose we should restrict searches to a couple fields, like fieldname and guid. Considering the datastore as multiple hashes should be the right vision. Right now search() seems to allow searching on any column, but is that really needed for the scopes of form autocomplete? Allowing any kind of searches is also a problem regarding indices, for example right now you allow searching on firstUsed but it has no index. @@ +92,5 @@ > + }, > + moz_formhistory_guid_index : { > + table : "moz_formhistory", > + columns : ["guid"] > + }, I'm not sure if we can touch the schema at this point, but this should rather be TEXT UNIQUE in the table definition, than an index here. it should be a UNIQUE index btw. @@ +96,5 @@ > + }, > + } > + }, > + dbConnection : null, // The database connection > + dbStmts : null, // Database statements for memoization should be an empty object, not null. @@ +108,5 @@ > + Services.console.logStringMessage("FormHistory: " + message); > + }, > + > + init : function() { > + Services.prefs.addObserver("browser.formfill.", this, false); this is probably now using weak references (as said by dolske, see the recent changes) @@ +111,5 @@ > + init : function() { > + Services.prefs.addObserver("browser.formfill.", this, false); > + > + this.updatePrefs(); > + this.dbStmts = {}; no need if you define it as an empty object @@ +119,5 @@ > + this.messageManager.loadFrameScript("chrome://satchel/content/formSubmitListener.js", true); > + this.messageManager.addMessageListener("FormHistory:FormSubmitEntries", this); > + > + // triggers needed service cleanup and db shutdown > + Services.obs.addObserver(this, "profile-before-change", false); this is probably now using weak references (as said by dolske, see the recent changes) @@ +202,5 @@ > + } > + return; > + default: > + throw "shouldn't be here"; > + } This is more complicated than it could be, though you may not want to fix that now, since involves deeper changes. It is up to you. The fact is that you could define a proper unique index on the table (I suppose on "fieldname, value") so that any duplicates addition will cause a conflict error, and manage the conflict error with an update (of useCount and lastUsed). Or you may even cheat with INSERT OR REPLACE, by using subqueries: INTERT OR REPLACE into moz_formhistory (id, fieldname, value, timesUsed, firstUsed, lastUsed, guid) VALUES ((SELECT id FROM moz_formhistory WHERE ...), :value, IFNULL((SELECT timesUsed FROM moz_formhistory WHERE ...), 1), ..., IFNULL((SELECT guid FROM moz_formhistory WHERE ...), :guid)) this would actually work as an INSERT OR UPDATE (that SQLite misses) and you would fire an atomic query @@ +206,5 @@ > + } > + }, > + > + _doAdd : function (aNewData, aCallbacks) { > + let guid = this.generateGUID(); I think Sync wants to be able to force a guid on addition rather than generate a new one, the guid should be the same across systems, so this looks wrong. @@ +215,5 @@ > + > + handleError: (aCallbacks && aCallbacks.onFailure) ? > + function(aError) { > + aCallbacks.onFailure(AsyncFormHistory.ADD, aNewData, aError); > + } : NOOP, I'd personally prefer seeing the opposite, so handleError: function (aError) { if (aCallbacks && aCallbacks.onFailure) aCallbacks.onFailure(AsyncFormHistory.ADD, aNewData, aError); } it looks much more readable to me... any reason to not just do so everywhere? Otherwise, you may define a prototype for an empty statementCallback and use Object.create to override just some property? Btw you should give some name to these anonymous functions. @@ +227,5 @@ > + }); > + }, > + > + _doUpdate : function (aId, aGuid, aCallbacks) { > + let now = Date.now() * 1000; // microseconds you added a microNow getter, didn't you? @@ +229,5 @@ > + > + _doUpdate : function (aId, aGuid, aCallbacks) { > + let now = Date.now() * 1000; // microseconds > + > + let query = "UPDATE moz_formhistory SET timesUsed = timesUsed + 1, lastUsed = :lastUsed WHERE id = :id"; What I said above, would kill all of this code. @@ +284,5 @@ > + > + remove : function (aSearchData, aCallbacks) { > + let stmt = this._makeRemoveStatement(aSearchData); > + stmt.executeAsync({ > + handleResult: NOOP, Just to answer Dolske, it's not Storage that can't handle a null property, it's xpconnect that complains in debug builds. @@ +342,5 @@ > + aSelectTerms = aSelectTerms.filter( > + function (field) { return !(field in aSearchData); }); > + > + if (aSelectTerms.length == 0) { > + throw "can't select nothing"; just to answer Dolske, SELECT count(1) is not faster than SELECT count(*), and btw count in SQLite is not a cheap call as one may expect (in the sense there is no place where a count is stored, so it has actually to count the rows). @@ +586,5 @@ > + this.log("Initializing Database"); > + > + let storage = Cc["@mozilla.org/storage/service;1"]. > + getService(Ci.mozIStorageService); > + this.dbConnection = storage.openDatabase(dbFile); openUnsharedDatabase @@ +641,5 @@ > + } > + > + // Upgrade to newer version... > + > + this.dbConnection.beginTransaction(); if you store all the needed creation statements in an array, you may just fire a conn.executeAsync(stmts) here, with an implicit transaction, would be less error-prone than using explicit transactions. And if everything is async makes sense to make creation async.

(In reply to Marco Bonardo [:mak] from comment #29) > > + "firstUsed" : "INTEGER", > > is firstUsed really that much important for autocomplete? (I really don't > know and not sure I have the time to figure all the connections), could it > be dropped? Is it maybe used by Sync? Not to my knowledge. We pretty much only care about last modified time.

Summary of Changes: - Renamed AsyncFormHistory -> FormHistory - Moved all non-API functions and members to the global component scope. Used object freezing on the FormHistory object. - Replaced (add, modify, remove) with single api call, 'updatePlaces' -- Because FormHistory now does the pre-processing on changes, we can ensure (at least through our API calls) that fieldname/value are a unique pair. -- Changes are represented by objects with set fields and optionally a modifier field, either 'bump' or 'remove'. --- The 'remove' modifier would remove all entries specified by the other fields (ANDed together). --- The 'bump' modifier either adds the (fieldname, value) entry to form history or updates the lastUsed time. This behavior used to be implicitly available through add and all it did was make the callers abstractly pretty. It would bork a lot of other expected behavior we would like from the API. --- Pseudo-fields allows the API to specify ranges in firstUsed and lastUsed via [ firstUsedStart, firstUsedEnd, lastUsedStart, lastUsedEnd ]. --- Getting rid of firstUsed sounds interesting... but I'm wondering if it's not a little out of scope here... - 'id' is no longer an accessible field. - All writes are prevented during private browsing mode. I think disabling remove during private browsing mode is also expected/consistent with current behavior considering that sanitize is disabled during PB. - Created function 'count' wrapping 'search' since people expect different callback parameters. - Pushes a lot of field validation before hitting anything async so that we can have sync exceptions. - Removed memoization. May re-implement this differently. When we executeAsync multiple statements, if we do caching then we have to be sure no two statements are actually trying to reference the same statement object. I considered cloning objects as necessary, but mozIAsyncStatements don't have a clone function... - Changed semantics of future dbMigration functions from executing statements to returning an array of async statements. If migration functions are modifying columns asynchronously, can we expect correct behavior of other API calls to block/queue other transactions or should this api take care of this somehow... - Added expireOldEntries and related observers. -- The vacuuming behaviour (copied form nsFormHistory.js) is weird. Consider if two expirations were performed, each deleting a large number of entries just below the vacuum threshold. Won't we accumulate a large amount of fragmentation? - Looking at new nsFormHistory bugs: -- Made observers use weak referencing. -- Database is initialized lazily. - Unless noted above, all the smaller comments are addressed by this bug.

@rnewman: The old api defined a whole lot of notifications through "satchel-storage-changed" and looking through the codebase it doesn't seem like that many were being used and none outside of sync. With the new Sync storage infrastructure stuff, what notifications will be needed?

(In reply to Felix Fung (:felix) from comment #31) > - Replaced (add, modify, remove) with single api call, 'updatePlaces' I suppose the suggestion was to have an API similar to updatePlaces (I can't find that suggestion atm), but I don't think it should be named the same, would be quite confusing... > - Removed memoization. May re-implement this differently. When we > executeAsync multiple statements, if we do caching then we have to be sure > no two statements are actually trying to reference the same statement > object. I considered cloning objects as necessary, but mozIAsyncStatements > don't have a clone function... statements on the same connection are serialized btw, they can't run concurrently. So I'm not sure what kind of issue are you trying to prevent, even if, I admit, I've not looked at the patch yet. What prevents you from reusing prepared statements? > -- The vacuuming behaviour (copied form nsFormHistory.js) is weird. Consider > if two expirations were performed, each deleting a large number of entries > just below the vacuum threshold. Won't we accumulate a large amount of > fragmentation? Why can't this just register with the VacuumManager component?

Additional Changes: - Used defineLazyModuleGetter - Update API call

@mak: To see why cached statements won't be as simple as they used to be, consider the following where neither of these fieldname value pairs exist. updatePlaces([ { fieldname : 'foo', value : 'bar' }, { fieldname : 'glad', value : 'lad' }] ...); We will create an INSERT statement for (foo, bar), call it STMTA. The SQL for (glad, lad) is the same as STMTA so naive caching would just reuse that statement before it's executed.. So we would end up overwriting STMTA.params and (foo, bar) never gets written to the DB.

well, for that you may try to use a BindingParamsArray, it's made so that you can bind multiple sets of params to the same prepared statement, the statement is internally cloned for each set of params. CookieService uses it if you want to take a look

(In reply to Felix Fung (:felix) from comment #32) > @rnewman: The old api defined a whole lot of notifications through > "satchel-storage-changed" and looking through the codebase it doesn't seem > like that many were being used and none outside of sync. With the new Sync > storage infrastructure stuff, what notifications will be needed? The idea is that we will only need notifications to decide when to sync, not to track data. For example, we'll kick off an instant sync (as we do today) when a bookmark or pref changes. We won't need to add the ID of changed items to a tracker. Instead, we'll be able to interrogate the storage component for items that have changed since some timestamp. That's why storing of guids and last modified times is so important: it's the foundation on which Sync will rest. As such, we'll want notifications of changes, removals, and additions, just as we do now; we just won't be misusing them quite so badly. I also very strongly encourage you to read and digest the comments here from Philipp and myself: https://gist.github.com/2cd878a9fc2dc78fff37#gistcomment-35092 We spent a fair amount of time opining on an async form history API; it would be a shame for that effort to be wasted. Thanks!

Additional Changes: - Removed 'removeAllEntries' call - Added statement memoization -- As a result, we can't guarantee the order of the changes in updatePlaces anymore...

(In reply to Marco Bonardo [:mak] from comment #33) > Why can't this just register with the VacuumManager component? I was looking for documentation on how to use VacuumManager. Besides one unit test... it's not quite clear how I would take advantage of the vacuum manager...

To use the VacuumManager, you need to make use of the mozIStorageVacuumParticipant interface and the magic vacuum-participant XPCOM category. The component manager tracks when services are defined to have categories, and interested parties ask for the list of services with a given category. In this case, the VacuumManager stores a list of vacuum-participant services.

Additional Changes: - Implemented mozIStorageVacuumParticipant - Renamed DBConnection -> databaseConnection (to match above interface) - Registered with VacuumManager

Comment on attachment 577540 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 577540 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/FormHistory.jsm @@ +59,5 @@ > +/** > + * Service state > + */ > + > +var debug = true; you use part var, part let in the same scope, just stick with let @@ +69,5 @@ > + return; > + } > + dump("FormHistory: " + message + "\n"); > + Services.console.logStringMessage("FormHistory: " + message); > +} nit: would be more compact as an if (debug) { @@ +81,5 @@ > +/** > + * Current database schema > + */ > + > +var dbSchema = { looks more like a const @@ +111,5 @@ > +}; > + > +/** > + * Wait... I'm copying things now... so I probably don't need this j0nx. > + */ ?? @@ +132,5 @@ > + 'firstUsedStart', > + 'firstUsedEnd', > + 'lastUsedStart', > + 'lastUsedEnd' > +]; all of these should be const, use trailing commas @@ +186,5 @@ > + * Storage statement creation and parameter binding > + */ > + > +function makeCountStatement(aSearchData) { > + let query = "SELECT COUNT(1) AS numEntries FROM moz_formhistory"; count(1) is not faster than the more usual count(*) @@ +364,5 @@ > + // version 3. > + if (version < 3 && !FormHistory.databaseConnection.tableExists("moz_formhistory")) > + dbCreate(); > + else if (version != DB_SCHEMA_VERSION) > + dbMigrate(version); nit: please brace these @@ +545,5 @@ > + } > +}; > + > +/** > + * updatePlacesWrite global: please don't use the word Places here, would be confusing, updateFormHistory may be fine @@ +581,5 @@ > + } > + > + for each (let stmt in stmts) { > + stmt.bindParameters(bindingArrays[stmt]); > + } Why don't you do this directly in the makeXXXStatement then? would not be simpler? @@ +632,5 @@ > + }, > + onFailure : function expireDeletionFailure(aError) { > + log("expireDeletionFailure"); > + throw Components.Exception(aError.message, > + Cr.NS_ERROR_UNEXPECTED); who is going to catch this since it's async? @@ +672,5 @@ > + > + aSearchData = validatePseudoData(aSearchData, "Search"); > + let stmt = makeSearchStatement(aSearchData, aSelectTerms); > + > + let results = []; may be a property of the handlers object @@ +717,5 @@ > + + "receive results.", > + Cr.NS_ERROR_ILLEGAL_VALUE); > + } > + > + let resultCount = null; you may have _count as a property of the handlers object ::: toolkit/components/satchel/satchel.manifest @@ +1,5 @@ > component {0c1bb408-71a2-403f-854a-3a0659829ded} nsFormHistory.js > contract @mozilla.org/satchel/form-history;1 {0c1bb408-71a2-403f-854a-3a0659829ded} > +component {e7637011-c7c7-4734-98c3-9da006d720be} FormHistory.jsm > +contract @mozilla.org/satchel/async-form-history;1 {e7637011-c7c7-4734-98c3-9da006d720be} > +category vacuum-participant FormHistory @mozilla.org/satchel/async-form-history;1 hm, well, looks like I am dumb, sorry. You can't register with the vacuum manager because this is not an xpcom component, you can't register a module this way. The point of categories (like vacuum-participant) is that they should be able to init a component on request, but your module is not an xpcom component with its factory. You should build a dedicated xpcom component just for this registration and import the module into it to forward calls, but I'm not sure it's worth it at this point. Even better, we may create a Storage component that is always registered and you just register objects with it, so that it forwards calls, but that's out of the scope of this bug, you may have to go back :(

Comment on attachment 577540 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 577540 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/FormHistory.jsm @@ +132,5 @@ > + 'firstUsedStart', > + 'firstUsedEnd', > + 'lastUsedStart', > + 'lastUsedEnd' > +]; I'm bikeshedding (sorry) but I don't like trailing commas and we haven't used them in satchel JS in the past. It looks like they're tolerated in the spec, I just know they've caused me problems in the past (probably in IE). It's a new file though and Marco has been rocking the reviews here, so I'll defer to him and we'll remain consistent in this file (just wanted to add my 2 cents).

I tried to use splinter review to do a fancy reply, but it didn't work. If it wasn't obvious, I was referring to Marco's comment about using trailing commas.

Surely IE had its fancy implementation, adding an undefined entry at the end of the array, but Ecmascript 5 covers the lack of spec, by stating they are allowed and should _not_ cause an undefined entry. FWIW, IE8 started doing the right thing (also related to trailing commas in objects). I mostly suggest using them to preserve blame (my alternative is to use prefix commas, but it looks strange to many, so I don't feel like suggesting it), but I would not block anything on such a thing. If people doesn't like them it's free to not use them.

Additional Changes: - Reverted VacuumManager changes - Addressed comments above @mak: I bindParameters after making the statements because after calling makeXXXStatement, and constructing a bindingArray, a later statement may modify that same bindingArray (adding more params) and we won't want to call bindParameters until after the bindingArray is completed. Or at least that's how I understand it.

@rnewman: For the notifications of add/modify/remove, how much information is needed in those notifications? Because the API is now more lax about its inputs, we don't necessarily have all of the same identifying information on modified entries. For add and modify, guids are available. On remove however, the API can remove on almost any property...

(In reply to Marco Bonardo [:mak] from comment #45) > Surely IE had its fancy implementation, adding an undefined entry at the end > of the array, but Ecmascript 5 covers the lack of spec, by stating they are > allowed and should _not_ cause an undefined entry. FWIW, IE8 started doing > the right thing (also related to trailing commas in objects). > I mostly suggest using them to preserve blame (my alternative is to use > prefix commas, but it looks strange to many, so I don't feel like suggesting > it), but I would not block anything on such a thing. If people doesn't like > them it's free to not use them. Ah another reason, trailing commas throw syntax errors in JSON.parse (though that's not an issue here, but it's another of those gotchas). Again, these are my personal preferences coming out, but I don't care about messing up blame like this (and I actually find prefix commas terribly ugly, so I don't really have any options for maintaining blame!) I wouldn't block either, that's why I'm deferring to you here since you've been reviewing. Since it appears neither of us is going to step on each others toes, Felix can decide :) (or if Dolske comes in with a review hammer, I guess he can decide). </bikeshedding>

I'm fine sticking with whatever Felix or Dolske prefer, I won't surely block on these things :) Btw I think the json case doesn't apply, JSON.stringify is standard compliant and thus handles trailing commas properly. JSON.parse gets a string input, you can do much worse damage than trailing commas if you build the string manually ;) That said, I can surely see anyone may have his own preferences, and there is probably none that is absolutely better.

Additional Changes: - Added notifications - Removed a few async throws that wouldn't be caught

Additional Changes: - Bugfix: notifications need nsISupport data - Bugfix: count

Summary of Changes: - Removed the expiration code from nsFormHistory.js which would conflict with expiry code contained in AsyncFormHistory.jsm

Summary of Changes: - Converted old formhistory_expire.sqlite to SCHEMA_VERSION=3 so that AsyncFormHistory.jsm would register it. - Converted test_expire.js to test_async_expire.js Note: The converted files are removed in a separate patch.

Comment on attachment 579525 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 579525 [details] [diff] [review]: ----------------------------------------------------------------- Looking nice! I want to trace through updateFormHistory() again, but only have a few comments otherwise. Will try to wrap up this last bit tonight or first thing tomorrow. ::: toolkit/components/satchel/FormHistory.jsm @@ +49,5 @@ > + "nsIUUIDGenerator"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "_privBrowsingSvc", > + "@mozilla.org/privatebrowsing;1", > + "nsIPrivateBrowsingService"); I believe this still needs the funky "If the service is not available, null will be returned." handling as in nsFormHistory.js. Because other apps (SeaMonkey, Thunderbird) might use form history but not have a private browsing service. *grumble* We should just add this to Services.jsm to avoid this madness. And/or add a dummy PBS to toolkit... Let me file a bug to just do that. @@ +77,5 @@ > + strWrapper.data = aString; > + sendNotification(aType, strWrapper); > +} > + > +function sendIntNotification(aType, aInt) { This used to send two ints, the start and end times for a range of data to clear... Clear Recent History lets you clear the the most recent N minutes, so how's that going to work? Looks like all the expiration code is assuming callers will only be expiring things older than a certain date. @@ +85,5 @@ > + sendNotification(aType, intWrapper); > +} > + > +function sendNotification(aType, aData) { > + Services.obs.notifyObservers(aData, "satchel-storage-changed", aType); Can you (briefly) describe (in a separate comment in this bug) all the notifications we'll be sending now and what their string/int data contains? This is for the benefit of out doc writers and updating devmo, so they've got a reference list to go from. @@ +107,5 @@ > + "value" : "TEXT NOT NULL", > + "timesUsed" : "INTEGER", > + "firstUsed" : "INTEGER", > + "lastUsed" : "INTEGER", > + "guid" : "TEXT" Since it came up, I am the trailing comma's BIGGEST FAN EVER, and am disappointed there is not one here. Actually I don't feel strongly either way, but I generally do like them. :) @@ +155,5 @@ > +function validateData(aData, aDataType) { > + let checkedData = {}; > + for (let field in aData) { > + if (validFields.indexOf(field) != -1 > + || modifierFlags.indexOf(field) != -1) { Style nit (sorry): operators at the end of line, not beginning... |foo &&\nbar| not |foo\n&& bar|. Though here this should fit on 1 line? I don't care too much about the "80 column max" thing, I think /js standardized on 100. I'd rather have an occasional long line if the alternative is awkwardly breaking it up. @@ +171,5 @@ > + let checkedData = {}; > + for (let field in aData) { > + if (validFields.indexOf(field) != -1 > + || modifierFlags.indexOf(field) != -1 > + || pseudoSelectors.indexOf(field) != -1) { Same nit. @@ +216,5 @@ > + // if no terms selected, select everything > + aSelectTerms = (aSelectTerms) ? aSelectTerms : validFields; > + // no need to fetch any field whose data is already provided > + aSelectTerms = aSelectTerms.filter( > + function(field) { return !(field in aSearchData); }); Is this optimization (?) really worth the complexity? Seems like the DB already has to access the data to check it, and so returning it in the result should be quite cheap? (i.e., instead of reconstructing known fields, just ask for them from the DB). @@ +307,5 @@ > + */ > + > +let dbConnection = null; > +let dbStmts = {}; > +let messageManager = null; This can be local to init(), it's not used elsewhere. @@ +316,5 @@ > + * Creates a statement, wraps it, and then does parameter replacement > + * Will use memoization so that statements can be reused. > + */ > +function dbCreateAsyncStatement(aQuery, aParams, aBatchingStmts, > + aBindingArrays) { This is why I don't care much about column limits. :) @@ +362,5 @@ > + log("Open Database"); > + > + let storage = Cc["@mozilla.org/storage/service;1"]. > + getService(Ci.mozIStorageService); > + return storage.openUnsharedDatabase(dbFile); nit, while you're here: Services.storage.* @@ +379,5 @@ > + // nsIFormHistory2 supported migrations from Firefox 3 onwards but when > + // FormHistory is released, we're 'end of life'ing everything before > + // version 3. > + if (version < 3 || !FormHistory.DBConnection.tableExists("moz_formhistory")) { > + dbCreate(); If version < 3, you should treat the DB as corrupt (because we can no longer migrate it)... So you'll be needing to call dbCleanup, opening a new connection, and initing it. Or, hmm, maybe you can just throw here, and the DBConnection getter will take care of all that. Yeah. Make sure there's a test that covers this case? @@ +449,5 @@ > + } > + } > + }); > + > + log("DB migration completed."); Nit, add log()s to the async callbacks to report what happens. @@ +459,5 @@ > + > +// WARNING: Newer dbMigrateToVersionXXX functions should return an > +// ordered list of mozIStorageAsyncStatements (e.g. through > +// dbCreateAsyncStatement) that modify and update the database rather than > +// executing them from the function. Hmm, that won't be possible in some cases. We'll probably need to chain together version-to-version callbacks, and suppress other access to the DB until migration finishes. But I guess we'll deal with that the next time we bump the schema! @@ +499,5 @@ > + // Create backup file > + let dbFile; > + if (backup) { > + let storage = Cc["@mozilla.org/storage/service;1"]. > + getService(Ci.mozIStorageService); nit, while you're here: Services.storage.* @@ +911,5 @@ > + updateFormHistoryWrite(checkedChanges, aCallbacks); > + } > + }, > + > + get DBConnection() { I like that the nsFormHistory.js code does a |delete| on the getter, so that if we accidentally refer to ourselves while initializing the connection we don't get stuck in a loop. I guess you can't do that here because the object is frozen. But I think you can make FormHistory.DBconnection just be a simple getter, which in turns calls a self-memoizing getter in the global scope. This would also allow the global functions to just use |dbConnection| instead of |FormHistory.DBConnection|, which is a little verbose and odd. (You could even just make FormHistory a tiny object where each function is a trivial wrapper to functions in the global scope... Fine either way.) @@ +926,5 @@ > + > + dbConnection = dbOpen(dbFile); > + dbInit(); > + } catch (e) { > + log("Initialization failed: " + e); Hmm, observation. Existing quirk: if dbInit() fails twice we end up a valid connection to an uninitialized DB (and never try again). Not sure there's any more we can do, other than constantly retrying. Which is bad, come to think of it, since that makes performance go to shit! I vaguely recall an old bug like this in pwmgr where someone was hitting a case like this. (I think you're fine as-is, no need to address this weird case.)

Additional Changes: - Fixed some of the nits - Fixed bug in search - Added param doCreate to dbInit. dbInit will throw the first time around so that the backup gets taken care of. It will call dbCreate the second time around. This is necessary otherwise we end up just throwing and never creating. We could just call dbCreate instead of calling dbInit a second time if you feel that's cleaner... Notes: - I think it's cleaner to keep the DBConnection getter the way it is. The way the initialization code works, we need to create dbConnection and THEN call dbInit which references dbConnection. Doing it otherwise would probably require us to introduce another intermediate variable like _dbConnection. - Clear Recent History doesn't use the expire path. It calls updateFormHistory directly with a removal flag. Similarly, nsIFormHistory2 called removeAllEntries/removeEntriesByTimeframe. The old notification doesn't work for removals because new API is more freeform when removing things. We can remove based on any conjunction of fields and so I'm just sending a "RemoveEntry" notification with no more information (which sucks). I couldn't find any old usage of the remove notifications and so I have no context in deciding what data _needs_ to get sent back. It exists as a placeholder atm... - If the statements produced by the migration functions are executed in a serial order all together in a single transaction, why won't that suffice for future schema changes? (Nothing that yes, I'd have to make a small change to make them execute serially...)

Can you merge some of the fixes that went into m-c? In particular, the argument to dbCleanup is always true. You might also like to get https://bugzilla.mozilla.org/show_bug.cgi?id=702848 so that clients can receive a notification that the connection will be closed. The comment about "already closed" is probably bogus, see https://bugzilla.mozilla.org/show_bug.cgi?id=696486. Now that we are using asyncClose, you probably need to spin the loop before you can remove the file. You can put the remove in a callback, but you would still need to spin it in DBConnection.

Comment on attachment 577356 [details] [diff] [review] Use AsyncFormHistory.jsm - sanitize.js Review of attachment 577356 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +276,5 @@ > searchBar.textbox.reset(); > } > > + if (this.range) { > + FormHistory.updatePlaces([{ I think this changed, so the patch should be updated.

Comment on attachment 579526 [details] [diff] [review] Remove Form History Expiration Code from nsFormHistory.js Review of attachment 579526 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I don't feel reviewing this removal, dolske knows interconnections of this old code much better than me.

Comment on attachment 579529 [details] [diff] [review] Testing Expiration Code in AsyncFormHistory.jsm Review of attachment 579529 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/test/unit/test_async_expire.js @@ +13,5 @@ > + * > + * The Original Code is Satchel Test Code. > + * > + * The Initial Developer of the Original Code is > + * Mozilla Corporation. nope, "the Mozilla Foundation." @@ +14,5 @@ > + * The Original Code is Satchel Test Code. > + * > + * The Initial Developer of the Original Code is > + * Mozilla Corporation. > + * Portions created by the Initial Developer are Copyright (C) 2009 I don't think so @@ +18,5 @@ > + * Portions created by the Initial Developer are Copyright (C) 2009 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): > + * Justin Dolske <dolske@mozilla.com> (Original Author) maybe not? @@ +49,5 @@ > + * Sync statements to simplify testing > + */ > + > +function countAllEntries() { > + let stmt = FormHistory.DBConnection.createStatement("SELECT COUNT(*) as numEntries FROM moz_formhistory"); global-nit: uppercase AS @@ +53,5 @@ > + let stmt = FormHistory.DBConnection.createStatement("SELECT COUNT(*) as numEntries FROM moz_formhistory"); > + do_check_true(stmt.executeStep()); > + let numEntries = stmt.row.numEntries; > + stmt.finalize(); > + return numEntries; you can avoid the temp var by using a try {executeStep;return;} finally {finalize;} @@ +104,5 @@ > + go: function go1() { > + // Expire history with default pref (180 days) > + do_check_true(entryExists("name-A", "value-A")); > + do_check_true(entryExists("181DaysOld", "foo")); > + do_check_true(entryExists("179DaysOld", "foo")); I hope the internal code considers DST changes, since if this runs when there is 1 day of difference due to DST it may fail... I'd probably suggest to test at 2 days distance, since I've seen too many DST timebombs in the past, or be ready with your sword at the hours surrounding next changes in PDT... ditto for the other tests @@ +192,5 @@ > + tests[currentTestIndex].go(); > + } > + } > + } > +}; fwiw, you could use the add_test(), run_next_test() method, it should be cleaner than manually going through tests, you could avoid desc by properly naming functions, go would be the main function while triggerExpiration may earn a callback for the check code and then invoke run_next_test(). @@ +207,5 @@ > + Services.obs.addObserver(TestObserver, "satchel-storage-changed", true); > + > + // ===== test init ===== > + var testfile = do_get_file("asyncformhistory_expire.sqlite"); > + var profileDir = dirSvc.get("ProfD", Ci.nsIFile); do_get_profile() @@ +234,5 @@ > + do_check_true(entryExists("name-C", "value-C")); > + > + // Check the original db size. > + // Do a vacuum to make sure the db has current page size. > + FormHistory.DBConnection.executeSimpleSQL("VACUUM"); the comment may say why it's important to have the current page size. I assume it's for proper size comparison that you do later @@ +251,5 @@ > + FormHistory.DBConnection.executeSimpleSQL("UPDATE moz_formhistory SET lastUsed=" + age179 + " WHERE lastUsed=179"); > + FormHistory.DBConnection.executeSimpleSQL("UPDATE moz_formhistory SET lastUsed=" + age31 + " WHERE lastUsed=31"); > + FormHistory.DBConnection.executeSimpleSQL("UPDATE moz_formhistory SET lastUsed=" + age29 + " WHERE lastUsed=29"); > + FormHistory.DBConnection.executeSimpleSQL("UPDATE moz_formhistory SET lastUsed=" + age11 + " WHERE lastUsed=9999"); > + FormHistory.DBConnection.executeSimpleSQL("UPDATE moz_formhistory SET lastUsed=" + age9 + " WHERE lastUsed=9"); looks like you may want an helper that gets the hours delay as argument? ::: toolkit/components/satchel/test/unit/xpcshell.ini @@ +16,5 @@ > [test_db_update_v999a.js] > [test_db_update_v999b.js] > [test_history_api.js] > [test_notify.js] > +[test_async_expire.js] I think the tests are in alphabetical order, so may be nice to preserve it

Comment on attachment 581219 [details] [diff] [review] AsyncFormHistory.jsm Review of attachment 581219 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/FormHistory.jsm @@ +256,5 @@ > + let queryTerms = makeQueryTerms(aSearchData); > + > + if (queryTerms.length == 0) { > + // remove all entries > + log("removeAllEntries"); Nit: I'd flip these lines, and make the comment a short note about not needing to modify the query. Just so anyone skimming this doesn't think this log-only block is missing something. @@ +309,5 @@ > +/* > + * dbCreateAsyncStatement > + * > + * Creates a statement, wraps it, and then does parameter replacement > + * Will use memoization so that statements can be reused. Hmm. We should remove the memoization. It's fine to do with a fixed-function API, to avoid creating the same handful of statements over and over. (In fact, iirc, the oooold code literally just created all the statements it would need in init()!] But with a more free-form API like this patch is adding, the number of cached (memoized) statements could be unbounded. I suppose we want to keep this, though, for the few statements we'd otherwise create frequently. [mak: is this still a perf concern?] Maybe the easiest fix would be to add a |cachable| flag here, and have makeXXXStatement() only set cachable to true for the simple querys used a lot? @@ +369,5 @@ > + let version = FormHistory.DBConnection.schemaVersion; > + > + // When FormHistory is released, we will no longer support the various schema versions prior to > + // this release that nsIFormHistory2 once did. > + if (version < 3 || !FormHistory.DBConnection.tableExists("moz_formhistory")) { This seems needlessly complicated with the |doCreate| flag. The effective behavior is the same in either case -- this function throws, and the caller has to clean up. [Boolean args are nice to avoid when possible, too, since you can't tell from the callsite what foo(true) or foo(false) means.] So just: dbInit() { if (!FormHistory.DBConnection.tableExists("moz_formhistory")) { dbCreate() return; } version = FormHistory.DBConnection.schemaVersion; if (version < 3) barf("too old! ᶘ ᵒᴥᵒᶅ"); else if (version != DB_SCHEMA_VERSION) dbMigrate(version); } @@ +828,5 @@ > + let checkedChanges = []; > + let modifiedKeys = {}; > + > + for each (let change in aChanges) { > + if (change.remove) { It's a bit unclear to have multiple operations branching through and if/else sequence (ie, operation being based on some implicit state of things), and the API is harder to document. This feels like it wants to be: switch(change.op) { ... } Where |op| is "remove", "bump", "add", "modify". ("modifyOrAdd" so the Sync case?). Ditto for updateFormHistoryWrite(). @@ +866,5 @@ > + let key = [ change.fieldname, change.value ]; > + if (modifiedKeys[key]) { > + throw Components.Exception( > + "FormHistory cannot modify the same entry more than once in a single " > + + "update: (" + change.fieldname + ", " + change.value + ")", Is this just a sanity check? What's the risk of allowing it? Because one can have multiple changes with the same GUID, which seems effectively the same. Should we block that too? There's also the tricky case of 1 operation specifying a GUID and 1 operation specifying the name/value, even though they refer to the same entry. Unless something terrible results from multiple operations, I'd rather not have to detect this. :) @@ +880,5 @@ > + }, { > + onSuccess : function updateFormHistoryPreprocessSuccess(aResults) { > + if (aResults.length == 1) { > + change.guid = aResults[0]["guid"]; > + } Explicit operation states would make this code clearer, because if .length == 0 the AddOrModify must be a Add and updateFormHistoryWrite only gets an Add or a Modify, and never an AddOrModify. Probably want some logging here for if .length > 1, since we have multiple entries with the same GUID and that's bad. :( nsFormHistory.js seems to just assume the 1st result is the GUID it wants to use. I wonder if it's better to skip the operation... Meh?

(In reply to Marco Bonardo [:mak] from comment #59) > > + // Expire history with default pref (180 days) > > + do_check_true(entryExists("name-A", "value-A")); > > + do_check_true(entryExists("181DaysOld", "foo")); > > + do_check_true(entryExists("179DaysOld", "foo")); > > I hope the internal code considers DST changes, since if this runs when > there is 1 day of difference due to DST it may fail... > I'd probably suggest to test at 2 days distance, since I've seen too many > DST timebombs in the past, or be ready with your sword at the hours > surrounding next changes in PDT... ditto for the other tests Shouldn't be any need to. All the time stuff is based on unix epoch, so DST and timezones are irrelevant.

(In reply to Felix Fung (:felix) from comment #55) > Notes: > - I think it's cleaner to keep the DBConnection getter the way it is. The > way the initialization code works, we need to create dbConnection and THEN > call dbInit which references dbConnection. Doing it otherwise would probably > require us to introduce another intermediate variable like _dbConnection. That should be fine, it probably just ends up being the intermediate storage (backend, really ;) that getters commonly have. But I think it's especially useful to help prevent reentrancy by someone adding new code in the init path that might access the connection, without realizing it's being set up. > - Clear Recent History doesn't use the expire path. It calls > updateFormHistory directly with a removal flag. Similarly, nsIFormHistory2 > called removeAllEntries/removeEntriesByTimeframe. Ooooooooooh. derp. > I couldn't find any old usage of the remove > notifications and so I have no context in deciding what data _needs_ to get > sent back. It exists as a placeholder atm... IIRC, the intention was that Sync would be able to use this to purge old entries itself via 1 operation, instead of carrying around a big pile of individual remove-entry operations. Not sure if that ever got implemented, or if it's actually useful in practice. Sync guys? > - If the statements produced by the migration functions are executed in a > serial order all together in a single transaction, why won't that suffice > for future schema changes? (Nothing that yes, I'd have to make a small > change to make them execute serially...) It's for the case where a migrator function can't make decisions on what to do until a previous migration has finished, or needs intermediate results. EG, dbMigrateToVersion3() wants to compute and add a GUID to any rows which don't already have a GUID.

Comment on attachment 572065 [details] [diff] [review] Use AsyncFormHistory.jsm - test/browser_sanitize-timespans.js Review of attachment 572065 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, other than mak's earlier review comments. r+ with that.

Comment on attachment 577356 [details] [diff] [review] Use AsyncFormHistory.jsm - sanitize.js Review of attachment 577356 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/sanitize.js @@ +40,5 @@ > # ***** END LICENSE BLOCK ***** > > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "FormHistory", > + "resource://gre/modules/FormHistory.jsm"); Hmm. Wonder if it would be worth adding a Services.FormHistory? Not for this bug. :)

Comment on attachment 579529 [details] [diff] [review] Testing Expiration Code in AsyncFormHistory.jsm Review of attachment 579529 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine. Mak also had some previous comments here, I think the only one that must be done is fixing the license header.

Comment on attachment 579526 [details] [diff] [review] Remove Form History Expiration Code from nsFormHistory.js Review of attachment 579526 [details] [diff] [review]: ----------------------------------------------------------------- I think we should just plan on removing nsFormHistory.js completely in one pass. From a quick chat, it sounds like you were just thinking we'd need to keep it for addon compat. I think we're fine to make a wholesale switch. Though the addons mxr does show ~60ish addon hits that reference "nsiformhist" in one way or another. We should make sure our Addons folk know about this, but otherwise I don't think there's a way around it. (Well, we _could_ write a somewhat backwards-compatible wrapper for the new code, but it would be inherently wonky because it's trying to implement an synchronous API using the new async API). And we've changed far bigger things without doing this. Test changes look fine, although I'd suggest just the test_expire_async.js from your other patch to be the new test_expire.js.

(In reply to Justin Dolske [:Dolske] from comment #60) > I suppose we want to keep this, though, for the few statements we'd > otherwise create frequently. [mak: is this still a perf concern?] It depends on a per-case basis. If you have hot paths where the same statement is executed multiple times may be a concern. Otherwise if your apis are seldom called, since you are just saving a prepare call (that in practice is SQLite compiling your statement in its internal representation), may be acceptable to not cache them. If this is used by Sync it may matter since it has to read or modify a lot of entries. Sdwilsh suggested (and I agree) we should have a StatementCache.jsm module that works similarly to the StatementCache native class, to simplify handling those.

Additional Changes: - updateFormHistory may explicitly call aCallbacks.onFailure if it encounters a bad state. (e.g. If the database contains multiple fieldname/value pairs.) - Removed modifierFlags in favour of the 'op' field in changes. - Simplified dbInit as per comments above - Removed modifiedKeys sanity check as per comments. Storage may just end up choking if given really bad data and we'll end up calling onFailure. Notes: - Did not touch the memoization stuff yet... - Still need to address comment 56

This part still looks problematic: 495 // FELIX 496 dbClose(); 497 dbFile.remove(false); dbClose uses asyncClose, but we immediately delete the file.

Like it says on the tin!

Additional Changes: - Made dbConnection a lazy getter. _dbConnection the intermediate variable. DBConnection just returns dbConnection. - Gave dbClose a blocking option. If blocking=true, then it will only return once the database connection has closed. dbClose is called blockingly during dbCleanup so that we can safely remove the file afterwards. - Added closeCallbacks which are called on dbClose a la comment 56

Additional Changes: - Updated caller to match API updates - In get canClear() for form history, make a blocking call to FormHistory.count.

Turns out, our migrators are written in C++ and use form-history. Is that a good enough reason to add some XPCOM?

(In reply to Felix Fung (:felix) from comment #73) > Turns out, our migrators are written in C++ and use form-history. Is that a > good enough reason to add some XPCOM? all the migrators are being rewritten in js, so I'd not care much... we can speed up that process in case (Mano is working on Safari, Opera has been removed, Felipe is working on IE that is likely the most hit).

How valuable is it that we disable clearing form history when there are no entries? If above comments are true, counting is expensive and locks up the main-thread...

Comment on attachment 582423 [details] [diff] [review] Use Asynchronous FormHistory.jsm for test_db_corrupt.js Review of attachment 582423 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/test/unit/test_db_corrupt.js @@ -73,5 @@ > - //// ===== 2 ===== > - //testnum++; > - //// File should be empty > - //do_check_false(fh.hasEntries); > - //do_check_false(fh.entryExists("name-A", "value-A")); Ehrm, these are not commented out in mozilla-central. I'm assuming this patch just happens to be against a temporary revision you had while getting tests working? @@ +124,2 @@ > > +add_test(function D() { D?! D: A better name would be nice. :)

Comment on attachment 582505 [details] [diff] [review] Use Asynchronous FormHistory.jsm for sanitize.js Review of attachment 582505 [details] [diff] [review]: ----------------------------------------------------------------- Imma mark this r+, but need to ponder the implications of having processNextEvent() in more places and see what others think. ::: browser/base/content/sanitize.js @@ +317,5 @@ > + var thread = Components.classes["@mozilla.org/thread-manager;1"] > + .getService().currentThread; > + while (blockingCount.numEntries == -1) { > + thread.processNextEvent(true); > + } Oh no, I was afraid this would happen. Yuck. It's really not good to spin the event loop, but we might have to as a temporary step until we change callers to deal with asyncness. [I suppose we could instead have a "num entries" counter that's tracked separately (ie, no SQL needed), but ick if that ever got out of sync.] Nit: I'd prefer to have a separate flag to signal that the operation finished (eg, consider if some bug resulted in aNumEntries == -1). let done = false; ... onSuccess/onFailure set done = true; while (!done) { t.processNextEvent(true); }

Comment on attachment 583757 [details] [diff] [review] (WIP) Use Asynchronous FormHistory.jsm for content/search.xml Review of attachment 583757 [details] [diff] [review]: ----------------------------------------------------------------- You shouldn't assume that FormHistory is defined in the current scope (or are you? I don't see anything defining it?). And again "hmm" on processNextEvent(). But looks fine from a skim as a WIP.

Comment on attachment 584088 [details] [diff] [review] Remove old migration tests Review of attachment 584088 [details] [diff] [review]: ----------------------------------------------------------------- You'll need to edit toolkit/components/satchel/test/unit/xpcshell.ini as well. r+ with that.

(In reply to Justin Dolske [:Dolske] from comment #78) > > + var thread = Components.classes["@mozilla.org/thread-manager;1"] > > + .getService().currentThread; > > + while (blockingCount.numEntries == -1) { > > + thread.processNextEvent(true); > > + } > > Oh no, I was afraid this would happen. Yuck. It's really not good to spin > the event loop, but we might have to as a temporary step until we change > callers to deal with asyncness. :((( Sadly it seems like you're right, dolske. We'll need to rewrite the Sanitizer to be async. While we might not want to block this bug on that, it should not be put on hold. Event loop spinning is not a pattern we want to establish at all. So my strong recommendation here is that at the very least this gets a big fat TODO comment referencing a bug that deals with this issue in a timely fashion. Also, ahem, please use the Services.jsm module wherever possible. In this case you can just use Services.tm.currentThread.

Summary of Changes: - Wrote function which takes any FormHistory api function, and makes it synchronous by spinning the event loop (call_blocking) - Transliterated test_db_update_v999a.js and test_db_update_v999b.js

Changes: - Used blocking_call, simplified the patch alot

Comment on attachment 585181 [details] [diff] [review] Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js Drive-by from the spinning event loop police :p >+function blocking_call() { Not to sound like a broken record, but please add a big fat comment here that this ghastly function must go away, but why it has to be here for now. Please refer to the bug in which it shall be removed. (If this bug doesn't exist yet, please file it.) Thanks! (Nit: It is unclear how to use blocking_call(). Maybe not super important since this is "just" a test, but it's pretty easy to just use a self-documenting function signature, e.g. function blocking_call(func, arg1, arg2, etc) { ... } ) Another drive-by: >+ let argsArray = [].slice.apply(arguments); In cross-browser JS I think the preferred spelling would be Array.prototype.slice.call(arguments) though in SpiderMonkey JS you can simply do Array.slice(arguments) which is much shorter and reads nicer. I recommend using that.

(In reply to Philipp von Weitershausen [:philikon] from comment #84) > Comment on attachment 585181 [details] [diff] [review] > Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js > > Drive-by from the spinning event loop police :p > > >+function blocking_call() { > > Not to sound like a broken record, but please add a big fat comment here > that this ghastly function must go away, but why it has to be here for now. I don't see a huge motivation to move away from spinning the event loop in test code. Since we really aren't concerned with performance issues in those cases, blocking_call really keeps the tests simple and straightforward. So I don't feel as if we should say something as strong as: it *must* go away.

(In reply to Felix Fung (:felix) from comment #85) > > Drive-by from the spinning event loop police :p *shows badge at the door* > > >+function blocking_call() { > > > > Not to sound like a broken record, but please add a big fat comment here > > that this ghastly function must go away, but why it has to be here for now. I'd also prefer it to be called callSpinningly, or spinningCall. Firstly to avoid mixing_case_conventions, but also this is *not* a blocking call. (Reminds me of the equivalent helper in Sync which used to be called "queryAsync", when of course it was quite the opposite!) > I don't see a huge motivation to move away from spinning the event loop in > test code. 1. Self-respect. 2. People copy test code when they come to find out how a module works. We follow our coding standard in test code, too, for similar reasons. 3. Spinning the event loop can cause crashes if it happens in certain situations. Having test code crash is annoying. 4. You're not really testing the API as it would be used in real code. 5. Proper async tests aren't difficult. add_test/run_next_test(). We've converted most of Sync's tests to be async, and it usually made them neater, self-describing, and also fixed some random oranges!

Comment on attachment 582465 [details] [diff] [review] Asynchronous FormHistory.jsm Blargh, I suck at timely reviews. Moving over to zpao to show me how it's done. Godspeed! :) I think from previous passes that I didn't have any significant issues with the code, so I suspect Paul will only need to focus on correctness kinds of issues. But his call. Oh, and as for the processNextEvent() stuff in the tests... We shouldn't do that. Even though it's just a test, PNE() can introduce all kinda of subtle issues. And so it should be strongly avoided where possible. Best thing to do it probably to refactor the test(s) a bit so that they're either event-driven, or use setInterval() to poll for an expected state. [setInterval pollings isn't the greatest pattern in the world either, but it's much less potentially-evil and already is used in a number of tests.]

Comment on attachment 582465 [details] [diff] [review] Asynchronous FormHistory.jsm Review of attachment 582465 [details] [diff] [review]: ----------------------------------------------------------------- First off, it's close. I mostly had nits. I haven't gotten the chance to say "thanks" yet, so thanks you for this and for keeping up after the intership is over! So it looks like it's possible to access FormHistory.DBConnection and have it be in the process of migrating (since those statements are async, but dbInit doesn't wait for that to complete. I'm not sure on the details, but could a consumer could execute a sync statement and be bad here (Mak, what do you think?) I still want to remove DBConnection from the the API because that just leads to complications & the possibility of some consumer to screw things up. If we're changing the world anyway, now is probably the best time to just do it. Sync currently uses it, but that can be fixed :) Dolske - we've talked about this before and I think you went back & forth on it, so I'll leave it up to you... ::: toolkit/components/satchel/FormHistory.jsm @@ +76,5 @@ > +} > + > +function sendStringNotification(aType, aString) { > + let strWrapper = > + Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString); Nit: formatting - Cc[..]. on first line, createInstance on the second, aligned with Cc. Same in sendInt... @@ +180,5 @@ > +} > + > +function makeQueryTerms(aQueryData) { > + return Object.keys(aQueryData).map( > + function(field) { Nit: I prefer putting the function on the same line as map and then unindenting the passed function's lines. It's more consistent with other anonymous functions throughout. @@ +229,5 @@ > + aNewData.timesUsed = (aNewData.timesUsed) ? aNewData.timesUsed : 1; > + aNewData.firstUsed = (aNewData.firstUsed) ? aNewData.firstUsed : aNow; > + aNewData.lastUsed = (aNewData.lastUsed) ? aNewData.lastUsed : aNow; > + return dbCreateAsyncStatement( > + query, aNewData, aBatchingStmts, aBindingArrays); Nit: just put this all on 1 line. I'd rather have a line over 80 chars than this. @@ +240,5 @@ > + guid : aGuid, > + }; > + > + return dbCreateAsyncStatement( > + query, queryParams, aBatchingStmts, aBindingArrays); Nit: 1 line @@ +257,5 @@ > + query += " WHERE " + queryTerms.join(" AND "); > + } > + > + return dbCreateAsyncStatement( > + query, aSearchData, aBatchingStmts, aBindingArrays); Nit: 1 line @@ +273,5 @@ > + query += queryTerms.join(", ") + " WHERE guid = :guid"; > + aNewData['guid'] = aGuid; > + > + return dbCreateAsyncStatement( > + query, aNewData, aBatchingStmts, aBindingArrays); Nit: 1 line @@ +309,5 @@ > + _dbConnection = dbOpen(dbFile); > + dbInit(); > + } catch (e) { > + log("Initialization failed: " + e); > + // If dbInit fails... I know this comment came from nsFormHistory, but let's have it also say that dbOpen can fail. (openUnsharedDatabase can also throw NS_ERROR_FILE_CORRUPTED). @@ +333,5 @@ > +/* > + * dbCreateAsyncStatement > + * > + * Creates a statement, wraps it, and then does parameter replacement > + * Will use memoization so that statements can be reused. I know killing the memoization is on your radar, but just a reminder (for both of us as I get up to speed with this code) @@ +475,5 @@ > +} > + > +// dbMigrateToVersion1 (bug 463154) -- unsupported as of Firefox 11 > +// dbMigrateToVersion2 (bug 243136) -- unsupported as of Firefox 11 > +// dbMigrateToVersion3 (bug 506402) -- unsupported as of Firefox 11 We'll want to update this whenever it lands, but also probably just say Gecko N (since this is toolkit code) @@ +554,5 @@ > + }; > + > + _dbConnection.asyncClose(blockingClose); > + > + let thread = Cc["@mozilla.org/thread-manager;1"].getService().currentThread; Services.tm.currentThread. @@ +786,5 @@ > + Services.obs.addObserver(FormHistoryObserver, "idle-daily", true); > + Services.obs.addObserver(FormHistoryObserver, "formhistory-expire-now", true); > + > + let messageManager = Cc["@mozilla.org/globalmessagemanager;1"] > + .getService(Ci.nsIChromeFrameMessageManager); Nit: Move period to previous line, align getService with Cc. @@ +788,5 @@ > + > + let messageManager = Cc["@mozilla.org/globalmessagemanager;1"] > + .getService(Ci.nsIChromeFrameMessageManager); > + messageManager.loadFrameScript( > + "chrome://satchel/content/formSubmitListener.js", true); Nit: just use 1 line @@ +847,5 @@ > + > + count : function formHistoryCount(aSearchData, aCallbacks) { > + if (!aCallbacks || !aCallbacks.onSuccess) { > + throw Components.Exception("Count must define an onSuccess callback to " > + + "receive results.", Nit: + on the previous line. There's another in `search`. In fact you could break these like you did the other exceptions in `updateFormHistory` (have whole string on 1 line) @@ +893,5 @@ > + let checkedChanges = []; > + > + function validIdentifier(change) { > + return change.guid ^ (change.fieldname && change.value); > + } 2 things: 1. Huh? I don't think you want to XOR anything here 2. There's no good reason to define the function here apart from the fact that it's being used here, but that means additional overhead everytime you run updateFormHistory (and since it falls out of scope, won't stay 'hot' if traced. You can leave it because I don't think it's a huge deal (and JS is better at that now). @@ +908,5 @@ > + continue; > + } > + } else { > + throw Components.Exception( > + "updateFormHistory op=\"modify\" does not correctly reference a entry.", Nit: There's a lot of \" going on in here. Can we just use single quotes inside the doubles (or visa versa)? @@ +1000,5 @@ > + return dbConnection; > + }, > +}; > + > +Object.freeze(FormHistory); Please add a comment here to make it clear why we're freezing (it's obvious to us, but for anybody new jumping in it's another barrier)

(In reply to Paul O'Shannessy [:zpao] from comment #88) > So it looks like it's possible to access FormHistory.DBConnection and have > it be in the process of migrating (since those statements are async, but > dbInit doesn't wait for that to complete. I'm not sure on the details, but > could a consumer could execute a sync statement and be bad here (Mak, what > do you think?) till we implement bug 702559 anyone can do synchronous stuff on the connection you are exposing, and those won't play nicely with your asynchronous stuff. Btw, anyone using an exposed connection to run WRITE statements should use the samurari sombrero of shame. Indeed a possibility may be to open a separate readonly connection and expose that one. Would use a bit more memory, though you may make it lazy and if nobody uses it no resources will be wasted.

Comment on attachment 585178 [details] [diff] [review] Use Asynchronous FormHistory.jsm for test_db_update_v999* Review of attachment 585178 [details] [diff] [review]: ----------------------------------------------------------------- Started to move to zpao, but I'll just drop in a quick review instead. Let's not user blocking_call() here (or, well, anywhere if possible). Should be simple enough to just convert these tests to be async (call async api, handle checks in callbacks).

zpao, do you have an ETA on reviewing this? felix, there are a slew of changes in that last few feedback reviews, but no new patches. are you able to still work on this, or just blocked on review before moving forward?

Comment on attachment 585181 [details] [diff] [review] Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js Same thing as what Dolske said, let's avoid blocking_call as much as possible. I know it keeps the test looking as procedural as possible. Callbacks should be ok (I would do named callbacks for each block of tests following each async step). If you wanted to get fancy, you may be able to get something like Task.js in for tests, but that's likely to be a bit more hassle.

Additional Changes - Fixed the above nits - Turns out, I wrote in the memoization at some point, I forgot to get rid of the comment

Comment on attachment 600303 [details] [diff] [review] Asynchronous FormHistory.jsm Review of attachment 600303 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Felix Fung (:felix) from comment #93) > - Turns out, I wrote in the memoization at some point, I forgot to get rid > of the comment The comment should stay if you're memoizing, but I got the feeling from comment #60 that you might remove it or add a cachable parameter. I think you should leave it or add a cachable param. Since you've looked at this more than anybody, what do you think? (you never responded to that comment, so now is your chance to pipe up) ::: toolkit/components/satchel/FormHistory.jsm @@ +50,5 @@ > + > +XPCOMUtils.defineLazyGetter(this, "_privBrowsingService", function() { > + if ("@mozilla.org/privatebrowsing;1" in Cc) { > + return Cc["@mozilla.org/privatebrowsing;1"] > + .getService(Ci.nsIPrivateBrowsingService); Nit: formatting > return Cc["@mozilla.org/privatebrowsing;1"]. > getService(Ci.nsIPrivateBrowsingService); @@ +77,5 @@ > + > +function sendStringNotification(aType, aString) { > + let strWrapper = > + Cc["@mozilla.org/supports-string;1"]. > + createInstance(Ci.nsISupportsString); Nit: formatting (there's a couple like this). Sorry that I wasn't clear about these last time around! > let strWrapper = Cc["@mozilla.org/supports-string;1"]. > createInstance(Ci.nsISupportsString); @@ +85,5 @@ > + > +function sendIntNotification(aType, aInt) { > + let intWrapper = > + Cc["@mozilla.org/supports-PRInt64;1"]. > + createInstance(Ci.nsISupportsPRInt64); Nit: formatting @@ +471,5 @@ > +} > + > +// dbMigrateToVersion1 (bug 463154) -- unsupported as of Firefox 11 > +// dbMigrateToVersion2 (bug 243136) -- unsupported as of Firefox 11 > +// dbMigrateToVersion3 (bug 506402) -- unsupported as of Firefox 11 Just a reminder that there will need to be updated when this lands @@ +782,5 @@ > + Services.obs.addObserver(FormHistoryObserver, "idle-daily", true); > + Services.obs.addObserver(FormHistoryObserver, "formhistory-expire-now", true); > + > + let messageManager = Cc["@mozilla.org/globalmessagemanager;1"]. > + getService(Ci.nsIChromeFrameMessageManager); Nit: formatting @@ +886,5 @@ > + let checkedChanges = []; > + > + function validIdentifier(change) { > + return change.guid ^ (change.fieldname && change.value); > + } I still don't think ^ is what you want.

About the memoization issue: So, looking at the internal consumers of the API, it makes sense to cache the statements since there's a small subset of queries. In regards to my comment 60, the problem is when add-ons start running their queries that have some arbitrary structure. I think a cacheable param would work but I'm on the fence about how that may be confusing from an API standpoint. Your opin? About validIdentifier: I actually did intend it to be XOR when I wrote it. Essentially, when identifying an entry for your change, you can _uniquely_ specify the entry using GUID or (fieldname, value) pair. I guess I didn't want users to think they could over-specify an entry in hopes of overwriting the GUID or fieldname.

(In reply to Felix Fung (:felix) from comment #95) > About the memoization issue: > So, looking at the internal consumers of the API, it makes sense to cache > the statements since there's a small subset of queries. In regards to my > comment 60, the problem is when add-ons start running their queries that > have some arbitrary structure. I think a cacheable param would work but I'm > on the fence about how that may be confusing from an API standpoint. Your > opin? I'm not overly worried about addons here. I think it'll affect a very small subset of users. Let's keep memoization in. If we did add a cachable param it would be to dbCreateAsyncStatement, which isn't exposed. We would only cache statements we have some confidence will be reused. But no matter... > About validIdentifier: > I actually did intend it to be XOR when I wrote it. Essentially, when > identifying an entry for your change, you can _uniquely_ specify the entry > using GUID or (fieldname, value) pair. I guess I didn't want users to think > they could over-specify an entry in hopes of overwriting the GUID or > fieldname. Ok, but the issue is that ^ is a bitwise XOR, not a logical XOR. I don't think it's working like you think. "{f60d9eac-9421-4abc-8491-8e8322b063d4}" ^ ("foo" && "bar") // 0 "{f60d9eac-9421-4abc-8491-8e8322b063d4}" ^ ("foo" && null) // 0 "{f60d9eac-9421-4abc-8491-8e8322b063d4}" ^ (null && null) // 0 Further, is that constraint specified anywhere? Since we don't have an IDL for this, we need to document things like this _somewhere_ (at least with public fn definition, probably also on mdc). Onto my next point... I don't see "modify" or "bump" tested at all. I'd really like to see a single file that steps through and really unit tests this API.

Comment on attachment 600303 [details] [diff] [review] Asynchronous FormHistory.jsm Review of attachment 600303 [details] [diff] [review]: ----------------------------------------------------------------- I really didn't meant to, but I accidentally found some more nits. Sorry! ::: toolkit/components/satchel/FormHistory.jsm @@ +150,5 @@ > + 'firstUsedStart', > + 'firstUsedEnd', > + 'lastUsedStart', > + 'lastUsedEnd', > +]; Nit: for validFields and pseudoSelectors, use double quotes in the literals. @@ +224,5 @@ > +function makeAddStatement(aNewData, aNow, aBatchingStmts, aBindingArrays) { > + log("addEntry"); > + > + let query = "INSERT INTO moz_formhistory (fieldname, value, timesUsed, firstUsed, lastUsed, guid) " + > + "VALUES (:fieldname, :value, :timesUsed, :firstUsed, :lastUsed, :guid)"; Nit: align the quotes on the 2nd line. @@ +268,5 @@ > + Cr.NS_ERROR_ILLEGAL_VALUE); > + } > + > + query += queryTerms.join(", ") + " WHERE guid = :guid"; > + aNewData['guid'] = aGuid; Nit: double quote for consistency. @@ +281,5 @@ > + let bytes = 0; > + for (let i = 1; bytes < 12 ; i+= 2) { > + // Skip dashes > + if (uuid[i] == "-") > + i++; Nit: spacing. 2 v 4.

Comment on attachment 600303 [details] [diff] [review] Asynchronous FormHistory.jsm Review of attachment 600303 [details] [diff] [review]: ----------------------------------------------------------------- It's close, but there are concerns. I'd like to see tests before I r+.

Updated patch which differs only by: - updating message manager interface to due to other changes - change to use 2.0 license

Address comment 59

Address comment 77

This patch differs by: - updating to 2.0 license - remove event loop code and replace with asynchronous version of canClear - update tests to deal with asynchronous changes

This patch changes: - always enable the cmd_clearhistory command due to the check being asynchronous. Does this matter much? - switch away from the old form history component when checking the number of entries

This patch differs by: - updating to 2.0 license - remove tests from xpcshell.ini

Unchanged, but updated for license changes

Change test to work asynchronously

Changes: - define and initialize FormHistory in browser.js (this also fixes an existing bug where form autocomplete does not work unless the searchbar is initialized, as only the searchbar initializes the old form history component) - license update - address comment 94

Try run is at https://tbpl.mozilla.org/?tree=Try&rev=ac246d851e9e Two comments are not addressed: From comment 94: > +} > + > +// dbMigrateToVersion1 (bug 463154) -- unsupported as of Firefox 11 > +// dbMigrateToVersion2 (bug 243136) -- unsupported as of Firefox 11 > +// dbMigrateToVersion3 (bug 506402) -- unsupported as of Firefox 11 > Just a reminder that there will need to be updated when this lands I don't know what this means. From comment 96: > I don't see "modify" or "bump" tested at all. I'd really like to see a single file that steps through and really unit tests this API. Haven't added these tests

Comment on attachment 688753 [details] [diff] [review] Asynchronous FormHistory.jsm, v2 >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -1185,16 +1195,18 @@ var gBrowserInit = { > CombinedStopReload.init(); > allTabs.readPref(); > TabsOnTop.init(); > BookmarksMenuButton.init(); > TabsInTitlebar.init(); > gPrivateBrowsingUI.init(); > retrieveToolbarIconsizesFromTheme(); > >+ var fm = FormHistory; // this initializes the form-fill-in component This looks like it isn't really tied to browser windows and should happen in nsBrowserGlue instead. Can you add an explicit init method instead of the magic initialization when importing the module for the first time?

Comment on attachment 688753 [details] [diff] [review] Asynchronous FormHistory.jsm, v2 I'll address Dao's comment 110 in the next iteration.

(In reply to Dão Gottwald [:dao] from comment #110) > Can you add an explicit init method instead of the magic initialization when > importing the module for the first time? Dao, some unit tests invoke the form history component while running, even though they don't directly test it. For example, test_cleanHistory_shutdown.js. This causes the test to fail because the init() method in the form history component hasn't been called. Do you think it is better to add a call to the init() method to all of these specific tests, or leave the automatic initialization in place?

I'm not sure I understand. Are you patching test_clearHistory_shutdown.js such that it imports FormHistory.jsm? If so, I think explicitly invoking the initialization is preferable there as well. Makes it clearer what's going on.

OK, so after (In reply to Dão Gottwald [:dao] from comment #113) > I'm not sure I understand. Are you patching test_clearHistory_shutdown.js > such that it imports FormHistory.jsm? No. But I discovered that the issue was that sanitize.js, which is invoked by the test was importing FormHistory without calling init().

This patch adds the deleted form history support that was added to nsFormHistory by bug 725881.

This patch converts various synchronous tests to use the new FormHistory module and adds more asynchronous tests to test_history_api.js.

What's the plan regarding the replacement of the old service? As far as I see here, the module and the old component can be used at the same time, that means both listen to "FormHistory:FormSubmitEntries" messages and add entries, that means double entries. Is comment 66 (remove nsFormHistory.js when the new async one lands) still the current plan? I'm currently reviewing the patch, though some comments clearly depend on this.

Comment on attachment 688753 [details] [diff] [review] Asynchronous FormHistory.jsm, v2 Review of attachment 688753 [details] [diff] [review]: ----------------------------------------------------------------- As discussed over IRC, would be healthier to use the new Sqlite.jsm module, but at this point we'll look into that into a follow-up, please file it. Globally looks like it could work, but it's really lacking comments and documentation, even just an opening comment explaining the API and how to use it would be welcome, even for a SR. ::: browser/base/content/browser.js @@ +92,5 @@ > +}); > +__defineSetter__("FormHistory", function (val) { > + delete this.FormHistory; > + return this.FormHistory = val; > +}); XPCOMUtils.defineLazyModuleGetter could be enough, considered FormHistory.jsm is a new module, there should be noone trying to import it yet, new code can adapt by using XPCOMUtils as well or deleting the property (not sure why AddonManager followed the PluralForm example). The existing PluralForm getter was added before XPCOMUtils and the setter was added for backwards compatibility of code already doing Cu.import in the same scope. Definitely not a case that may happen here. But actually, if we do what I suggest below we may even avoid this. @@ +1199,5 @@ > TabsInTitlebar.init(); > gPrivateBrowsingUI.init(); > retrieveToolbarIconsizesFromTheme(); > > + var fm = FormHistory; // this initializes the form-fill-in component there's no need to assign it to a var. That said yeah, this looks a bit unclean. Supposing the plan is to remove the old service, I propose that: we merge FormHistoryObserver and FormHistoryListener to a new xpcom component (FormHistoryGlue? FormHistoryKickstarter?) that registers in the profile-after-change category. This component takes care of listening to observer topics and messages (implements nsIObserver and nsIFrameMessageListener) and of registering the form fill listener (itself). On receiving topics, it will redirect the calls to the module. This has the advantage to handle init by itself, and really makes the module lazy (as it is now it's fake-lazy, we basically import it immediately). The disadvantage is that apps that don't use form history (any?) will have this component starting, though it will be really small and can check browser.formfill.enable to avoid doing any unneecessary work. Plus I suppose they could avoid shipping it completely by not shipping the manifest. ::: toolkit/components/satchel/FormHistory.jsm @@ +20,5 @@ > + return Cc["@mozilla.org/privatebrowsing;1"]. > + getService(Ci.nsIPrivateBrowsingService); > + } > + return null; > +}); per bug 723001, this should go away @@ +32,5 @@ > + */ > + > +let debug = false; > +let enabled = true; > +let saveHttpsForms = true; I think these would be better as getters so that we remove the need to update prefs in init(). Basically we could have a _prefs {debug, enabled, saveHttps} cached object, an ensurePrefs helper that, if the cache is undefined will load prefs. on pref change we'd just wipe the cache. something like get debug() {ensurePrefs(); return _prefs.debug} The scope is to completely get rid of init() so the module becomes really lazy. @@ +57,5 @@ > +} > + > +function sendNotification(aType, aData) { > + Services.obs.notifyObservers(aData, "satchel-storage-changed", aType); > +} these 3 could be merged into just sendNotification(aType, aData) by checking typeof(aData) and creating the right wrapper. @@ +129,5 @@ > + Cr.NS_ERROR_ILLEGAL_VALUE); > + } > + } > + return checkedData; > +} considered at the first error it throws, I don't see the point of moving everything to a new object... I'd say to just go through all fields and throw if something is wrong. @@ +131,5 @@ > + } > + return checkedData; > +} > + > +function validatePseudoData(aData, aDataType) { same comments as above... plus I really don't understand this "pseudo" prefix... so what if we rename pseudoSelectors to searchFilters, validateData to validateOpData, validatePseudoData to validateSearchData? @@ +145,5 @@ > + } > + return checkedData; > +} > + > +function makeQueryTerms(aQueryData) { s/Terms/Predicates/ . Though, if I looked correctly everytime we get these we join, either on " AND " or ", "/ So this could actually take a second optional param, aDelimiter = " AND " and directly return the built predicates string. at that point the if (queryTerms.length != 0) checks could be simplified to just if (queryPredicates) @@ +193,5 @@ > + "VALUES (:fieldname, :value, :timesUsed, :firstUsed, :lastUsed, :guid)"; > + > + aNewData.timesUsed = (aNewData.timesUsed) ? aNewData.timesUsed : 1; > + aNewData.firstUsed = (aNewData.firstUsed) ? aNewData.firstUsed : aNow; > + aNewData.lastUsed = (aNewData.lastUsed) ? aNewData.lastUsed : aNow; these can be rewritten as val = one || two; @@ +267,5 @@ > + dbFile = Services.dirsvc.get("ProfD", Ci.nsIFile).clone(); > + dbFile.append("formhistory.sqlite"); > + log("Opening database at " + dbFile.path); > + > + _dbConnection = dbOpen(dbFile); dbOpen has only 2 call points, I think it doesn't deserve a separate function, the logging is also already present here in both points... please just hardcode the call @@ +272,5 @@ > + dbInit(); > + } catch (e) { > + log("Initialization failed: " + e); > + // If dbOpen or dbInit fails... > + if (e.result == Cr.NS_ERROR_FILE_CORRUPTED) { this could be rewritten as } catch (e if e.result == Cr.NS_ERROR_FILE_CORRUPTED) { dbCleanup } catch (e) { throw } @@ +288,5 @@ > +}); > + > + > + > +let dbStmts = {}; Could be a Map() @@ +289,5 @@ > + > + > + > +let dbStmts = {}; > +let closeCallbacks = []; this needs some documentation about what is it and how it's supposed to be used... If it's just to notify consumers it's going away, I wonder if it couldn't just rely on existing notifications (so request that consumers stops using it at profile-change-teardown) or fire its own formhistory-shutdown notification @@ +311,5 @@ > + if (!bindingArray) { > + // first time using a particular statement in update > + aBindingArrays[stmt] = bindingArray = > + stmt.newBindingParamsArray(); > + aBatchingStmts.push(stmt); I'm not sure why we need aBatchingStmts, couldn't this just always return a stmt, and the caller store it when needed (it may use a Set for example)? @@ +320,5 @@ > + bindingParams.bindByName(field, aParams[field]); > + } > + bindingArray.addParams(bindingParams); > + return; > + } else { return/else @@ +321,5 @@ > + } > + bindingArray.addParams(bindingParams); > + return; > + } else { > + if (aParams) { The batching side doesn't check aParams, if it's optional it should be checked both sides, otherwise nowhere @@ +372,5 @@ > + log("Creating DB -- tables"); > + for (let name in dbSchema.tables) { > + let table = dbSchema.tables[name]; > + let tSQL = [[col, table[col]].join(" ") for (col in table)].join(", "); > + _dbConnection.createTable(name, tSQL); Please file a follow-up bug to make dbCreate asynchronous... once we use Sqlite.jsm that will come for free. As it is now, createTable, executeSimpleSQL and schemaVersion will be on mainthread. @@ +386,5 @@ > + > + _dbConnection.schemaVersion = DB_SCHEMA_VERSION; > +} > + > +function dbMigrate(oldVersion) { nit: all these "db" prefixed methods make me think the intent here was to create a Database helper object... @@ +422,5 @@ > + _dbConnection.executeAsync(migrateStmts, migrateStmts.length, { > + handleResult : NOOP, > + handleError : function dbMigrateHandleError(aError) { > + throw Components.Exception(aError.message, > + Cr.NS_ERROR_UNEXPECTED); I suppose a reportError would be more useful than throwing to nothing @@ +457,5 @@ > + [col for (col in table)].join(", ") + > + " FROM " + name; > + try { > + let stmt = _dbConnection.createStatement(query); > + // (no need to execute statement, if it compiled we're good) this is another synchronous piece that should be handled in the above follow-up. @@ +497,5 @@ > + (closeCallbacks.shift()).complete(); > + } catch (e) { > + Components.utils.reportError(e); > + } > + } as I said, not sure if we couldn't rather come with something simpler (nsIObserver topic or shutdown topics) I also don't think consumers expect us to notify DBClose when the module starts, tries to init the database and fails cause the db is corrupt, creating a new database. It should really not notify in such a case. @@ +503,5 @@ > + // Connection may never have been created if say dbOpen failed but we still > + // end up calling dbClose as part of the rest of dbCleanup. > + if (!_dbConnection) { > + return; > + } if we have no connection we have no statements, so the above finalize could be moved after this check @@ +520,5 @@ > + while (!blockingClose.completed) { > + thread.processNextEvent(true); > + } > + } else { > + _dbConnection.asyncClose(); we may pass the callback in both cases, note that the callback has the [function] annotation, so we can just let closed = false; _dbConnection.asyncClose(function () { closed = true; }); if (aBlocking) { while (!closed) ... } @@ +579,5 @@ > + // for each change, we either create and append a new storage statement to > + // stmts or bind a new set of parameters to an existing storage statement. > + // stmts and bindingArrays are updated when makeXXXStatement eventually > + // calls dbCreateAsyncStatement. > + let stmts = []; may be a Set @@ +581,5 @@ > + // stmts and bindingArrays are updated when makeXXXStatement eventually > + // calls dbCreateAsyncStatement. > + let stmts = []; > + let notifications = []; > + let bindingArrays = {}; may be a Map @@ +586,5 @@ > + > + for each (let change in aChanges) { > + switch (change.op) { > + case "remove": > + delete change.op; so, what about doing externally to the switch: let operation = change.op; delete change.op; we do this regardless the branch... @@ +615,5 @@ > + makeAddStatement(change, now, stmts, bindingArrays); > + notifications.push([ "FormHistory-Add", change.guid ]); > + break; > + default: > + // We should've already guaranteed that change.op is one of the above though an additional Throw woudln't hurt! @@ +627,5 @@ > + let handlers = { > + handleCompletion : > + function updateFormHistoryCompletionHandler(aReason) { > + if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) { > + for each (let notification in notifications) { for (let notification of notifications) or even better for (let [notification, param] of notifications) @@ +632,5 @@ > + // We're either sending a GUID or nothing at all. > + if (notification[1]) { > + sendStringNotification(notification[0], notification[1]); > + } else { > + sendNotification(notification[0], null); the if/else is pointless, we already set the param to null above... @@ +650,5 @@ > + handlers.handleError = > + function updateFormHistoryErrorHandler(aError) { > + aCallbacks.onFailure(aError); > + }; > + } I miss why not putting this directly into handleError? sounds like a nano perf optimization that makes the code harder to follow. @@ +696,5 @@ > + > + FormHistory.updateFormHistory([ > + { > + lastUsedEnd : aExpireTime, > + remove : true shouldn't this be op: "remove" ? The lack of documentation is puzzling :( This may also point out that this needs a test for expiration. @@ +715,5 @@ > + */ > +function expireOldEntriesVacuum(aExpireTime, aBeginningCount) { > + FormHistory.count(null, { > + onSuccess : function expireEndCountSuccess(aEndingCount) { > + if (aBeginningCount - aEndingCount > 500) { many small expirations would skip this. if we create the external component for initialization, we may maybe also register it to the vacuum-participant category, so that it's also automatically vacuumed every 30 days. @@ +736,5 @@ > + } > + }); > +} > + > +function init() { as I said, we may avoid init() with the component, adding some "internal" _methods to FormHistory to allow the component to communicate with it @@ +837,5 @@ > + > + updateFormHistory : function formHistoryUpdate(aChanges, aCallbacks) { > + // Prevent any writes to form history during private browsing. > + if (!enabled || > + _privBrowsingService && _privBrowsingService.privateBrowsingEnabled) { per bug 723001 the pb condition should go away. @@ +845,5 @@ > + let searchHandler = { > + numSearches : 0, > + completedSearches : 0, > + failed : false > + }; this is used much later in the method, could be moved there... and would be nice to document what it does @@ +847,5 @@ > + completedSearches : 0, > + failed : false > + }; > + > + let checkedChanges = []; again, we throw in any case where aChanges contains bogus input. so I'm not sure about the value of copying all the input to a new array

I also think we may change updateFormHistory to accept either an array of changes or a single change, I see many cases where we really just need a single change and it's easy to support.

Comment on attachment 688751 [details] [diff] [review] Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js, v2 Review of attachment 688751 [details] [diff] [review]: ----------------------------------------------------------------- this test is likely going to change and bitrot soon for async history (patch pending review), thus clearing the request for now. Regardless, the current browser chrome tests async story is quite bad, we have nextStep and generatorTest in browser-test.js, but it's not really as nice as xpcshell add_task API, indeed it's barely used in just 8 tests, while others define their own generator and iterator. It's likely we may want addTask() functions, and if those are defined we should create a default runTest that runs those tasks, plus import Promise into the scope. So we can write better async browser-chrome tests. worth a bug?

Comment on attachment 688747 [details] [diff] [review] Use Asynchronous FormHistory.jsm for content/search.xml, v2 Review of attachment 688747 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/search/content/search.xml @@ +436,5 @@ > } > > // Save the current value in the form history > if (textValue && !PrivateBrowsingUtils.isWindowPrivate(window)) { > + FormHistory.updateFormHistory( would be better to not rely on global FormHistory object, and rather have this.FormHistory ::: browser/components/search/test/browser_426329.js @@ +188,3 @@ > > testSearchHistory(); > }, 5000); ugh, a 5 seconds timeout :( @@ +193,5 @@ > function testSearchHistory() { > var textbox = searchBar._textbox; > for (var i = 0; i < searchEntries.length; i++) { > + let stmt = FormHistory.DBConnection.createStatement( > + "SELECT COUNT(*) AS numEntries FROM moz_formhistory WHERE fieldname = :fieldname AND value = :value"); are we sure the additions already made to the database here? doesn't look like we are waiting... a best way would be to create an async statement and make the test continue on its completion, since all statements are enqueued that's enough to ensure additions completed.

(In reply to Marco Bonardo [:mak] from comment #117) > What's the plan regarding the replacement of the old service? ... > Is comment 66 (remove nsFormHistory.js when the new async one lands) still > the current plan? That's my assumption.

Comment on attachment 688740 [details] [diff] [review] Use Asynchronous FormHistory.jsm for sanitize.js the test is bitrotting soon unfortunately, I'm clearing request for now until we have the service patch, by then the tests should be stable enough to have a final patch.

There's a couple on intermittent failures still with the test browser_sanitizeDialog.js and test_form_autocomplete.html so I will investigate while the main code gets reviewed.

Comment on attachment 712523 [details] [diff] [review] Use Asynchronous FormHistory.jsm for content/search.xml, v3 >--- a/browser/components/search/content/search.xml >+++ b/browser/components/search/content/search.xml >+ <field name="FormHistory" readonly="true"> >+ let formHistory = {}; >+ Components.utils.import("resource://gre/modules/FormHistory.jsm", formHistory); >+ formHistory.FormHistory; >+ </field> you can simplify this to: Components.utils.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;

Comment on attachment 712527 [details] [diff] [review] Asynchronous FormHistory.jsm, v3 Review of attachment 712527 [details] [diff] [review]: ----------------------------------------------------------------- The startup path is much better, unfortunately the async initialization of the database has some serious issue :( ::: browser/base/content/browser.js @@ +83,5 @@ > return this.PluralForm = val; > }); > > +XPCOMUtils.defineLazyModuleGetter(this, "FormHistory", > + "resource://gre/modules/FormHistory.jsm"); why is this needed in this patch, if there's nothing using it yet? ::: browser/components/nsBrowserGlue.js @@ +59,5 @@ > XPCOMUtils.defineLazyModuleGetter(this, "RecentWindow", > "resource:///modules/RecentWindow.jsm"); > > +XPCOMUtils.defineLazyModuleGetter(this, "FormHistory", > + "resource://gre/modules/FormHistory.jsm"); ditto, nothing is using it yet and initialization is now done in the startup component. ::: toolkit/components/satchel/FormHistory.jsm @@ +11,5 @@ > + * search(terms, queryData, callback) > + * Look up values that have been previously stored. > + * terms - array of terms to return data for > + * queryData - object that contains the query terms > + * The query object contains properties for each term to match where the value comma after match @@ +14,5 @@ > + * queryData - object that contains the query terms > + * The query object contains properties for each term to match where the value > + * is the value that term must have. For example { term1: value1, term2: value2 } > + * callback - callback that is called when results are available or an error occurs. > + * The callback is passed a result array contains each found entry, typo: contains @@ +16,5 @@ > + * is the value that term must have. For example { term1: value1, term2: value2 } > + * callback - callback that is called when results are available or an error occurs. > + * The callback is passed a result array contains each found entry, > + * where each element in the array is an object containing a property > + * for each search term. I'm not english mother tongue, but sounds like these descriptions could be improved a bit @@ +19,5 @@ > + * where each element in the array is an object containing a property > + * for each search term. > + * count(terms, queryData, callback) > + * Find the number of stored entries that match the given criteria. > + * queryData - array of objects that indicate the query (as with the search function) maybe s/ (as with the search function)/. See the search method for details./ @@ +53,5 @@ > + * lastUsedStart - search for entries last accessed after or at this time > + * lastUsedEnd - search for entries last accessed before or at this time > + * > + * In all of the above methods, the callback argument should be an object with a > + * onSuccess(args) and onFailure(error) function. s/a/an/ trailing space @@ +65,5 @@ > + > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "_uuidService", no need to underscore prefix this, it's already hidden by the module scope @@ +83,5 @@ > +function ensurePrefs() { > + if (prefsSet) > + return; > + > + prefsSet = true; these should be Prefs.prefsSet... though I suppose a better naming may be Prefs.initialized... or even move ensurePrefs() into Prefs.ensureInitialized() and use an _initialized internal? @@ +105,5 @@ > + let intWrapper = Cc["@mozilla.org/supports-PRInt64;1"]. > + createInstance(Ci.nsISupportsPRInt64); > + intWrapper.data = aData; > + aData = intWrapper; > + } if no other formats are expected than "string" and "number" this should probably throw a Cr error @@ +206,5 @@ > +function makeCountStatement(aSearchData) { > + let query = "SELECT COUNT(*) AS numEntries FROM moz_formhistory"; > + let queryTerms = makeQueryPredicates(aSearchData); > + > + if (queryTerms) { nit: pointless newline @@ +216,5 @@ > +function makeSearchStatement(aSearchData, aSelectTerms) { > + let query = "SELECT " + aSelectTerms.join(", ") + " FROM moz_formhistory"; > + let queryTerms = makeQueryPredicates(aSearchData); > + > + if (queryTerms) { nit: pointless newline @@ +221,5 @@ > + query += " WHERE " + queryTerms; > + return dbCreateAsyncStatement(query, aSearchData); > + } > + > + return dbCreateAsyncStatement(query); why here we differentiate the call, by not passing aSearchData, while in makeCountStatement we always pass it? If we throw on invalid input, aSearchData may just be an object containing some keys, an empty object or undef/null. Either we make dbCreateAsyncStatement able to also handle the empty object case, or we throw if we get an empty object, in both cases we should not need to differentiate the call. @@ +225,5 @@ > + return dbCreateAsyncStatement(query); > +} > + > +function makeAddStatement(aNewData, aNow, aBindingArrays) { > + log("addEntry"); the log could be a bit more corpose, maybe adding a json stringify of aNewData? Btw, I'm not sure we should log here, at statement creation, we should probably log down in the FormHistory method actually invoking this... @@ +291,5 @@ > + raw += String.fromCharCode(hexVal); > + bytes++; > + } > + return btoa(raw); > +} scope-creep: we should really move short guid generation to some shared place in toolkit @@ +417,5 @@ > + // should switch to a different table or file.] > + > + if (!dbAreExpectedColumnsPresent()) > + throw Components.Exception("DB is missing expected columns", > + Cr.NS_ERROR_FILE_CORRUPTED); please brace multiline ifs @@ +440,5 @@ > + _dbConnection.executeAsync(migrateStmts, migrateStmts.length, { > + handleResult : NOOP, > + handleError : function dbMigrateHandleError(aError) { > + throw Components.Exception(aError.message, > + Cr.NS_ERROR_UNEXPECTED); So, there are a couple problems here: 1. migration is async, but consumers do FormHistory.DBConnection.something. That means when "something" is synchronous, it runs on an old version of the database. Nothing synchronous should run on DBConnection, but just looking at the tests attached in this bug, they are full of wrong calls to createStatement() and executeSimpleSQL(), so it's really a footgun (if we can't do it right, let alone third party consumers) 2. we swallow migration errors here, that means if it fails for whatever reason (unexpected data?) nothing will notify dbInit, that means it won't try to mark things as corrupt and recover by creating a new db I don't see an easy solution: A. we reimplement the whole DB thing in SQLite.jsm and expose the wrapped connection. Though since it's promise based, the DBConnection getter will need a callback and it won't be usable from cpp. B. we add an AdoptConnection() method to SQLite.jsm and expose the wrapped connection. Same as above but we don't have to reimplement everything. C. we don't expose DBConnection at all, rather add needed async APIs to the module D. we just fix internal and tests consumers to use async statements. We can already be sure it will be (ab)used wrongly by add-ons and new code, based on past experience. Also consider most of dbInit now is synchronous but it will have to be converted, that means .DBConnection.sycnhronousSomething will actually act on an empty database at that point... At this point, supposing we are not willing to rewrite everything, the only safe way out I see is C, long-term plan is A. Unless we have consumers who can't work without a connection handle. @@ +449,5 @@ > + } > + } > + }); > + > + log("DB migration completed."); shouldn't this be logged in handleCompletion? @@ +503,5 @@ > + dbClose(false); > + dbFile.remove(false); > +} > + > +function dbClose(aShutdown) { needs logging @@ +505,5 @@ > +} > + > +function dbClose(aShutdown) { > + if (aShutdown) { > + sendNotification("FormHistory-Shutdown", null); please lowercase notifications, it's hard enough to remember names of the notifications without having to remember casing :) @@ +553,5 @@ > + for each (let change in aChanges) { > + let operation = change.op; > + delete change.op; > + let stmt; > + switch (operation) { these operations would love some logging @@ +556,5 @@ > + let stmt; > + switch (operation) { > + case "remove": > + stmt = makeRemoveStatement(change, bindingArrays); > + notifications.push([ "FormHistory-Remove", null ]); lc notifications casing (not going to repeat) @@ +632,5 @@ > + * > + * Removes entries from database. > + */ > +function expireOldEntriesDeletion(aExpireTime, aBeginningCount) { > + log("expireOldEntriesDeletion"); arguments in the log @@ +872,5 @@ > + } > + }, > + > + get DBConnection() { > + return dbConnection; if the second try to init dbConnection fails the lazy getter may throw, since this is an attribute could be better to return null than throwing. @@ +889,5 @@ > + let expireDays = Services.prefs.getIntPref("browser.formfill.expire_days"); > + // Calculate expireTime in microseconds > + let expireTime = (Date.now() - expireDays * DAY_IN_MS) * 1000; > + > + sendNotification("FormHistory-BeforeExpireOldEntries", expireTime); casing ::: toolkit/components/satchel/FormHistoryStartup.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + > +const Cc = Components.classes; nit: too many newlines @@ +4,5 @@ > + > + > +const Cc = Components.classes; > +const Ci = Components.interfaces; > +const Cr = Components.results; Cr is unused in this component @@ +10,5 @@ > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm"); > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +function FormHistoryStartup() { > + Services.obs.addObserver(this, "profile-after-change", true); you should not need to do this, since the component is in the profile-after-change category, it gets the notifications already. The observer service first notifies everything in the category cache and then common observers. like this you are likely being notified twice. @@ +25,5 @@ > + > + observe: function(subject, topic, data) { > + switch (topic) { > + case "nsPref:changed": > + this.FormHistory.updatePrefs(); I think you could move the lazyModuleGetter to the component scope so this just becomes FormHistory.updatePrefs() (and so on later) @@ +43,5 @@ > + }, > + > + inited: false, > + > + init: function init() nit: there should be no more the need to label functions, after recent improvements to the js stack. But if you really want to, you should also name observe (either all or none) @@ +56,5 @@ > + Services.prefs.addObserver("browser.formfill.", this, true); > + > + // triggers needed service cleanup and db shutdown > + Services.obs.addObserver(this, "profile-before-change", true); > + Services.obs.addObserver(this, "idle-daily", true); you can register your component in multiple categories, just add the idle-daily category to the manifest, it's a valid category ::: toolkit/components/satchel/Makefile.in @@ +38,5 @@ > $(NULL) > > EXTRA_PP_COMPONENTS = \ > nsFormHistory.js \ > + FormHistoryStartup.js \ I don't see any preprocessing in the file, should likely be moved to EXTRA_COMPONENTS

Comment on attachment 712528 [details] [diff] [review] Add deleted form history table, v2 Review of attachment 712528 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/satchel/FormHistory.jsm @@ +285,5 @@ > > +function makeMoveToDeletedStatement(aGuid, aNow, aData, aBindingArrays) { > +#ifdef ANDROID > + let query = "INSERT INTO moz_deleted_formhistory (guid, timeDeleted)"; > + let queryTerms = makeQueryPredicates(aData); The original implementation is fancy... I couldn't even find where these entries are removed, I suppose Android backend does, cause otherwise would be a privacy hit. Btw, let's try to bring this to the 20th century, what about on DB init, ifdef Android, CREATE TEMP TRIGGER ON BEFORE DELETE (http://www.sqlite.org/lang_createtrigger.html) that automagically copies stuff from the table to the deleted table when a DELETE is executed. I can provide the trigger syntax if you wish. That will basically reduce this patch to a change to dbInit, that is what should have been done originally, unless I missed something.

Comment on attachment 712523 [details] [diff] [review] Use Asynchronous FormHistory.jsm for content/search.xml, v3 Review of attachment 712523 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/search/test/browser_426329.js @@ +192,5 @@ > > function testSearchHistory() { > var textbox = searchBar._textbox; > for (var i = 0; i < searchEntries.length; i++) { > + let stmt = FormHistory.DBConnection.createStatement( this clearly depends on my previous comments... regardless, we can't use synchronous APIs on DBConnection.

> So, there are a couple problems here: > 1. migration is async, but consumers do FormHistory.DBConnection.something. > That means when "something" is synchronous, it runs on an old version of the > database. Nothing synchronous should run on DBConnection, but just looking > at the tests attached in this bug, they are full of wrong calls to > createStatement() and executeSimpleSQL(), so it's really a footgun (if we > can't do it right, let alone third party consumers) > 2. we swallow migration errors here, that means if it fails for whatever > reason (unexpected data?) nothing will notify dbInit, that means it won't > try to mark things as corrupt and recover by creating a new db The later patch that implements the migration to version 4 (https://bugzilla.mozilla.org/attachment.cgi?id=712528) changes migration to be synchronous.

(In reply to Neil Deakin from comment #138) > The later patch that implements the migration to version 4 > (https://bugzilla.mozilla.org/attachment.cgi?id=712528) changes migration to > be synchronous. That doesn't help with the fact we will have to convert all that code to be async asap, and then DBConnection will be totally broken. I don't care what we do internally now provided we can transition to async in future. If we put out that synchronous getter now like it is, code and add-ons will start using it and then we'd have to fight this battle again in short future. if we don't expose the DBConnection getter, then we could change the underlying code at any time. But as I said I don't know how hard is to replace test queries with proper APIs here. If the problem here is avoiding to rewrite hundreds of tests, I think at a minimum we should expose _DBConnection (internal prefixed) and be really clear (GIANT WARNING) in documenting it, that _ONLY TESTS_ should be using it and it will be removed shortly. Then file a follow-up to expose a proper SQLite.jsm connection for anyone else.

> Btw, let's try to bring this to the 20th century, what about on DB init, > ifdef Android, CREATE TEMP TRIGGER ON BEFORE DELETE > (http://www.sqlite.org/lang_createtrigger.html) that automagically copies > stuff from the table to the deleted table when a DELETE is executed. > I can provide the trigger syntax if you wish. > That will basically reduce this patch to a change to dbInit, that is what > should have been done originally, unless I missed something. Can you provide this syntax? Or just provide a patch if it is that simple.

sure, autoflagging to rememeber to do that

I tried it, the problem is that a trigger would regress bug 756701, cause it cannot differentiate single deletes from clear-everything deletes (SQLite has FOR EACH ROW triggers, but not FOR EACH STATEMENT, and doesn't support temporarily triggers disabling), unless we temporarily remove it on clear, and recreate it just later, but that would lose the advantage of code simplicity. All of the alternatives I may think of are worse than what we have. We'll just take your approach ported from the previous code, since I don't see a winning alternative (apart fixing notifications so that Sync doesn't have to access a private database, but this is out-of-scope). sorry for the red-herring

So I've converted all of the tests to be asynchronous and removed the dbConnection property. However there are three specific database calls that I haven't removed, and was wondering what the best approach for these is: 1. There's a complicated query at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormAutoComplete.js#245 to retrieve results for autocomplete. Should I just move this query and result processing to FormHistory.jsm? 2. One test needs to call VACUUM. The current patch just adds a vacuum method. 3. The deleted table tests need to perform operations on the deleted table to verify correctness. The operations are: tableExists, count rows, and get a deleted row. Apart from tableExists the tests are disabled anyway. Maybe I should just remove them and add only a tableExists method.

(In reply to Neil Deakin from comment #144) > 1. There's a complicated query at > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/ > nsFormAutoComplete.js#245 to retrieve results for autocomplete. Should I > just move this query and result processing to FormHistory.jsm? That's the "frecency" calculation used by nsFAC to order results from FormHistory, it is still a way to query formhistory and thus I think makes sense to add that as a special select query (including that into the current API would be bonus points since it already allows to do common selects). The problem is rather the fact nsFAC runs that as a synchronous query :( > 2. One test needs to call VACUUM. The current patch just adds a vacuum > method. Sounds good, honestly I think we should just remove the special vacuum management from here and register the component with the vacuum manager, but that's a follow-up thing (and on the other side would also expose again the connection, so it's possible may require changes...). For now it will be ok to add vacuum(). > 3. The deleted table tests need to perform operations on the deleted table > to verify correctness. The operations are: tableExists, count rows, and get > a deleted row. Apart from tableExists the tests are disabled anyway. Maybe I > should just remove them and add only a tableExists method. The test could just open its own connection to the database and run whatever queries it wants. We don't need to use the service connection at any cost.

> The problem is rather the fact nsFAC runs that as a synchronous query :( That is bug 697377, although I'm just going to include the patches here since the patches for both bugs are interconnected.