Last Comment Bug 566746 - (asyncFormHistory) Form history should use asynchronous storage API
(asyncFormHistory)
: Form history should use asynchronous storage API
Status: RESOLVED FIXED
[Snappy:P1][Async:P1]
: addon-compat, dev-doc-needed, main-thread-io
Product: Toolkit
Classification: Components
Component: Form Manager (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla23
Assigned To: Mark Hammond [:markh]
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
: 551976 795095 (view as bug list)
Depends on: 868296 942613 980244 817731 851641 872151 873429 896142 909357
Blocks: 630705 StorageJank 912031 asyncFormAC 720311 854438
  Show dependency treegraph
 
Reported: 2010-05-18 16:13 PDT by Benjamin Stover (:stechz)
Modified: 2014-03-06 18:37 PST (History)
36 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP - Async draft for nsFormHistory.js (21.95 KB, patch)
2011-10-11 14:54 PDT, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
WIP2 - Async draft for nsFormHistory.js (24.65 KB, patch)
2011-10-12 07:13 PDT, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
WIP3 - Async draft for nsFormHistory.js (24.61 KB, patch)
2011-10-13 05:27 PDT, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
nsIAsyncFormHistory (26.23 KB, patch)
2011-10-13 12:11 PDT, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
nsIAsyncFormHistory (24.79 KB, patch)
2011-10-18 11:43 PDT, Felix Fung (:felix)
mak77: feedback-
Details | Diff | Splinter Review
nsIAsyncFormHistory - sanitize.js (1.74 KB, patch)
2011-10-18 11:47 PDT, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
AsyncFormHistory.jsm (24.32 KB, patch)
2011-11-04 13:35 PDT, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
Use AsyncFormHistory.jsm - sanitize.js (2.13 KB, patch)
2011-11-04 13:38 PDT, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
Use AsyncFormHistory.jsm - test/browser_sanitize-timespans.js (27.29 KB, patch)
2011-11-04 13:40 PDT, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
AsyncFormHistory.jsm (24.36 KB, patch)
2011-11-10 13:12 PST, Felix Fung (:felix)
dolske: review-
Details | Diff | Splinter Review
AsyncFormHistory.jsm (27.17 KB, patch)
2011-11-28 13:23 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
Use AsyncFormHistory.jsm - sanitize.js (2.97 KB, patch)
2011-11-28 13:42 PST, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
AsyncFormHistory.jsm (28.70 KB, patch)
2011-11-28 22:11 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
AsyncFormHistory.jsm (28.53 KB, patch)
2011-11-29 03:06 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
AsyncFormHistory.jsm (28.85 KB, patch)
2011-11-29 16:48 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
AsyncFormHistory.jsm (29.05 KB, patch)
2011-12-05 04:28 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
AsyncFormHistory.jsm (29.80 KB, patch)
2011-12-06 16:42 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
Remove Form History Expiration Code from nsFormHistory.js (16.68 KB, patch)
2011-12-06 16:45 PST, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
Testing Expiration Code in AsyncFormHistory.jsm (41.24 KB, patch)
2011-12-06 16:48 PST, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
AsyncFormHistory.jsm (29.42 KB, patch)
2011-12-13 03:34 PST, Felix Fung (:felix)
dolske: review-
Details | Diff | Splinter Review
AsyncFormHistory.jsm (30.67 KB, patch)
2011-12-14 04:59 PST, Felix Fung (:felix)
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for test_db_corrupt.js (6.01 KB, patch)
2011-12-16 15:54 PST, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
Asynchronous FormHistory.jsm (31.41 KB, patch)
2011-12-16 19:15 PST, Felix Fung (:felix)
paul: review-
paul: feedback+
Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for sanitize.js (4.33 KB, patch)
2011-12-17 01:36 PST, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
(WIP) Use Asynchronous FormHistory.jsm for content/search.xml (4.83 KB, patch)
2011-12-22 03:58 PST, Felix Fung (:felix)
dolske: feedback+
Details | Diff | Splinter Review
Remove old migration tests (26.07 KB, patch)
2011-12-23 11:01 PST, Felix Fung (:felix)
dolske: review+
Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for test_db_update_v999* (9.57 KB, patch)
2011-12-31 22:10 PST, Felix Fung (:felix)
dolske: review-
Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js (25.03 KB, patch)
2011-12-31 23:39 PST, Felix Fung (:felix)
paul: review-
Details | Diff | Splinter Review
Asynchronous FormHistory.jsm (31.23 KB, patch)
2012-02-23 21:22 PST, Felix Fung (:felix)
paul: review-
Details | Diff | Splinter Review
Remove Form History Expiration Code from nsFormHistory.js, v2 (15.17 KB, patch)
2012-12-05 06:13 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Testing Expiration Code in AsyncFormHistory.jsm, v2 (39.17 KB, patch)
2012-12-05 06:17 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for test_db_corrupt.js, v2 (5.81 KB, patch)
2012-12-05 06:18 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for sanitize.js (19.19 KB, patch)
2012-12-05 06:28 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for content/search.xml, v2 (5.50 KB, patch)
2012-12-05 06:41 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Remove old migration tests, v2 (17.50 KB, patch)
2012-12-05 06:42 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for test_db_update_v999*, v2 (7.82 KB, patch)
2012-12-05 06:45 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js, v2 (27.62 KB, patch)
2012-12-05 06:47 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Asynchronous FormHistory.jsm, v2 (31.44 KB, patch)
2012-12-05 06:51 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Add deleted form history table (9.63 KB, patch)
2013-01-02 08:26 PST, Neil Deakin
no flags Details | Diff | Splinter Review
More tests (39.29 KB, patch)
2013-01-02 08:28 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Testing Expiration Code in AsyncFormHistory.jsm, v3 (39.26 KB, patch)
2013-02-11 10:40 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for test_db_corrupt.js, v3 (5.83 KB, patch)
2013-02-11 10:42 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for sanitize.js, v4 (14.73 KB, patch)
2013-02-11 10:44 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for content/search.xml, v3 (6.42 KB, patch)
2013-02-11 10:44 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for test_db_update_v999*, v3 (7.86 KB, patch)
2013-02-11 10:47 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js, v3 (28.02 KB, patch)
2013-02-11 10:47 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Asynchronous FormHistory.jsm, v3 (37.87 KB, patch)
2013-02-11 10:48 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Add deleted form history table, v2 (6.78 KB, patch)
2013-02-11 10:49 PST, Neil Deakin
no flags Details | Diff | Splinter Review
More tests, v2 (63.58 KB, patch)
2013-02-11 10:49 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Create new module FormHistory.jsm with handles asynchronous form history (37.03 KB, patch)
2013-02-25 12:59 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Part 1 - Create new module FormHistory.jsm with handles asynchronous form history (42.37 KB, patch)
2013-03-01 11:13 PST, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 2 - Remove expireOldEntries from nsFormHistory.js (15.17 KB, patch)
2013-03-01 14:11 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Part 3 - Changes to shared form history tests files to support asynchronous form history (7.66 KB, patch)
2013-03-01 14:12 PST, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 4 - AsyncFormHistory.jsm - Expiration Test (39.27 KB, patch)
2013-03-01 14:13 PST, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 5 - Use Asynchronous FormHistory.jsm for test_db_corrupt.js (5.25 KB, patch)
2013-03-01 14:13 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Part 6 - Use Asynchronous FormHistory.jsm for sanitize.js (25.85 KB, patch)
2013-03-01 14:14 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Part 7 - Use Asynchronous FormHistory.jsm for content/search.xml (8.23 KB, patch)
2013-03-01 14:15 PST, Neil Deakin
mak77: review-
Details | Diff | Splinter Review
Part 8 - Remove Tests for Old Form History Migrations (17.50 KB, patch)
2013-03-01 14:16 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Part 9 - Use Asynchronous FormHistory.jsm for test_db (6.89 KB, patch)
2013-03-01 14:17 PST, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 10 - Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js (30.46 KB, patch)
2013-03-01 14:17 PST, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 11 - support deleted table in form history (11.34 KB, patch)
2013-03-01 14:19 PST, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 12 - Main tests for asynchronous form history module FormHistory.jsm and changes to notification test (27.95 KB, patch)
2013-03-01 14:20 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Part 7.2 - Use Asynchronous FormHistory.jsm for content/search.xml (9.46 KB, patch)
2013-03-25 05:24 PDT, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 13 - convert to using { handleResult, handleError, handleCompletion} form for callbacks (61.20 KB, patch)
2013-03-27 09:56 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 1.2 - Create new module FormHistory.jsm with handles asynchronous form history (42.25 KB, patch)
2013-03-29 11:48 PDT, Neil Deakin
enndeakin: review+
Details | Diff | Splinter Review
Part 12.2 - Main tests for asynchronous form history module FormHistory.jsm and changes to notification test (29.71 KB, patch)
2013-03-29 11:51 PDT, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 13.2 - convert to using { handleResult, handleError, handleCompletion} form for callbacks (61.94 KB, patch)
2013-03-29 11:52 PDT, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 6.2 - Use Asynchronous FormHistory.jsm for sanitize.js (26.23 KB, patch)
2013-04-02 07:23 PDT, Neil Deakin
mak77: review+
Details | Diff | Splinter Review
Part 14 - implement asynchronous autcomplete on android (11.14 KB, patch)
2013-04-10 14:11 PDT, Neil Deakin
margaret.leibovic: review+
Details | Diff | Splinter Review
Have sanitize() return a promise (5.93 KB, patch)
2013-05-03 01:50 PDT, Mark Hammond [:markh]
gavin.sharp: feedback+
Details | Diff | Splinter Review
Part 6.2.1 - sanitize dialog and browser_bug409624.js wait until async operation complete (7.61 KB, patch)
2013-05-06 05:27 PDT, Mark Hammond [:markh]
mak77: review-
Details | Diff | Splinter Review
Part 6.2.1 - Updated per comments (10.00 KB, patch)
2013-05-08 06:39 PDT, Mark Hammond [:markh]
mak77: feedback+
Details | Diff | Splinter Review
Part 6.2.1 - Updated per new comments (10.38 KB, patch)
2013-05-08 16:09 PDT, Mark Hammond [:markh]
mak77: review+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2010-05-18 16:13:37 PDT
This probably involves a rewrite of nsIFormHistory2 and nsIFormAutocomplete APIs.
Comment 1 Shawn Wilsher :sdwilsh 2010-09-27 13:37:32 PDT
*** Bug 551976 has been marked as a duplicate of this bug. ***
Comment 2 Felix Fung (:felix) 2011-10-06 16:41:59 PDT
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?
Comment 3 :Margaret Leibovic 2011-10-06 19:03:49 PDT
(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)
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-07 11:57:48 PDT
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.
Comment 5 Felix Fung (:felix) 2011-10-11 14:54:47 PDT
Created attachment 566363 [details] [diff] [review]
WIP - Async draft for nsFormHistory.js

- 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)
Comment 6 Felix Fung (:felix) 2011-10-12 07:13:05 PDT
Created attachment 566521 [details] [diff] [review]
WIP2 - Async draft for nsFormHistory.js

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?
Comment 7 Marco Bonardo [::mak] 2011-10-12 07:34:04 PDT
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 8 Marco Bonardo [::mak] 2011-10-12 07:38:07 PDT
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.
Comment 9 Felix Fung (:felix) 2011-10-13 05:27:24 PDT
Created attachment 566786 [details] [diff] [review]
WIP3 - Async draft for nsFormHistory.js

Additional Changes:
- Batch uses begin/commitTransaction
- Fixed some handlers to use handleCompletion instead of handleResult
- Replaced getExistingID with waitForExistingID (async call)
Comment 10 Felix Fung (:felix) 2011-10-13 05:31:39 PDT
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
Comment 11 Felix Fung (:felix) 2011-10-13 12:11:22 PDT
Created attachment 566904 [details] [diff] [review]
nsIAsyncFormHistory

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.
Comment 12 Felix Fung (:felix) 2011-10-17 12:54:48 PDT
aSearchData format needs to be better specified. Most notably, how would 'removeEntriesByTimeframe' look like?
Comment 13 Felix Fung (:felix) 2011-10-18 11:43:07 PDT
Created attachment 567812 [details] [diff] [review]
nsIAsyncFormHistory

Additional Changes:
- Got rid of XPCOM interface
- Added pseudo-selectors (beginTime/endTime) for API
Comment 14 Felix Fung (:felix) 2011-10-18 11:47:25 PDT
Created attachment 567816 [details] [diff] [review]
nsIAsyncFormHistory - sanitize.js

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.
Comment 15 Marco Bonardo [::mak] 2011-10-18 14:44:25 PDT
Dolske, if your review queue is loaded I'd be happy to do a first-pass, if you wish.
Comment 16 Felix Fung (:felix) 2011-10-25 11:18:59 PDT
Comment on attachment 567812 [details] [diff] [review]
nsIAsyncFormHistory

Talked to Dolske, would love if you could do a first pass
Comment 17 Marco Bonardo [::mak] 2011-10-27 07:53:34 PDT
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 18 Marco Bonardo [::mak] 2011-10-27 16:20:56 PDT
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 = [];

= {}
Comment 19 Felix Fung (:felix) 2011-11-04 13:35:04 PDT
Created attachment 572061 [details] [diff] [review]
AsyncFormHistory.jsm

- 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.
Comment 20 Felix Fung (:felix) 2011-11-04 13:38:15 PDT
Created attachment 572064 [details] [diff] [review]
Use AsyncFormHistory.jsm - sanitize.js

- 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.)
Comment 21 Felix Fung (:felix) 2011-11-04 13:40:37 PDT
Created attachment 572065 [details] [diff] [review]
Use AsyncFormHistory.jsm - test/browser_sanitize-timespans.js

- Replaced all usage of nsIFormHistory2 with AsyncFormHistory
- This test passes, but leaks
Comment 22 Shawn Wilsher :sdwilsh 2011-11-06 15:40:29 PST
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`.
Comment 23 Felix Fung (:felix) 2011-11-10 13:12:45 PST
Created attachment 573623 [details] [diff] [review]
AsyncFormHistory.jsm

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) )
Comment 24 Felix Fung (:felix) 2011-11-10 13:14:32 PST
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...
Comment 25 Richard Newman [:rnewman] 2011-11-11 07:04:40 PST
(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 26 Justin Dolske [:Dolske] 2011-11-15 17:18:27 PST
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 27 Marco Bonardo [::mak] 2011-11-21 03:25:40 PST
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 28 Marco Bonardo [::mak] 2011-11-26 04:13:09 PST
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 29 Marco Bonardo [::mak] 2011-11-26 05:22:29 PST
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.
Comment 30 Richard Newman [:rnewman] 2011-11-26 06:14:53 PST
(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.
Comment 31 Felix Fung (:felix) 2011-11-28 13:23:26 PST
Created attachment 577349 [details] [diff] [review]
AsyncFormHistory.jsm

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.
Comment 32 Felix Fung (:felix) 2011-11-28 13:25:42 PST
@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?
Comment 33 Marco Bonardo [::mak] 2011-11-28 13:38:44 PST
(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?
Comment 34 Felix Fung (:felix) 2011-11-28 13:42:11 PST
Created attachment 577356 [details] [diff] [review]
Use AsyncFormHistory.jsm - sanitize.js

Additional Changes:
- Used defineLazyModuleGetter
- Update API call
Comment 35 Felix Fung (:felix) 2011-11-28 13:49:54 PST
@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.
Comment 36 Marco Bonardo [::mak] 2011-11-28 13:59:50 PST
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
Comment 37 Richard Newman [:rnewman] 2011-11-28 14:51:30 PST
(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!
Comment 38 Felix Fung (:felix) 2011-11-28 22:11:19 PST
Created attachment 577494 [details] [diff] [review]
AsyncFormHistory.jsm

Additional Changes:
- Removed 'removeAllEntries' call
- Added statement memoization
-- As a result, we can't guarantee the order of the changes in updatePlaces anymore...
Comment 39 Felix Fung (:felix) 2011-11-28 22:14:26 PST
(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...
Comment 40 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-11-28 23:22:54 PST
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.
Comment 41 Felix Fung (:felix) 2011-11-29 03:06:15 PST
Created attachment 577540 [details] [diff] [review]
AsyncFormHistory.jsm

Additional Changes:
- Implemented mozIStorageVacuumParticipant
- Renamed DBConnection -> databaseConnection (to match above interface)
- Registered with VacuumManager
Comment 42 Marco Bonardo [::mak] 2011-11-29 09:06:32 PST
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 43 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-29 11:28:03 PST
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).
Comment 44 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-29 11:30:05 PST
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.
Comment 45 Marco Bonardo [::mak] 2011-11-29 15:16:01 PST
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.
Comment 46 Felix Fung (:felix) 2011-11-29 16:48:14 PST
Created attachment 577799 [details] [diff] [review]
AsyncFormHistory.jsm

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.
Comment 47 Felix Fung (:felix) 2011-11-29 16:50:04 PST
@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...
Comment 48 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-12-01 17:31:55 PST
(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>
Comment 49 Marco Bonardo [::mak] 2011-12-02 05:08:39 PST
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.
Comment 50 Felix Fung (:felix) 2011-12-05 04:28:54 PST
Created attachment 579029 [details] [diff] [review]
AsyncFormHistory.jsm

Additional Changes:
- Added notifications
- Removed a few async throws that wouldn't be caught
Comment 51 Felix Fung (:felix) 2011-12-06 16:42:52 PST
Created attachment 579525 [details] [diff] [review]
AsyncFormHistory.jsm

Additional Changes:
- Bugfix: notifications need nsISupport data
- Bugfix: count
Comment 52 Felix Fung (:felix) 2011-12-06 16:45:13 PST
Created attachment 579526 [details] [diff] [review]
Remove Form History Expiration Code from nsFormHistory.js

Summary of Changes:
- Removed the expiration code from nsFormHistory.js which would conflict with expiry code contained in AsyncFormHistory.jsm
Comment 53 Felix Fung (:felix) 2011-12-06 16:48:04 PST
Created attachment 579529 [details] [diff] [review]
Testing Expiration Code 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 54 Justin Dolske [:Dolske] 2011-12-12 18:55:44 PST
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.)
Comment 55 Felix Fung (:felix) 2011-12-13 03:34:49 PST
Created attachment 581219 [details] [diff] [review]
AsyncFormHistory.jsm

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...)
Comment 56 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-13 11:22:10 PST
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 57 Marco Bonardo [::mak] 2011-12-13 14:51:26 PST
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 58 Marco Bonardo [::mak] 2011-12-13 14:57:15 PST
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 59 Marco Bonardo [::mak] 2011-12-13 15:17:43 PST
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 60 Justin Dolske [:Dolske] 2011-12-13 17:25:56 PST
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?
Comment 61 Justin Dolske [:Dolske] 2011-12-13 17:29:10 PST
(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.
Comment 62 Justin Dolske [:Dolske] 2011-12-13 17:42:06 PST
(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 63 Justin Dolske [:Dolske] 2011-12-13 19:17:33 PST
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 64 Justin Dolske [:Dolske] 2011-12-13 19:21:55 PST
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 65 Justin Dolske [:Dolske] 2011-12-13 19:34:30 PST
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 66 Justin Dolske [:Dolske] 2011-12-13 19:44:12 PST
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.
Comment 67 Marco Bonardo [::mak] 2011-12-14 03:20:00 PST
(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.
Comment 68 Felix Fung (:felix) 2011-12-14 04:59:51 PST
Created attachment 581595 [details] [diff] [review]
AsyncFormHistory.jsm

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
Comment 69 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-12-14 05:39:55 PST
This part still looks problematic:

495	  // FELIX
496	  dbClose();
497	  dbFile.remove(false);

dbClose uses asyncClose, but we immediately delete the file.
Comment 70 Felix Fung (:felix) 2011-12-16 15:54:15 PST
Created attachment 582423 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_db_corrupt.js

Like it says on the tin!
Comment 71 Felix Fung (:felix) 2011-12-16 19:15:54 PST
Created attachment 582465 [details] [diff] [review]
Asynchronous FormHistory.jsm

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
Comment 72 Felix Fung (:felix) 2011-12-17 01:36:05 PST
Created attachment 582505 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for sanitize.js

Additional Changes:
- Updated caller to match API updates
- In get canClear() for form history, make a blocking call to FormHistory.count.
Comment 73 Felix Fung (:felix) 2011-12-21 13:22:45 PST
Turns out, our migrators are written in C++ and use form-history. Is that a good enough reason to add some XPCOM?
Comment 74 Marco Bonardo [::mak] 2011-12-21 13:29:01 PST
(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).
Comment 75 Felix Fung (:felix) 2011-12-22 03:58:34 PST
Created attachment 583757 [details] [diff] [review]
(WIP) Use Asynchronous FormHistory.jsm for content/search.xml

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 76 Felix Fung (:felix) 2011-12-23 11:01:46 PST
Created attachment 584088 [details] [diff] [review]
Remove old migration tests
Comment 77 Justin Dolske [:Dolske] 2011-12-29 19:30:03 PST
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 78 Justin Dolske [:Dolske] 2011-12-29 19:46:28 PST
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 79 Justin Dolske [:Dolske] 2011-12-29 19:50:56 PST
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 80 Justin Dolske [:Dolske] 2011-12-29 19:52:42 PST
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.
Comment 81 Philipp von Weitershausen [:philikon] 2011-12-29 21:32:14 PST
(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.
Comment 82 Felix Fung (:felix) 2011-12-31 22:10:02 PST
Created attachment 585178 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_db_update_v999*

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
Comment 83 Felix Fung (:felix) 2011-12-31 23:39:06 PST
Created attachment 585181 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js

Changes:
- Used blocking_call, simplified the patch alot
Comment 84 Philipp von Weitershausen [:philikon] 2012-01-01 03:08:15 PST
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.
Comment 85 Felix Fung (:felix) 2012-01-01 10:21:25 PST
(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.
Comment 86 Richard Newman [:rnewman] 2012-01-01 11:07:03 PST
(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 87 Justin Dolske [:Dolske] 2012-01-17 14:23:51 PST
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 88 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-01-26 15:17:16 PST
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)
Comment 89 Marco Bonardo [::mak] 2012-01-26 15:39:11 PST
(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 90 Justin Dolske [:Dolske] 2012-02-01 16:19:57 PST
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).
Comment 91 Dietrich Ayala (:dietrich) 2012-02-08 09:20:44 PST
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 92 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-08 10:54:10 PST
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.
Comment 93 Felix Fung (:felix) 2012-02-23 21:22:35 PST
Created attachment 600303 [details] [diff] [review]
Asynchronous FormHistory.jsm

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 94 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-24 16:21:52 PST
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.
Comment 95 Felix Fung (:felix) 2012-02-25 12:55:21 PST
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.
Comment 96 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-27 18:37:55 PST
(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 97 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-02-27 18:38:55 PST
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 98 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-03-27 16:18:02 PDT
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+.
Comment 99 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-09-27 13:55:06 PDT
*** Bug 795095 has been marked as a duplicate of this bug. ***
Comment 100 Neil Deakin 2012-12-05 06:13:26 PST
Created attachment 688737 [details] [diff] [review]
Remove Form History Expiration Code from nsFormHistory.js, v2

Updated patch which differs only by:

- updating message manager interface to due to other changes
- change to use 2.0 license
Comment 101 Neil Deakin 2012-12-05 06:17:01 PST
Created attachment 688738 [details] [diff] [review]
Testing Expiration Code in AsyncFormHistory.jsm, v2

Address comment 59
Comment 102 Neil Deakin 2012-12-05 06:18:28 PST
Created attachment 688739 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_db_corrupt.js, v2

Address comment 77
Comment 103 Neil Deakin 2012-12-05 06:28:00 PST
Created attachment 688740 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for sanitize.js

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
Comment 104 Neil Deakin 2012-12-05 06:41:24 PST
Created attachment 688747 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for content/search.xml, v2

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
Comment 105 Neil Deakin 2012-12-05 06:42:28 PST
Created attachment 688748 [details] [diff] [review]
Remove old migration tests, v2

This patch differs by:

- updating to 2.0 license
- remove tests from xpcshell.ini
Comment 106 Neil Deakin 2012-12-05 06:45:50 PST
Created attachment 688749 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_db_update_v999*, v2

Unchanged, but updated for license changes
Comment 107 Neil Deakin 2012-12-05 06:47:30 PST
Created attachment 688751 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js, v2

Change test to work asynchronously
Comment 108 Neil Deakin 2012-12-05 06:51:43 PST
Created attachment 688753 [details] [diff] [review]
Asynchronous FormHistory.jsm, v2

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
Comment 109 Neil Deakin 2012-12-05 06:53:28 PST
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 110 Dão Gottwald [:dao] 2012-12-05 06:57:07 PST
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 111 Neil Deakin 2012-12-06 06:27:30 PST
Comment on attachment 688753 [details] [diff] [review]
Asynchronous FormHistory.jsm, v2

I'll address Dao's comment 110 in the next iteration.
Comment 112 Neil Deakin 2012-12-19 06:26:17 PST
(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?
Comment 113 Dão Gottwald [:dao] 2012-12-19 08:38:19 PST
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.
Comment 114 Neil Deakin 2012-12-19 09:15:40 PST
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().
Comment 115 Neil Deakin 2013-01-02 08:26:48 PST
Created attachment 697035 [details] [diff] [review]
Add deleted form history table

This patch adds the deleted form history support that was added to nsFormHistory by bug 725881.
Comment 116 Neil Deakin 2013-01-02 08:28:09 PST
Created attachment 697037 [details] [diff] [review]
More tests

This patch converts various synchronous tests to use the new FormHistory module and adds more asynchronous tests to test_history_api.js.
Comment 117 Marco Bonardo [::mak] 2013-01-23 07:05:05 PST
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 118 Marco Bonardo [::mak] 2013-01-23 13:08:19 PST
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
Comment 119 Marco Bonardo [::mak] 2013-01-23 13:14:57 PST
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 120 Marco Bonardo [::mak] 2013-01-23 13:23:23 PST
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 121 Marco Bonardo [::mak] 2013-01-23 13:29:30 PST
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.
Comment 122 Justin Dolske [:Dolske] 2013-01-23 16:05:59 PST
(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 123 Marco Bonardo [::mak] 2013-01-24 12:55:41 PST
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.
Comment 124 Neil Deakin 2013-02-11 10:40:16 PST
Created attachment 712519 [details] [diff] [review]
Testing Expiration Code in AsyncFormHistory.jsm, v3
Comment 125 Neil Deakin 2013-02-11 10:42:48 PST
Created attachment 712520 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_db_corrupt.js, v3
Comment 126 Neil Deakin 2013-02-11 10:44:04 PST
Created attachment 712522 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for sanitize.js, v4
Comment 127 Neil Deakin 2013-02-11 10:44:39 PST
Created attachment 712523 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for content/search.xml, v3
Comment 128 Neil Deakin 2013-02-11 10:47:30 PST
Created attachment 712524 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for test_db_update_v999*, v3
Comment 129 Neil Deakin 2013-02-11 10:47:59 PST
Created attachment 712525 [details] [diff] [review]
Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js, v3
Comment 130 Neil Deakin 2013-02-11 10:48:30 PST
Created attachment 712527 [details] [diff] [review]
Asynchronous FormHistory.jsm, v3
Comment 131 Neil Deakin 2013-02-11 10:49:11 PST
Created attachment 712528 [details] [diff] [review]
Add deleted form history table, v2
Comment 132 Neil Deakin 2013-02-11 10:49:47 PST
Created attachment 712529 [details] [diff] [review]
More tests, v2
Comment 133 Neil Deakin 2013-02-11 10:56:29 PST
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 134 Dão Gottwald [:dao] 2013-02-11 14:05:47 PST
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 135 Marco Bonardo [::mak] 2013-02-13 08:16:09 PST
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 136 Marco Bonardo [::mak] 2013-02-13 08:49:04 PST
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 137 Marco Bonardo [::mak] 2013-02-13 08:50:40 PST
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.
Comment 138 Neil Deakin 2013-02-13 12:31:43 PST
> 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.
Comment 139 Marco Bonardo [::mak] 2013-02-13 12:52:24 PST
(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.
Comment 140 Neil Deakin 2013-02-20 05:12:54 PST
> 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.
Comment 141 Marco Bonardo [::mak] 2013-02-20 05:22:36 PST
sure, autoflagging to rememeber to do that
Comment 142 Marco Bonardo [::mak] 2013-02-21 05:48:57 PST
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
Comment 143 Neil Deakin 2013-02-25 12:59:02 PST
Created attachment 718032 [details] [diff] [review]
Create new module FormHistory.jsm with handles asynchronous form history
Comment 144 Neil Deakin 2013-02-25 13:06:08 PST
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.
Comment 145 Marco Bonardo [::mak] 2013-02-26 14:49:56 PST
(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.
Comment 146 Neil Deakin 2013-02-27 09:31:25 PST
> 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.
Comment 147 Neil Deakin 2013-03-01 11:13:28 PST
Created attachment 720050 [details] [diff] [review]
Part 1 - Create new module FormHistory.jsm with handles asynchronous form history
Comment 148 Neil Deakin 2013-03-01 14:11:44 PST
Created attachment 720127 [details] [diff] [review]
Part 2 - Remove expireOldEntries from nsFormHistory.js

This patch was already reviewed.
Comment 149 Neil Deakin 2013-03-01 14:12:18 PST
Created attachment 720129 [details] [diff] [review]
Part 3 - Changes to shared form history tests files to support asynchronous form history
Comment 150 Neil Deakin 2013-03-01 14:13:04 PST
Created attachment 720130 [details] [diff] [review]
Part 4 - AsyncFormHistory.jsm - Expiration Test
Comment 151 Neil Deakin 2013-03-01 14:13:47 PST
Created attachment 720131 [details] [diff] [review]
Part 5 - Use Asynchronous FormHistory.jsm for test_db_corrupt.js
Comment 152 Neil Deakin 2013-03-01 14:14:39 PST
Created attachment 720132 [details] [diff] [review]
Part 6 - Use Asynchronous FormHistory.jsm for sanitize.js
Comment 153 Neil Deakin 2013-03-01 14:15:23 PST
Created attachment 720133 [details] [diff] [review]
Part 7 - Use Asynchronous FormHistory.jsm for content/search.xml
Comment 154 Neil Deakin 2013-03-01 14:16:11 PST
Created attachment 720134 [details] [diff] [review]
Part 8 - Remove Tests for Old Form History Migrations

This patch has already been reviewed.
Comment 155 Neil Deakin 2013-03-01 14:17:11 PST
Created attachment 720136 [details] [diff] [review]
Part 9 - Use Asynchronous FormHistory.jsm for test_db
Comment 156 Neil Deakin 2013-03-01 14:17:43 PST
Created attachment 720137 [details] [diff] [review]
Part 10 - Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js
Comment 157 Neil Deakin 2013-03-01 14:19:27 PST
Created attachment 720138 [details] [diff] [review]
Part 11 - support deleted table in form history
Comment 158 Neil Deakin 2013-03-01 14:20:04 PST
Created attachment 720139 [details] [diff] [review]
Part 12 - Main tests for asynchronous form history module FormHistory.jsm and changes to notification test
Comment 159 Neil Deakin 2013-03-01 14:21:55 PST
The three additional patches that are part of bug 697377 I will post later. The patches above are reordered a bit to combine changes from similar files together.
Comment 160 Marco Bonardo [::mak] 2013-03-13 14:33:32 PDT
Comment on attachment 720050 [details] [diff] [review]
Part 1 - Create new module FormHistory.jsm with handles asynchronous form history

Review of attachment 720050 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/FormHistory.jsm
@@ +4,5 @@
> +
> +/**
> + * FormHistory
> + *
> + * Used to stored values that have been entered into forms which may later

typo: "stored"

@@ +278,5 @@
> +    log("removeEntries");
> +    query += " WHERE " + queryTerms;
> +  } else {
> +    log("removeAllEntries");
> +    // Not specifying any fields means we should remove remove all entries. We

typo: remove remove

@@ +285,5 @@
> +
> +  return dbCreateAsyncStatement(query, aSearchData, aBindingArrays);
> +}
> +
> +function makeModifyStatement(aGuid, aNewData, aBindingArrays) {

I wonder if op="modify" should rather be op="update" (and by that change all "modify" to "update" in this module). It's a minor thing, but update is a more common term used in interfaces. What do you think?

@@ +480,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

Are these versions right?

@@ +632,5 @@
> +  }
> +
> +  let handlers = {
> +    handleCompletion :
> +      function updateFormHistoryCompletionHandler(aReason) {

the indentation here is a bit fancy, I don't see a reason this couldn't be onelined

@@ +645,5 @@
> +          }
> +        }
> +      },
> +    handleError :
> +      function updateFormHistoryErrorHandler(aError) {

ditto

@@ +1016,5 @@
> +    return dbConnection.schemaVersion;
> +  },
> +
> +  vacuum: function vacuum() {
> +    dbConnection.executeSimpleSQL("VACUUM");

does this really have to be synchronous? It's quite bad :(
what is it used for and why do we have to expose it? I don't see a valid case for consumers to request a vacuum, it should just be handled internally imo (I actually think it should not even be handled as it is currently, but that's out of scope).
Comment 161 Marco Bonardo [::mak] 2013-03-13 14:44:31 PDT
Comment on attachment 720129 [details] [diff] [review]
Part 3 - Changes to shared form history tests files to support asynchronous form history

Review of attachment 720129 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/test/satchel_common.js
@@ +105,4 @@
>  
> +    countEntries(expected.name, expected.value,
> +      function(num) {
> +        ok(num > 0, expected.message); 

trailing space
Comment 162 Marco Bonardo [::mak] 2013-03-13 14:59:23 PDT
Comment on attachment 720130 [details] [diff] [review]
Part 4 - AsyncFormHistory.jsm - Expiration Test

Review of attachment 720130 [details] [diff] [review]:
-----------------------------------------------------------------

As a general note, you are using generators here, a more efficient way to write readable async tests is by using add_task. It may look strange at the beginning, but it's really good in the end.
Not mandatory clearly since these are already written and we could cleanup them later, I just feel like these tests would be far easier with it.

::: toolkit/components/satchel/test/unit/test_async_expire.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/FormHistory.jsm");

These could probably be imported in the head file, since I expect most/all tests will use them.

@@ +11,5 @@
> +
> +function addEntry(fieldname, value, then) {
> +  let now = Date.now() * 1000;
> +  FormHistory.update({op: "add", fieldname: fieldname, value: value, timesUsed: 1,
> +                                    firstUsed: now, lastUsed: now },

broken indentation

@@ +25,5 @@
> +                                onFailure: function (error) {
> +                                  do_throw("Error occurred updating form history: " + error);
> +                                }
> +                              });
> +}

addEntry and update (the names could be a bit more coherent) could be moved to head file as well.

@@ +31,5 @@
> +function triggerExpiration() {
> +  // We can't easily fake a "daily idle" event, so for testing purposes form
> +  // history listens for another notification to trigger an immediate
> +  // expiration.
> +  Services.obs.notifyObservers(null, "formhistory-expire-now", null);

well, you could send idle-daily to the startup component observe method, I suppose

@@ +105,5 @@
> +
> +  // Check the original db size.
> +  // Do a vacuum to make sure the db has current page size, so that a comparison
> +  // of the old and new size can be made later.
> +  FormHistory.vacuum();

Ah, I see what's the vacuum for...
I honestly don't think what we are testing here is worth the cruft that an add-on using this vacuum API may cause, especially since I think we should change the vacuum behavior here shortly, I'd skip testing the db size for now and remove that vacuum pseudo api.
Comment 163 Marco Bonardo [::mak] 2013-03-13 15:15:14 PDT
Comment on attachment 720133 [details] [diff] [review]
Part 7 - Use Asynchronous FormHistory.jsm for content/search.xml

Review of attachment 720133 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/search/content/search.xml
@@ +111,5 @@
>        <field name="_engines">null</field>
> +      <field name="FormHistory" readonly="true">
> +        let formHistory = {};
> +        Components.utils.import("resource://gre/modules/FormHistory.jsm", formHistory);
> +        formHistory.FormHistory;

This should work:
(Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;

@@ +450,5 @@
> +              }, {
> +                onFailure : function(aError) {
> +                  Components.utils.reportError("Saving search to form history failed: " + aError.message);
> +                }
> +              });

nit: I'd like to suggest a more compact indentation:

this.FormHistory.update(
  { op: "bump",
    fieldname: textBox.getAttribute("autocompletesearchparam"),
    value: textValue },
  { onFailure: function(aError) {
      Components.utils.reportError("Saving search to form history failed: " + aError.message);
  });

@@ +738,1 @@
>            return true;

we could probably still do the async update using FormHistory.count (updating commands again when the callback is invoked)... But I agree with removing the complication, having clear history always enabled is probably not an issue here, the search box is used often so in vast majority of cases would be enabled, and one is likely to use this command to cleanup something, than at random times.

@@ +742,5 @@
>            switch (aCommand) {
>              case "cmd_clearhistory":
>                var param = this._self.getAttribute("autocompletesearchparam");
> +              this.FormHistory.update(
> +                { op : "bump", fieldname : param }, null);

should this be "remove"? if the test passes like this something may be wrong with it... or there isn't a test.

::: browser/components/search/test/browser_426329.js
@@ +4,5 @@
>  this._scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>                       getService(Ci.mozIJSSubScriptLoader);
>  this._scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", chromeUtils);
>  
> +let FormHistory = SpecialPowers.formHistory;

I don't think we need to use SpecialPowers in browser-chrome tests...
Comment 164 Marco Bonardo [::mak] 2013-03-13 15:30:57 PDT
Comment on attachment 720138 [details] [diff] [review]
Part 11 - support deleted table in form history

Review of attachment 720138 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit scared by the fact there isn't a test verifying things are being properly added to the deleted table, though making one would require to change the thing so that instead of excluding code the ifdef just defined a bool var, and we skip the code (so that the test may somehow set the var and test the thing).
I assume proper handling was tested manually for now.

::: toolkit/components/satchel/test/unit/test_db_update_v4.js
@@ +4,5 @@
>  
>  var testnum = 0;
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/FormHistory.jsm");

move to head

@@ +21,5 @@
>    try {
>  
>    // ===== test init =====
>    var testfile = do_get_file("formhistory_v3.sqlite");
> +  var profileDir = dirSvc.get("ProfD", Components.interfaces.nsIFile);

Why the change from Ci to Components.interfaces? I suppose the head defines Ci

@@ +46,3 @@
>    // Check that the index was added
> +  do_check_true(dbConnection.tableExists("moz_deleted_formhistory"));
> +  yield dbConnection.asyncClose(function () iter.next());

since you only use synchronous APIs here, you may just call synchronous .close()

::: toolkit/components/satchel/test/unit/test_db_update_v4b.js
@@ +4,5 @@
>  
>  var testnum = 0;
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/FormHistory.jsm");

ditto

@@ +21,5 @@
>    try {
>  
>    // ===== test init =====
>    var testfile = do_get_file("formhistory_v3v4.sqlite");
> +  var profileDir = dirSvc.get("ProfD", Components.interfaces.nsIFile);

ditto

@@ +46,3 @@
>    // Check that the index was added
> +  do_check_true(dbConnection.tableExists("moz_deleted_formhistory"));
> +  yield dbConnection.asyncClose(function () iter.next());

ditto
Comment 165 Marco Bonardo [::mak] 2013-03-13 15:33:28 PDT
Comment on attachment 720136 [details] [diff] [review]
Part 9 - Use Asynchronous FormHistory.jsm for test_db

Review of attachment 720136 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/test/unit/test_db_update_v999a.js
@@ +13,5 @@
>  
> +let Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FormHistory.jsm");

head :)

@@ +76,4 @@
>  
>    } catch (e) {
>      throw "FAILED in test #" + testnum + " -- " + e;
>    }

shouldn't do_test_finished invoked after the catch?

::: toolkit/components/satchel/test/unit/test_db_update_v999b.js
@@ +13,5 @@
>  
> +let Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FormHistory.jsm");

ditto

@@ +95,5 @@
> +  yield updateEntry("remove", "name-A", "value-A", next_test);
> +  yield countEntries(null, null, checkZero);
> +  yield countEntries("name-A", "value-A", checkZero);
> +
> +  do_test_finished();

ditto
Comment 166 Marco Bonardo [::mak] 2013-03-14 08:34:35 PDT
Comment on attachment 720137 [details] [diff] [review]
Part 10 - Use Asynchronous FormHistory.jsm for browser_sanitize-timespans.js

Review of attachment 720137 [details] [diff] [review]:
-----------------------------------------------------------------

could probably be improved here and there, but the logic looks ok

::: browser/base/content/test/browser_sanitize-timespans.js
@@ +32,5 @@
> +function promiseFormHistoryRemoved()
> +{
> +  formHistoryPromise = Promise.defer();
> +  return formHistoryPromise.promise;
> +}

why couldn't we just avoid all of these globals and do:

function promiseFormHistoryRemoved() {
  let deferred = Promise.defer();
  Services.obs.addObserver(function onfh() {
    Services.obs.removeObserver(onfh, "satchel-storage-changed", false);
    deferred.resolve();
  }, "satchel-storage-changed", false);
  return deferred.promise;
}

@@ +54,2 @@
>    });
>  }

I think something like this would be cleaner:

function test() {
  waitForExplicitFinish();

  Task.spawn(function() {
    yield setupDownloads();
    yield setupFormHistory();
    yield setupHistory();
    yield onHistoryRead();
  }).then(finish);
}

it would just requires some changes to setupHistory (should return a promise instead of getting a callback)

@@ +67,5 @@
> +                             deferred.reject(error)
> +                           }
> +                         });
> +  return deferred.promise;
> +}

fwiw, you may pass out the count and do checks like is(yield countEntries(name), 0, msg); without hiding the logic... not that matters much.
Comment 167 Marco Bonardo [::mak] 2013-03-22 05:48:26 PDT
Comment on attachment 720132 [details] [diff] [review]
Part 6 - Use Asynchronous FormHistory.jsm for sanitize.js

Review of attachment 720132 [details] [diff] [review]:
-----------------------------------------------------------------

At first glance, this would probably be cleaner using a task to go through the canClear so that it can yield over them and async ones may return promises... should be cleaner as a whole but I\m not sure if that would require deeper changes to the external pseudo-API.

::: browser/base/content/sanitize.js
@@ +54,5 @@
>      else
>        range = this.range || Sanitizer.getClearRange();
>        
>      for (var itemName in this.items) {
> +      var self = this;

could you please use bind(this) instead?

@@ +82,3 @@
>        }
>      }
>      return errors;

we fill errors it in the callback function, that means for async canClearItem it is not tracked. As a consequence all of this seems a bit fragile, maybe the error reporting method should change

@@ +255,5 @@
> +            firstUsedEnd : this.range[1],
> +          });
> +        } else {
> +          FormHistory.update({ op : "remove" });
> +        }

I'd suggest to build the object with op:"remove", if there's a range add the firstUsed properties to it, and in any case call FormHistory.update

@@ +272,5 @@
>              if (searchBar.value ||
>                  transactionMgr.numberOfUndoItems ||
> +                transactionMgr.numberOfRedoItems) {
> +              aCallback("formdata", true, aArg);
> +              return;

just for coherence with other canClear functions, should return false synchronously (and the proper value asynchronously).
also in the other return points.

@@ +289,5 @@
> +            aCallback("formdata", aNumEntries > 0, aArg);
> +          },
> +          onFailure : function(aError) {
> +            aCallback("formdata", false, aArg);
> +          }

I'm not totally happy with the callback format, looks like it may rather be just { complete(aStatus, aValues) } or a more complete (but more coherent with the rest of the codebase) {handleError, handleResult, handleCompletion}
onSuccess, onFailure looks dangerously similar to promises .then handlers, while it's not. I suggest asking Gavin for SR on this

::: browser/base/content/sanitizeDialog.js
@@ +47,5 @@
>      for (let i = 0; i < sanitizeItemList.length; i++) {
>        let prefItem = sanitizeItemList[i];
>        let name = s.getNameFromPreference(prefItem.getAttribute("preference"));
> +      s.canClearItem(name, function canClearCallback(aItem, aResult, aPrefItem) {
> +        if (!aResult) {

s/aResult/aCanClear/

@@ +283,5 @@
>      for (let i = 0; i < sanitizeItemList.length; i++) {
>        let prefItem = sanitizeItemList[i];
>        let name = s.getNameFromPreference(prefItem.getAttribute("preference"));
> +      s.canClearItem(name, function canClearCallback(aResult) {
> +        if (!aResult) {

ditto

::: browser/base/content/test/browser_bug409624.js
@@ +6,5 @@
>    waitForExplicitFinish();
>  
> +  // This test relies on the form history being empty to start with so delete
> +  // all the items first.
> +  SpecialPowers.formHistory.update({ op: "remove" },

do we need SpecialPowers in b-c tests?
Comment 168 Marco Bonardo [::mak] 2013-03-22 05:57:05 PDT
Comment on attachment 720139 [details] [diff] [review]
Part 12 - Main tests for asynchronous form history module FormHistory.jsm and changes to notification test

Review of attachment 720139 [details] [diff] [review]:
-----------------------------------------------------------------

xpcshell-tests should use add_task, not other customized async runners, we have far too many custom runners in the tree and soon or later there will be some bug to remove them, let's try to not make things worse for it

::: toolkit/components/satchel/test/unit/test_history_api.js
@@ +10,5 @@
>  
> +function supportsDeletedTable()
> +{
> +  // XXXndeakin The deleted table is only supported on Android which doesn't run this test.
> +  return false;

so, looks like you may want to either ifdef stuff, or use xpcshell.ini and duplicate the test having a version that runs only on android and one that runs everywhere else
Comment 169 Richard Newman [:rnewman] 2013-03-22 08:48:44 PDT
Comment on attachment 720132 [details] [diff] [review]
Part 6 - Use Asynchronous FormHistory.jsm for sanitize.js

Review of attachment 720132 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/sanitize.js
@@ +54,5 @@
>      else
>        range = this.range || Sanitizer.getClearRange();
>        
>      for (var itemName in this.items) {
> +      var self = this;

As a general point of joy, fat arrows are now in Nightly, so most cases of having to use bind(this) can be avoided.

http://wiki.ecmascript.org/doku.php?id=harmony:arrow_function_syntax

And as another general point: use `let`, not `var`, particularly for scoped stuff like this!
Comment 170 Neil Deakin 2013-03-25 05:24:38 PDT
Created attachment 728927 [details] [diff] [review]
Part 7.2 - Use Asynchronous FormHistory.jsm for content/search.xml
Comment 171 Neil Deakin 2013-03-25 06:22:21 PDT
Comment on attachment 720050 [details] [diff] [review]
Part 1 - Create new module FormHistory.jsm with handles asynchronous form history

Gavin, Marco suggests changing the callback api from 
{ onSuccess: function()., onFailure: function() }.

to one more like mozIStorageStatementCallback. Since the arguments would be different (not database-specific) maybe this might be more confusing. Thoughts?
Comment 172 Marco Bonardo [::mak] 2013-03-25 06:27:58 PDT
fwiw, apart Storage, contentPrefService2 and history also use { handleResult, handleError, handleCompletion } callbacks.
Comment 173 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-26 07:56:20 PDT
Comment on attachment 720050 [details] [diff] [review]
Part 1 - Create new module FormHistory.jsm with handles asynchronous form history

From a conversation on IRC, Marco just didn't like the middle ground of a mozIStorageStatementCallback-ish callback definition that didn't go all the way:

<mak> gavin: I think either we go the full package error,result,complete, or we try to simplify to the single function that is much better for consumers

As I understand it this API always returns all of its results at once, and there is no reason to believe we'd change it to returning data in chunks - assuming that's the case, I think it would be best to simplify and just move to a single callback function that takes both an nsresult/bool for status, and an array of results (possibly null):

callback(nsresult/bool status, results)

nsresult vs. bool I don't feel strongly about - bool is nicer from a pure-JS API point of view, but nsresult is a bit more expressive and is more conventional for Mozilla APIs.

(And if we do need to adjust the API later, shimming in compatibility for a different form of callbacks wouldn't be so bad.)
Comment 174 Marco Bonardo [::mak] 2013-03-26 08:43:04 PDT
Comment on attachment 728927 [details] [diff] [review]
Part 7.2 - Use Asynchronous FormHistory.jsm for content/search.xml

Review of attachment 728927 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/search/content/search.xml
@@ +739,5 @@
>                var param = this._self.getAttribute("autocompletesearchparam");
> +
> +              let searchBar = this._self.parentNode;
> +
> +              let fh = (Components.utils.import("resource://gre/modules/FormHistory.jsm", {}));

couldn't you use searchBar.FormHistory, or even BrowserSearch.searchBar.FormHistory here?

::: browser/components/search/test/browser_426329.js
@@ +4,5 @@
>  this._scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>                       getService(Ci.mozIJSSubScriptLoader);
>  this._scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", chromeUtils);
>  
> +let FormHistory = (Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;

nit: may use defineLazyModuleGetter
Comment 175 Neil Deakin 2013-03-27 09:56:23 PDT
Created attachment 730206 [details] [diff] [review]
Part 13 - convert to using { handleResult, handleError, handleCompletion} form for callbacks

This patch does as Marco suggests. I could also switch to a simpler form as Gavin suggests. This patch is not completely tested but all of the form history related tests pass locally.
Comment 176 Neil Deakin 2013-03-29 11:48:24 PDT
Created attachment 731249 [details] [diff] [review]
Part 1.2 - Create new module FormHistory.jsm with handles asynchronous form history
Comment 177 Neil Deakin 2013-03-29 11:51:59 PDT
Created attachment 731253 [details] [diff] [review]
Part 12.2 - Main tests for asynchronous form history module FormHistory.jsm and changes to notification test
Comment 178 Neil Deakin 2013-03-29 11:52:43 PDT
Created attachment 731254 [details] [diff] [review]
Part 13.2 - convert to using { handleResult, handleError, handleCompletion} form for callbacks
Comment 179 Neil Deakin 2013-04-02 07:23:21 PDT
Created attachment 732320 [details] [diff] [review]
Part 6.2 - Use Asynchronous FormHistory.jsm for sanitize.js

> At first glance, this would probably be cleaner using a task to go
> through the canClear so that it can yield over them and async ones may
> return promises... should be cleaner as a whole but I\m not sure if that
> would require deeper changes to the external pseudo-API.

I'll investigate this as a followup
Comment 180 Marco Bonardo [::mak] 2013-04-04 08:27:46 PDT
Comment on attachment 731253 [details] [diff] [review]
Part 12.2 - Main tests for asynchronous form history module FormHistory.jsm and changes to notification test

Review of attachment 731253 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/test/unit/test_history_api.js
@@ +376,5 @@
> +  yield promiseCountEntries("field4", "value4", checkNotExists);
> +  yield promiseCountEntries(null, null, function(num) do_check_eq(num, 2));
> +  yield countDeletedEntries(10);
> +
> +  dbConnection.asyncClose(do_test_finished);

may you please do this in the finally if dbConnection is defined?
Comment 181 Marco Bonardo [::mak] 2013-04-04 08:53:42 PDT
Comment on attachment 732320 [details] [diff] [review]
Part 6.2 - Use Asynchronous FormHistory.jsm for sanitize.js

Review of attachment 732320 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/sanitize.js
@@ +60,3 @@
>        item.range = range;
> +      if ("clear" in item && branch.getBoolPref(itemName)) {
> +        let clearCallback = (itemName, aResult) => {

nit: I'd still prefer a better name for aResult, like aCanClear.

@@ +60,5 @@
>        item.range = range;
> +      if ("clear" in item && branch.getBoolPref(itemName)) {
> +        let clearCallback = (itemName, aResult) => {
> +          // Some of these clear() may raise exceptions (see bug #265028)
> +          // to sanitize as much as possible, we catch and store them, 

trailing space

@@ +69,5 @@
> +          try {
> +            if (aResult)
> +              item.clear();
> +          } catch(er) {
> +            if (!errors) 

trailing space

@@ +255,5 @@
>  
> +        let change = { op: "remove" };
> +        if (this.range) {
> +          change.firstUsedStart = this.range[0];
> +          change.firstUsedEnd = this.range[1];

nit: [ change.firstUsedStart, change.firstUsedEnd ] = this.range;

::: browser/base/content/test/browser_bug409624.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   *  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/. */
>  
> +let FormHistory = (Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;

nit: defineLazyModuleGetter is easier (and won't leak)

::: browser/base/content/test/browser_sanitizeDialog.js
@@ +23,5 @@
>  Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader)
>                                             .loadSubScript("chrome://browser/content/sanitize.js", tempScope);
>  let Sanitizer = tempScope.Sanitizer;
>  
> +let FormHistory = (Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;

ditto
Comment 182 Marco Bonardo [::mak] 2013-04-04 09:37:16 PDT
Comment on attachment 731254 [details] [diff] [review]
Part 13.2 - convert to using { handleResult, handleError, handleCompletion} form for callbacks

Review of attachment 731254 [details] [diff] [review]:
-----------------------------------------------------------------

I like this experimenting with arrow functions :)

::: browser/base/content/sanitize.js
@@ +289,3 @@
>          let countDone = {
> +          handleResult : function(aResult) count = aResult,
> +          handleCompletion :

handleError with a cu.reportError would be nice, we should try to never swallow errors
Comment 183 Neil Deakin 2013-04-10 14:11:30 PDT
Created attachment 735965 [details] [diff] [review]
Part 14 - implement asynchronous autcomplete on android

This is the conversion of the android form history to be asynchronous. A bit confusing here as the android version is using a wrapper that I assumed was a separate implementation.

The tests seems to pass and autocomplete on a page appears to be works, but hopefully someone can test for sure.

Builds are https://tbpl.mozilla.org/?tree=Try&rev=b6e76df2d7a2
Comment 184 :Margaret Leibovic 2013-04-10 15:25:04 PDT
(In reply to Neil Deakin from comment #183)
> Created attachment 735965 [details] [diff] [review]
> Part 14 - implement asynchronous autcomplete on android
> 
> This is the conversion of the android form history to be asynchronous. A bit
> confusing here as the android version is using a wrapper that I assumed was
> a separate implementation.

It looks like our FormAutocomplete.js implementation was added for contacts support in bug 578691, and we don't actually use that for native fennec, so you can go ahead and kill that file! It's nice that you brought this to our attention :)

I can file a follow-up to kill the related unused code that goes along with that, since that's not your responsibility.

> The tests seems to pass and autocomplete on a page appears to be works, but
> hopefully someone can test for sure.
> 
> Builds are https://tbpl.mozilla.org/?tree=Try&rev=b6e76df2d7a2

Unfortunately, we don't have tests for this :/ However, I did try out the build on some test pages, and it works as expected (including the interaction with form validation), so things look good.
Comment 185 :Margaret Leibovic 2013-04-10 15:31:48 PDT
Comment on attachment 735965 [details] [diff] [review]
Part 14 - implement asynchronous autcomplete on android

Review of attachment 735965 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but you should just get rid of FormAutoComplete.js. r=me with that addressed.

::: mobile/android/chrome/content/browser.js
@@ +4652,5 @@
> +
> +        // Supply a label and value, since they can differ for datalist suggestions
> +        suggestions.push({ label: value, value: value });
> +        if (aCallback)
> +          aCallback(suggestions);

I think we can get rid of all of these |if (aCallback)| checks, since we're never calling _getAutoCompleteSuggetsions/_showAutoCompleteSuggestions without a valid aCallback parameter.
Comment 186 Neil Deakin 2013-04-10 18:32:06 PDT
(In reply to :Margaret Leibovic from comment #185)
> Comment on attachment 735965 [details] [diff] [review]
> Part 14 - implement asynchronous autcomplete on android
> 
> Review of attachment 735965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but you should just get rid of FormAutoComplete.js. r=me
> with that addressed.

With some logging enabled, I see that code being called during some tests. I'll leave it for now and file a followup bug.
Comment 188 Ryan VanderMeulen [:RyanVM] 2013-04-11 08:54:32 PDT
Unfortunately, this had to be backed out for frequent OSX debug mochitest-5 crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc39d8b649a

https://tbpl.mozilla.org/php/getParsedLog.php?id=21685513&tree=Mozilla-Inbound

08:34:56     INFO -  171399 INFO TEST-INFO | /tests/toolkit/components/satchel/test/test_form_autocomplete_with_list.html | expecting popup for test 300
08:34:56     INFO -  Assertion failure: mPresContext->mLayoutPhaseCount[eLayoutPhase_Reflow] == 0 (painting in the middle of reflow), at ../../../layout/base/nsAutoLayoutPhase.cpp:37
08:34:59  WARNING -  TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_autocomplete_with_list.html | Exited with code 1 during test run
08:34:59     INFO -  INFO | automation.py | Application ran for: 0:13:05.919909
08:34:59     INFO -  INFO | zombiecheck | Reading PID log: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpQu_7Wkpidlog
08:35:15  WARNING -  PROCESS-CRASH | /tests/toolkit/components/satchel/test/test_form_autocomplete_with_list.html | application crashed [@ nsAutoLayoutPhase::Enter()]
08:35:15     INFO -  Crash dump filename: /var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/tmpkZJoD6/minidumps/E350A22D-DAF2-4ECB-8FBD-E4FDA6BED10E.dmp
08:35:15     INFO -  Operating system: Mac OS X
08:35:15     INFO -                    10.7.2 11C74
08:35:15     INFO -  CPU: amd64
08:35:15     INFO -       family 6 model 23 stepping 10
08:35:15     INFO -       2 CPUs
08:35:15     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
08:35:15     INFO -  Crash address: 0x0
08:35:15     INFO -  Thread 0 (crashed)
08:35:15     INFO -   0  XUL!nsAutoLayoutPhase::Enter() [nsAutoLayoutPhase.cpp:0b669f2d896a : 34 + 0x0]
08:35:15     INFO -      rbx = 0x00007fff759f1630   r12 = 0x00007fff5fbfb4d8
08:35:15     INFO -      r13 = 0x000000010ca78de0   r14 = 0x00007fff5fbfb508
08:35:15     INFO -      r15 = 0x000000010ca780b0   rip = 0x00000001013fbe3f
08:35:15     INFO -      rsp = 0x00007fff5fbfb2f0   rbp = 0x00007fff5fbfb300
08:35:15     INFO -      Found by: given as instruction pointer in context
08:35:15     INFO -   1  XUL!PresShell::Paint(nsView*, nsRegion const&, unsigned int) [nsPresShell.cpp:0b669f2d896a : 5492 + 0x4]
08:35:15     INFO -      rip = 0x00000001013de9c0   rsp = 0x00007fff5fbfb310
08:35:15     INFO -      rbp = 0x00007fff5fbfb4b0
08:35:15     INFO -      Found by: stack scanning
Comment 189 Ryan VanderMeulen [:RyanVM] 2013-04-11 08:56:04 PDT
Note that bug 851641 is filed for the same intermittent failure, but with this applied, it was happening nearly all the time.
Comment 190 :Margaret Leibovic 2013-04-11 17:15:03 PDT
(In reply to Neil Deakin from comment #186)
> (In reply to :Margaret Leibovic from comment #185)
> > Comment on attachment 735965 [details] [diff] [review]
> > Part 14 - implement asynchronous autcomplete on android
> > 
> > Review of attachment 735965 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me, but you should just get rid of FormAutoComplete.js. r=me
> > with that addressed.
> 
> With some logging enabled, I see that code being called during some tests.
> I'll leave it for now and file a followup bug.

Well, as long as that component is still there, that's the one that will be used, so it makes sense that it's being called. I filed bug 861005, and I can just take care of the issue there.
Comment 191 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-02 09:58:39 PDT
Re-landed this as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/555f2b757639
Comment 192 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-02 14:51:02 PDT
(try run with that patch: https://tbpl.mozilla.org/?tree=Try&rev=1ad668e3d7a3)
Comment 193 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-02 17:14:51 PDT
Backed out for intermittent failures in bc:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=555f2b757639

16:04:15 WARNING - TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_sanitizeDialog.js | form entry 10-minutes-ago should no longer exist - Got 1, expected false 

Over to markh who graciously offered to take a look.
Comment 194 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-02 17:15:19 PDT
<RyanVM> slight variation - https://tbpl.mozilla.org/php/getParsedLog.php?id=22525336&tree=Mozilla-Inbound
<RyanVM> but a ~30% failure rate, you should be able to hit it on Try easily enough
Comment 195 Ryan VanderMeulen [:RyanVM] 2013-05-02 18:38:05 PDT
Backout:
https://hg.mozilla.org/mozilla-central/rev/555f2b757639
Comment 196 Mark Hammond [:markh] 2013-05-02 22:10:35 PDT
The sanitize() function in sanitize.js has a number of problems in this patch:

* |itemCount| is undefined, as |this.items| is an object rather than an array.
* This function is almost always called with |errorHandler| being undefined - but it unconditionally calls it on errors.
* When |errorHandler| is called, it is called with an |error| object, which is undefined - errors are pushed to an |errors| array.
* the |errorHandler| is attempted to be called multiple times with potentially the same errors (although all the points above mean neither of them will actually work.)

and the main problem causing this orange is that clearing the form data is now "truly async" - the callback to clear these items happens after the function has returned (and therefore after the dialog has been closed).  Thus, the form data is not guaranteed to be complete when the dialog closes (and I guess may not happen at all, depending on exactly what happens as the dialog and its scripts are torn down)

So off the top of my head, we need to:
* Make sanitize() itself async - either by having it return a promise, or by adding another callback for completion, and modify all the tests that call this.
* Have sanitize.xul close the dialog on completion rather than as soon as sanitize() returns.  As this is done via an ondialogaccept() handler, we'd need to ensure |false| is returned by this handler, then call window.close() on completion.  I guess this also means we'd want to disable both buttons on the dialog while this is happening.

I'm going to play with having sanitize() return a promise, and have it be "fulfilled" with the errors (ie, either null, or the array of errors.)  However, I'm not sure how deep we want to push promises into this code, so a simple callback approach might be preferred.  Any guidance here?
Comment 197 Mark Hammond [:markh] 2013-05-03 01:50:32 PDT
Created attachment 745061 [details] [diff] [review]
Have sanitize() return a promise

This is what I was thinking.  Flagging Gavin for feedback in the expectation he will pass it on to someone else, but anyone else can go for it too!

There's likely to be a couple of other test changes as a result of this which I'll complete and upload based on the feedback here.
Comment 198 Mark Hammond [:markh] 2013-05-03 07:14:53 PDT
windows only, "bc" only try at https://tbpl.mozilla.org/?tree=Try&rev=a3cd3b3281b3 which has lots of (presumably) unrelated orange on win8, but no trace of the orange causing the backout.  There's at least one "potential" orange remaining though - browser_bug409624.js also uses "sanitize" to clear form history.
Comment 199 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-03 20:13:01 PDT
Comment on attachment 745061 [details] [diff] [review]
Have sanitize() return a promise

Looks reasonable to me. You should ask Marco for review.
Comment 200 Mark Hammond [:markh] 2013-05-06 05:27:10 PDT
Created attachment 745832 [details] [diff] [review]
Part 6.2.1 - sanitize dialog and browser_bug409624.js wait until async operation complete

This is basically a fix to Part 6.2.  browser_sanitizeDialog.js was failing as the dialog was being closed before the form data was cleared - see comment 196.  The patch also includes a fix to browser_bug409624.js, which uses sanitize() directly and also clears form data, and although no orange has yet been seen here yet, it's probably inevitable it will appear :)

Try at https://tbpl.mozilla.org/?tree=Try&rev=21b2c4777cb8
Comment 201 Marco Bonardo [::mak] 2013-05-07 16:20:16 PDT
Comment on attachment 745832 [details] [diff] [review]
Part 6.2.1 - sanitize dialog and browser_bug409624.js wait until async operation complete

Review of attachment 745832 [details] [diff] [review]:
-----------------------------------------------------------------

I think the promise may be fine, on the other side this patch breaks some functionality (for which at this point I must suspect we don't have a test) since changes the error handler callback with a promise, but doesn't seem to change the code using the callback.
But mostly I dislike the fact we are making the UI look laggy by not fulfilling the user's request to close the dialog, I can imagine the user clicking Accept and nothing happens for hundreds milliseconds. This is going the opposite direction of snappy for which we are making this change, imo.

::: browser/base/content/sanitize.js
@@ +60,5 @@
>  
> +    let itemCount = Object.keys(this.items).length;
> +    let onItemComplete = function() {
> +      if (!--itemCount) {
> +        deferred.resolve(errors);

Why resolving the promise rather than rejecting it?
Notice that even to the reject callback you can pass whatever value you want, would make more sense to resolve() and reject(errors) to me.
Though, I think the real question is how do we use the error handler here?
The only use I could find is this one, in the patch that has been pushed:

   2.199  Sanitizer._checkAndSanitize = function() 
   2.200  {
   2.201    const prefs = Sanitizer.prefs;
   2.202    if (prefs.getBoolPref(Sanitizer.prefShutdown) && 
   2.203        !prefs.prefHasUserValue(Sanitizer.prefDidShutdown)) {
   2.204      // this is a shutdown or a startup after an unclean exit
   2.205      var s = new Sanitizer();
   2.206      s.prefDomain = "privacy.clearOnShutdown.";
   2.207 -    s.sanitize() || // sanitize() returns null on full success
   2.208 -      prefs.setBoolPref(Sanitizer.prefDidShutdown, true);
   2.209 +    let errorHandler = function() prefs.setBoolPref(Sanitizer.prefDidShutdown, true);
   2.210 +    s.sanitize(errorHandler);
   2.211    }
   2.212  };

So there are a couple things to notice here:

1. you are not fixing this code in your patch to use the promise, but you removed the errorHandler callback, that means this code is broken
2. nothing is interested in the single errors, so you could just reject() without collecting them. You could just reportError the single failures (unless you find other consumers that I couldn't find).

... and at this point would be more robust to write all of this as a simple return Task.spawn(...) and yield at the clearCallback (making a deferred, resolving it inside the callback and yielding on the promise). The decreasing counter looks a bit more fragile... But this is optional, the important thing is to fix the above brokeness.

::: browser/base/content/sanitize.xul
@@ +26,5 @@
>              dlgbuttons="accept,cancel"
>              title="&sanitizeDialog2.title;"
>              noneverythingtitle="&sanitizeDialog2.title;"
>              style="width: &dialog.width2;;"
> +            ondialogaccept="return gSanitizePromptDialog.sanitize();">

I don't think this is a good solution cause it will make the UI look slow or even blocked, the user clicks on Accept and nothing happens, though the window closes after some hundreds ms (in the best case).
Moving to async should avoid non-responding UI, not the opposite.

We need a different solution here that tests can use without making the UI laggy, dumb idea off my head could be to add a notification fired through the observer service and make the tests wait for it. Or you could get the sanitizer object out of the dialog window and expose an API to register sanitize observers (not sure if something like this is feasible, just wondering).
Comment 202 Marco Bonardo [::mak] 2013-05-07 16:21:30 PDT
PS: if it's easier for you, you can coalesce all already reviewed parts into a single patch, I can see how it may be problematic to work on parts for this specific scope.
Comment 203 Mark Hammond [:markh] 2013-05-07 19:25:21 PDT
(In reply to Marco Bonardo [:mak] from comment #201)
> Comment on attachment 745832 [details] [diff] [review]
...
> Why resolving the promise rather than rejecting it?
> Notice that even to the reject callback you can pass whatever value you
> want, would make more sense to resolve() and reject(errors) to me.
> Though, I think the real question is how do we use the error handler here?
> The only use I could find is this one, in the patch that has been pushed:

Right - *sigh* - I failed to notice this one.  Re "resolve" vs "reject", I guess it's just a matter of taste - I took the approach of "the operation mostly worked" (resolve) versus "the operation failed" (reject) - but I'm happy to change this.

> I don't think this is a good solution cause it will make the UI look slow or
> even blocked, the user clicks on Accept and nothing happens, though the
> window closes after some hundreds ms (in the best case).
> Moving to async should avoid non-responding UI, not the opposite.

My problem here is that it seems hard to guarantee the operation will complete.  If the window is destroyed before all the callbacks are made, it seems possible that the window teardown might kill enough of the window environment such that the callbacks are either never made or fail in obscure ways.

I believe the same basic problem exists in Sanitizer._checkAndSanitize when called by onShutdown() - the errors will not be reported until after sanitize() returns - which may obviously be after shutdown has completed (or even just after the pref service has been shutdown) - so I'm not sure how we can rely on the error handler setting a preference.

> We need a different solution here that tests can use without making the UI
> laggy, dumb idea off my head could be to add a notification fired through
> the observer service and make the tests wait for it.

Sure - although as above, I didn't take this approach purely for the tests - it seems inherently fragile to be destroying the window while important callbacks are pending.  (FWIW, I'd be much less concerned for this "window" case if the work was being done in a .jsm - however, even then it seems the shutdown case would remain fragile)

Can you clarify how my 2 concerns are either not relevant or should be handled - specifically:

* The window being closed before sanitize is complete, which runs the risk that the operation might not complete.
* That this is called on shutdown, meaning the callbacks might not fire, or even if they do, the preference service might not exist by the time they are called.
Comment 204 Marco Bonardo [::mak] 2013-05-08 02:42:47 PDT
(In reply to Mark Hammond (:markh) from comment #203)
> My problem here is that it seems hard to guarantee the operation will
> complete.  If the window is destroyed before all the callbacks are made, it
> seems possible that the window teardown might kill enough of the window
> environment such that the callbacks are either never made or fail in obscure
> ways.

We could somehow react to the user request, maybe by disabling the Accept/Cancel buttons and changing the Accept label to "Clearing"? Anything that wouldn't make the UI appear as unresponsive to the user's action.

> I believe the same basic problem exists in Sanitizer._checkAndSanitize when
> called by onShutdown() - the errors will not be reported until after
> sanitize() returns - which may obviously be after shutdown has completed (or
> even just after the pref service has been shutdown) - so I'm not sure how we
> can rely on the error handler setting a preference.

That's unfortunate, but we don't have an async shutdown procedure, yet.
Regardless, we can suppose it will work most of the times, the other times, on startup, we'll consider it an unclean shutdown and try to sanitize again. This is still good privacy-wise, not that good for startup, but should not happen frequently. We could maybe (in a followup) add a telemetry probe to see how many times it happens on startup, and if it ends up being a problem, cook up something better.
Comment 205 Mark Hammond [:markh] 2013-05-08 04:20:34 PDT
(In reply to Marco Bonardo [:mak] from comment #204)

> We could somehow react to the user request, maybe by disabling the
> Accept/Cancel buttons and changing the Accept label to "Clearing"?

The patch I uploaded did disable the buttons - I'll add that new string when I update the patch.

> That's unfortunate, but we don't have an async shutdown procedure, yet.
> Regardless, we can suppose it will work most of the times, the other times,
> on startup, we'll consider it an unclean shutdown and try to sanitize again.
> This is still good privacy-wise, not that good for startup, but should not
> happen frequently. We could maybe (in a followup) add a telemetry probe to
> see how many times it happens on startup, and if it ends up being a problem,
> cook up something better.

OK, np, thanks.
Comment 206 Mark Hammond [:markh] 2013-05-08 05:25:10 PDT
(In reply to Marco Bonardo [:mak] from comment #201)
> Comment on attachment 745832 [details] [diff] [review]
...
> Though, I think the real question is how do we use the error handler here?
> The only use I could find is this one, in the patch that has been pushed:
> 
>    2.199  Sanitizer._checkAndSanitize = function() 
>    2.200  {
>    2.201    const prefs = Sanitizer.prefs;
>    2.202    if (prefs.getBoolPref(Sanitizer.prefShutdown) && 
>    2.203        !prefs.prefHasUserValue(Sanitizer.prefDidShutdown)) {
>    2.204      // this is a shutdown or a startup after an unclean exit
>    2.205      var s = new Sanitizer();
>    2.206      s.prefDomain = "privacy.clearOnShutdown.";
>    2.207 -    s.sanitize() || // sanitize() returns null on full success
>    2.208 -      prefs.setBoolPref(Sanitizer.prefDidShutdown, true);
>    2.209 +    let errorHandler = function()
> prefs.setBoolPref(Sanitizer.prefDidShutdown, true);
>    2.210 +    s.sanitize(errorHandler);
>    2.211    }
>    2.212  };
> 
> So there are a couple things to notice here:

Actually, one other thing to notice here is that this part of the patch is actually wrong :)  The pref was previously set to true on success, now it is only set on error.
Comment 207 Mark Hammond [:markh] 2013-05-08 06:39:21 PDT
Created attachment 746930 [details] [diff] [review]
Part 6.2.1 - Updated per comments

(In reply to Marco Bonardo [:mak] from comment #201)
> Comment on attachment 745832 [details] [diff] [review]
> Part 6.2.1 - sanitize dialog and browser_bug409624.js wait until async
> operation complete
...
> ... and at this point would be more robust to write all of this as a simple
> return Task.spawn(...) and yield at the clearCallback (making a deferred,
> resolving it inside the callback and yielding on the promise). The
> decreasing counter looks a bit more fragile... But this is optional, the
> important thing is to fix the above brokeness.

I couldn't immediately see how to do the above and also have the task "remember" errors so the rejection happens after everything has been done, so I left it as a single deferred which is rejected with all errors after everything has been attempted.  I've also fixed that shutdown case so the pref is only set on success.

> I don't think this is a good solution cause it will make the UI look slow or
> even blocked, the user clicks on Accept and nothing happens, though the
> window closes after some hundreds ms (in the best case).
> Moving to async should avoid non-responding UI, not the opposite.

This patch both disables the buttons and sets the accept button label to "Clearing".

> We need a different solution here that tests can use without making the UI
> laggy, dumb idea off my head could be to add a notification fired through
> the observer service and make the tests wait for it. Or you could get the
> sanitizer object out of the dialog window and expose an API to register
> sanitize observers (not sure if something like this is feasible, just
> wondering).

Given the above and that the dialog doesn't close until everything has done, I don't think this is necessary.
Comment 208 Marco Bonardo [::mak] 2013-05-08 09:30:31 PDT
Comment on attachment 746930 [details] [diff] [review]
Part 6.2.1 - Updated per comments

Review of attachment 746930 [details] [diff] [review]:
-----------------------------------------------------------------

almost there

::: browser/base/content/sanitize.js
@@ +41,5 @@
>    
>    /**
> +   * Deletes privacy sensitive data in a batch, according to user preferences.
> +   * Returns a promise which is resolved with an array of exception objects or
> +   * null if no errors occurred.

Should be updated to state rejection

@@ +60,5 @@
>  
> +    let itemCount = Object.keys(this.items).length;
> +    let onItemComplete = function() {
> +      if (!--itemCount) {
> +        errors ? deferred.reject(errors) : deferred.resolve();

As I said, I think it's not worth it to collect errors since there are no consumers of it, just reportError them to the console, so here you can just reject or resolve... that's why I was suggesting alternatively to use Task, cause it's not worth to collect errors.
Btw, you may keep the current logic and just have a boolean error flag.

@@ +81,5 @@
>            } catch(er) {
>              if (!errors)
>                errors = {};
>              errors[itemName] = er;
>              dump("Error sanitizing " + itemName + ": " + er + "\n");

this dump doesn't make sense, likely leftover debug spew

@@ +522,5 @@
>        !prefs.prefHasUserValue(Sanitizer.prefDidShutdown)) {
>      // this is a shutdown or a startup after an unclean exit
>      var s = new Sanitizer();
>      s.prefDomain = "privacy.clearOnShutdown.";
> +    s.sanitize(errorHandler).then(function() {

should not be passing something to sanitize, since it doesn't take arguments
Comment 209 Mark Hammond [:markh] 2013-05-08 16:09:40 PDT
Created attachment 747184 [details] [diff] [review]
Part 6.2.1 - Updated per new comments

Hopefully this ticks the boxes :)
Comment 210 Marco Bonardo [::mak] 2013-05-09 05:10:35 PDT
Comment on attachment 747184 [details] [diff] [review]
Part 6.2.1 - Updated per new comments

Review of attachment 747184 [details] [diff] [review]:
-----------------------------------------------------------------

it looks ok, please get a try run, and we should be fine.

Note You need to log in before you can comment on or make changes to this bug.