878677 - Use FormHistory.jsm instead of nsIFormHistory2 for syncing form history

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

We are trying to remove nsIFormHistory2 and nsFormHistory.js in favour of FormHistory.jsm for a couple of reasons - mainly to avoid synchronous SQL.  This bug is to track this effort for the consumers of nsIFormHistory2 in the sync code.

Comment 1

[Richard Newman [:rnewman]]

(Mangling the English language for consistency's sake.)

N.B. look for necessary changes in TPS, too.

Comment 2

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

Attachment: allow FormHistory.jsm to change the GUID for an existing value, and notify with the GUID of a deleted item

This patch allows an 'update' to change to GUID of an existing item, by allowing a 'newGuid' element to appear in the change record.

It also sends the GUID of a deleted item when sending a satchel-storage-changed/formhistory-remove notification so sync can track the deletion.

I'll request review/feedback on this patch once I get some feedback on the sync changes (but the sync changes obviously still depends on this patch)

Comment 3

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

Attachment: Use FormHistory.jsm to sync form data.

This patch uses FormHistory.jsm to sync the form data.  It adds a couple of "spinningly" methods for querying and updating the form history and seems to work OK.  The patch seems largely complete, but I'm requesting feedback to make sure I'm on the right track.

Comment 4

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

Attachment: Update the "tracker" part of the form sync code to use FormHistory.jsm

This patch updates the "tracker" code to work with FormHistory.jsm.  It is split as its own patch simply to aid review.

Note a slightly earlier version of this patch series was pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=ca629a393241 - it seems green, but I'm not sure if the tps tests were actually exercised there...

Comment 5

[Gregory Szorc [:gps]]

RelEng build automation does not exercise TPS tests - you'll need to run them locally or push to services-central (a one-off VM runs tests).

https://developer.mozilla.org/en-US/docs/TPS

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 761317 [details] [diff] [review]
Use FormHistory.jsm to sync form data.

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

You've changed the behavior of getGUID here. Previously, if a given name/value pair was found but had no GUID, one would be generated and assigned. Would I be correct in assuming that you're now generating strong random 12-character GUIDs for every form history entry, and thus 

  return results[0].guid;

will never return null?

r+ if that's true, and thanks for getting this done!

(Sorry for the nitty review-instead-of-feedback; can't help it!)

::: services/sync/modules/engines/forms.js
@@ +37,4 @@
>    _guidCols:     ["guid"],
>  
> +  // Do a "sync" search by spinning the event loop until it completes.
> +  _searchSpinningly: function(aTerms, aSearchData) {

We use 'modern' (foo, not aFoo) style in services/ (and elsewhere, for that matter!).

@@ +49,5 @@
> +      }
> +    };
> +    Svc.FormHistory.search(aTerms, aSearchData, callbacks);
> +    Async.waitForSyncCallback(syncCallback);
> +    return results;

I think you can just do this:

  let cb = Async.makeSpinningCallback();
  let callbacks = {
    ...
    handleCompletion: function(reason) {
      cb(results);
    }
  };
  Svc.FormHistory.search(terms, searchData, callbacks);
  return cb.wait();

And you might elect to check for !results.length here, and just return null; kill some lines in the callers.

@@ +60,5 @@
> +        syncCallback();
> +      }
> +    };
> +    Svc.FormHistory.update(changes, callbacks);
> +    Async.waitForSyncCallback(syncCallback);

Same.

@@ +71,5 @@
>  
>    getEntry: function getEntry(guid) {
> +    let results = this._searchSpinningly(this._getEntryCols, {'guid': guid});
> +    if (!results.length)
> +      return null;

{}

@@ +77,5 @@
>    },
>  
>    getGUID: function getGUID(name, value) {
>      // Query for the provided entry.
> +    let query = { 'fieldname': name, 'value': value }

"

@@ +79,5 @@
>    getGUID: function getGUID(name, value) {
>      // Query for the provided entry.
> +    let query = { 'fieldname': name, 'value': value }
> +    let results = this._searchSpinningly(this._guidCols, query);
> +    if (results.length == 0) {

!results.length

(matches above, and good style)

@@ +87,5 @@
>    },
>  
>    hasGUID: function hasGUID(guid) {
> +    // could probably use a count function here, but searchSpinningly exists...
> +    let results = this._searchSpinningly(this._guidCols, {'guid': guid});

""

@@ +153,3 @@
>      let guids = {};
> +    for (let result of results)
> +      guids[result.guid] = true;

{}

@@ +181,5 @@
> +    let change = {
> +      op: 'add',
> +      fieldname: record.name,
> +      value: record.value
> +    }

;

@@ +188,5 @@
>  
>    remove: function FormStore_remove(record) {
>      this._log.trace("Removing form record: " + record.id);
> +    let change = {
> +      op: 'remove',

Is this a no-op if the GUID isn't found in storage?

@@ +201,5 @@
>  
>    wipe: function FormStore_wipe() {
> +    let change = {
> +      op: 'remove'
> +    }

;

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 761318 [details] [diff] [review]
Update the "tracker" part of the form sync code to use FormHistory.jsm

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

::: services/sync/modules/engines/forms.js
@@ +280,5 @@
>          break;
>        case "satchel-storage-changed":
> +        if (data == "formhistory-add" || data == "formhistory-remove") {
> +          let guid = subject.QueryInterface(Ci.nsISupportsString).toString();
> +          this.addChangedID(guid);

Awesome!

Comment 8

[Richard Newman [:rnewman]]

Mark: let me know if you want me to address my nits and land this on s-c (which I presume you don't have checked out).

Comment 9

[James Bonacci [:jbonacci]]

If so, then some QA testing would be nice (if applicable)...

Comment 10

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

Attachment: allow FormHistory.jsm to change the GUID for an existing value, and notify with the GUID of a deleted item

This version of the patch uses double-quotes instead of single quotes.

This patch make 2 changes:

* It allows an update() to change the GUID of an existing entry, which is something sync needs to do.  This is done by specifying a field name of 'newGuid' - for example, the following change record could be used:

let change = {
  op: "update",
  guid: oldGUID,
  newGuid: newGUID,
};

(obviously it is impossible to use "guid" for both the old and new values).  The patch also ensures that no other field names can be used when updating the GUID - ie, you can't change the fieldname or value at the same time.  This obviously isn't a hard-requirement, it just seems prudent.

The second change is relatively trivial - it arranges to send the record's guid when sending a "formhistory-remove" notification.  Again, sync wants to know the GUID of deletions so it can weave (hah!) its magic.

Requesting review from enn as he most recently touched this - please feel free to reassign if desired.

Comment 11

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

Attachment: Updated per feedback

(In reply to Richard Newman [:rnewman] from comment #6)
> Comment on attachment 761317 [details] [diff] [review]
> Use FormHistory.jsm to sync form data.
> 
> Review of attachment 761317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You've changed the behavior of getGUID here. Previously, if a given
> name/value pair was found but had no GUID, one would be generated and
> assigned. Would I be correct in assuming that you're now generating strong
> random 12-character GUIDs for every form history entry, and thus 
> 
>   return results[0].guid;
> 
> will never return null?

That's true, and the schema migration code in nsFormHistory.js (where the guid was initially added) updated all existing entries to have a guid - so I think this is safe.

> I think you can just do this:
> 
>   let cb = Async.makeSpinningCallback();
>   let callbacks = {
>     ...
>     handleCompletion: function(reason) {
>       cb(results);

cb(null, results) actually, as the first param is the 'error' - I've changed it to this.

> And you might elect to check for !results.length here, and just return null;
> kill some lines in the callers.

I did this, but it didn't make things much easier and made one case harder - so I stuck with always returning a (possibly empty) array.

> >    getGUID: function getGUID(name, value) {
> >      // Query for the provided entry.
> > +    let query = { 'fieldname': name, 'value': value }
> 
> "

As discussed on IRC, all such constructs now consistently use no quotes at all.

> @@ +79,5 @@
> >    getGUID: function getGUID(name, value) {
> >      // Query for the provided entry.
> > +    let query = { 'fieldname': name, 'value': value }
> > +    let results = this._searchSpinningly(this._guidCols, query);
> > +    if (results.length == 0) {
> 
> !results.length
> 
> (matches above, and good style)

Ended up with: return results.length ? results[0].guid : null;

> >    remove: function FormStore_remove(record) {
> >      this._log.trace("Removing form record: " + record.id);
> > +    let change = {
> > +      op: 'remove',
> 
> Is this a no-op if the GUID isn't found in storage?

Actually, it will have the side-effect of sending a notification of the (non-) deletion, but I assume tracking is disabled when this is called.  I can update if that is a problem (but it will obviously affect performance in the "usual" case, as it will require a search followed by the update).  Re-requesting r+ for this issue alone.

There were also a couple of other single-quotes which I changed.

As mentioned in IRC, I failed to get the TPS tests to run locally, so feel free to take over this bug for landing (assuming enn gives r+ on the FormHistory patch)

Comment 12

[Neil Deakin]

Comment on attachment 761841 [details] [diff] [review]
allow FormHistory.jsm to change the GUID for an existing value, and notify with the GUID of a deleted item

Looks good but add some documentation on this at the top of FormHistory.jsm under the 'update' section and/or 'Terms'.

Comment 13

[Richard Newman [:rnewman]]

(In reply to Mark Hammond (:markh) from comment #11)

> > Is this a no-op if the GUID isn't found in storage?
> 
> Actually, it will have the side-effect of sending a notification of the
> (non-) deletion, but I assume tracking is disabled when this is called.

It is. So long as it doesn't throw or logspew, I'm happy!

> As mentioned in IRC, I failed to get the TPS tests to run locally, so feel
> free to take over this bug for landing (assuming enn gives r+ on the
> FormHistory patch)

Will do!

Comment 14

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

Attachment: Document newGuid in the 'terms' section

This patch adds documentation for newGuid to the 'terms' section.

Comment 15

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

Assigning to Richard for landing as he knows how to wrangle the TPS tests.  Please let me know or assign back to me if I can be any more help.

Comment 16

[Richard Newman [:rnewman]]

Pushed to s-c to watch.

   https://hg.mozilla.org/services/services-central/rev/9126675b827a
   https://hg.mozilla.org/services/services-central/rev/31f1458c6a0b

Setting needinfo on myself to watch for TPS.

Comment 17

[Richard Newman [:rnewman]]

xpcshell is failing after this push. Apparently something isn't closing the DB from the correct thread (perhaps because threads are already joined at this point?).

Let me know Tuesday morning if you'd rather fix-in-place on s-c, or have me back this out. I don't mind leaving s-c orange for a little while.


18:25:02     INFO -  Sync.SyncScheduler	DEBUG	Clearing sync triggers and the global score.
18:25:02     INFO -  TEST-PASS | (xpcshell/head.js) | 4 (+ 0) check(s) passed
18:25:02     INFO -  TEST-INFO | (xpcshell/head.js) | 0 check(s) todo
18:25:02     INFO -  WARNING: NS_ENSURE_TRUE(mThread != PR_GetCurrentThread()) failed: file ../../../xpcom/threads/nsThread.cpp, line 445
18:25:02     INFO -  WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../xpcom/base/nsExceptionService.cpp, line 167
18:25:02     INFO -  WARNING: An event was posted to a thread that will never run it (rejected): file ../../../xpcom/threads/nsThread.cpp, line 367
18:25:02     INFO -  WARNING: NS_ENSURE_TRUE(asyncCloseWasCalled) failed: file ../../../storage/src/mozStorageConnection.cpp, line 943
18:25:02     INFO -  Assertion failure: !mAsyncExecutionThread, at ../../../storage/src/mozStorageConnection.cpp:415
18:25:02     INFO -  <<<<<<<
18:25:19  WARNING -  PROCESS-CRASH | /Users/cltbld/talos-slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/test_collections_recovery.js | application crashed [@ mozilla::storage::Connection::~Connection()]
18:25:19     INFO -  Crash dump filename: /Users/cltbld/talos-slave/test/build/tests/xpcshell/tests/services/sync/tests/unit/8FACE4C2-9ADF-4638-A84A-9147604DB640.dmp
18:25:19     INFO -  Operating system: Mac OS X
18:25:19     INFO -                    10.6.8 10K549
18:25:19     INFO -  CPU: amd64
18:25:19     INFO -       family 6 model 23 stepping 10
18:25:19     INFO -       2 CPUs
18:25:19     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
18:25:19     INFO -  Crash address: 0x0
18:25:19     INFO -  Thread 0 (crashed)
18:25:19     INFO -   0  XUL!mozilla::storage::Connection::~Connection() [mozStorageConnection.cpp : 415 + 0x0]
18:25:19     INFO -      rbx = 0x00007fff706d22f8   r12 = 0x0000000000000002
18:25:19     INFO -      r13 = 0x0000000000000001   r14 = 0x0000000000000000
18:25:19     INFO -      r15 = 0x0000000108207720   rip = 0x0000000101429bca
18:25:19     INFO -      rsp = 0x00007fff5fbfe2b0   rbp = 0x00007fff5fbfe2c0
18:25:19     INFO -      Found by: given as instruction pointer in context
18:25:19     INFO -   1  XUL!mozilla::storage::Connection::Release() [mozStorageConnection.cpp : 412 + 0x7]
18:25:19     INFO -      rip = 0x0000000101429f0a   rsp = 0x00007fff5fbfe2d0
18:25:19     INFO -      rbp = 0x00007fff5fbfe2f0
18:25:19     INFO -      Found by: stack scanning
18:25:19     INFO -   2  XUL!nsTArray_Impl<nsRefPtr<mozilla::storage::Connection>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) [nsAutoPtr.h : 880 + 0x4]
18:25:19     INFO -      rip = 0x00000001014346cd   rsp = 0x00007fff5fbfe300
18:25:19     INFO -      rbp = 0x00007fff5fbfe330
18:25:19     INFO -      Found by: stack scanning
18:25:19     INFO -   3  XUL!mozilla::storage::Service::unregisterConnection(mozilla::storage::Connection*) [nsTArray.h : 1116 + 0x9]
18:25:19     INFO -      rip = 0x0000000101432889   rsp = 0x00007fff5fbfe340
18:25:19     INFO -      rbp = 0x00007fff5fbfe360
18:25:19     INFO -      Found by: stack scanning
18:25:19     INFO -   4  XUL!mozilla::storage::Connection::Release() [mozStorageConnection.cpp : 436 + 0x7]
18:25:19     INFO -      rip = 0x0000000101429f59   rsp = 0x00007fff5fbfe370
18:25:19     INFO -      rbp = 0x00007fff5fbfe390
18:25:19     INFO -      Found by: stack scanning
18:25:19     INFO -   5  XUL!XPCJSRuntime::GCCallback(JSRuntime*, JSGCStatus) [XPCJSRuntime.cpp : 626 + 0x8]
18:25:19     INFO -      rip = 0x00000001011d8cad   rsp = 0x00007fff5fbfe3a0
18:25:19     INFO -      rbp = 0x00007fff5fbfe3e0
18:25:19     INFO -      Found by: stack scanning
18:25:19     INFO -   6  XUL!PRMJ_Now() [prmjtime.cpp:31f1458c6a0b : 204 + 0x4]
18:25:19     INFO -      rip = 0x0000000102592223   rsp = 0x00007fff5fbfe3b0
18:25:19     INFO -      rbp = 0x00007fff5fbfe3e0
18:25:19     INFO -      Found by: stack scanning

Comment 18

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

(In reply to Richard Newman [:rnewman] from comment #17)
> xpcshell is failing after this push. Apparently something isn't closing the
> DB from the correct thread (perhaps because threads are already joined at
> this point?).

Is it possible an async operation hadn't completed (eg, earlier on the async thread, causing this attempt to close on (probably) the main thread?  Or something...:)

> Let me know Tuesday morning if you'd rather fix-in-place on s-c, or have me
> back this out. I don't mind leaving s-c orange for a little while.

A fix isn't obvious :(  We could probably arrange for all pending form operations to complete, but it's not clear to me where we would hook this into sync.

Do you know what specific test failed?

Comment 19

[Marco Bonardo [:mak]]

if an async statement has been executed before asyncClose() call, it will complete, maybe there's an async statement being created earlier but used after asyncClose? Or some getter is reopening a connection after xpcom-shutdown?

Comment 20

[Richard Newman [:rnewman]]

There are ten failures in services/sync/tests/unit.

asyncClose was not called at all, according to the log.

Comment 21

[Richard Newman [:rnewman]]

Backed out the combined part 2:

https://hg.mozilla.org/services/services-central/rev/7144ed2dfe0e

(Mark, please reimport the landed patch when you work on fixing this. Easily repeatable locally.)

Comment 22

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

Attachment: Explicitly shutdown the FormHistory module as xpcshell tests exit.

FormHistory observes the 'profile-before-change' notification to shutdown.  I'm not familiar with that notification, but I assume it is sent on browser shutdown, otherwise we'd end up with the same basic problem in mochitests.  This isn't sent in an xpcshell context which explains the problem we see.

I tried adding an observer for xpcom-shutdown-threads, but it seems this was too late.  So I ended up just adding an explicit shutdown for the xpcshell tests - this patch adds a cleanup function that calls FormHistory.shutdown() and solves the problem for me.  It has the added bonus of not touching the form history code itself - IMO it's fine to re-address this if we ever end up with other xpcshell tests that use FormHistory.  Thus, I think it should be fine to take with just a "local" review...

Comment 23

[Richard Newman [:rnewman]]

Did you try watching xpcom-shutdown instead of xpcom-shutdown-threads? I think the former occurs earlier in the shutdown sequence, and it would be a little safer than registering an out-of-band cleanup for some places in which this code runs, IMO.

(Note that you should keep profile-before-change, because I think Firefox just kills everything if it's switching profiles...)

Comment 24

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

(In reply to Richard Newman [:rnewman] from comment #23)
> Did you try watching xpcom-shutdown instead of xpcom-shutdown-threads? I
> think the former occurs earlier in the shutdown sequence, and it would be a
> little safer than registering an out-of-band cleanup for some places in
> which this code runs, IMO.

I meant to mention that - I did try xpcom-shutdown and couldn't see any evidence of it being called at all (which surprised me) - I just had a dump() statement as the first line of the handler - I didn't see the dump and still got the shutdown assertion.

I then tried xpcom-shutdown-threads, and while I *did* see the dump(), I got an assertion telling me something like "the event you posted will not be acted on as something something thread something" ...

> (Note that you should keep profile-before-change, because I think Firefox
> just kills everything if it's switching profiles...)

Yeah, that was my intent.  I'll have another go at that and keep track of the specific failures I see (and maybe I'll take this opportunity to learn how to break into the debugger during an xpcshell test...)

Comment 25

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

Attachment: Explicitly initialize the formstartup service

So most of that can be explained as sloppiness on my part.  The reason my earlier attempts failed is simply that the component which observes the notifications for shutdown wasn't initialized.  And I obviously jumped to a conclusion about an assertion I saw...

The approach taken by the FormHistory tests themselves is to ensure the form-history-startup service has been initialized - once it has, the xpcshell tests shutdown cleanly.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/test/unit/head_satchel.js#21

So I figure that approach must be OK...

Comment 26

[Marco Bonardo [:mak]]

(In reply to Mark Hammond (:markh) from comment #22)
> Created attachment 764561 [details] [diff] [review]
> Explicitly shutdown the FormHistory module as xpcshell tests exit.
> 
> FormHistory observes the 'profile-before-change' notification to shutdown. 
> I'm not familiar with that notification, but I assume it is sent on browser
> shutdown, otherwise we'd end up with the same basic problem in mochitests. 
> This isn't sent in an xpcshell context which explains the problem we see.

Nope, xpcshell tests sends the notification, provided the test (usually head.js) properly invokes do_get_profile() once. Not doing so would be wrong regardless when testing a component that depends on having a profile.

> I tried adding an observer for xpcom-shutdown-threads, but it seems this was
> too late.

that is very very late.

Comment 27

[Marco Bonardo [:mak]]

(In reply to Mark Hammond (:markh) from comment #24)
> I meant to mention that - I did try xpcom-shutdown and couldn't see any
> evidence of it being called at all (which surprised me) - I just had a
> dump() statement as the first line of the handler - I didn't see the dump
> and still got the shutdown assertion.

xpcom-shutdown in xpcshell-tests is fired when the xpcshell process goes away, much after the test environment has been closed.

Comment 28

[Richard Newman [:rnewman]]

Comment on attachment 764581 [details] [diff] [review]
Explicitly initialize the formstartup service

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

::: services/sync/tests/unit/head_helpers.js
@@ +7,5 @@
> +// that it has been initialized.
> +// This ensures it will observe the necessary notifications for clean shutdown.
> +let formHistoryStartup = Cc["@mozilla.org/satchel/form-history-startup;1"].
> +                          getService(Ci.nsIObserver);
> +formHistoryStartup.observe(null, "profile-after-change", null);

Simply invoking `do_get_profile();` from testing/xpcshell/head.js will cause this notification to be sent. That's the best approach.

Comment 29

[Richard Newman [:rnewman]]

I'll finish this up.

Comment 30

[Richard Newman [:rnewman]]

Interestingly, sync/tests/unit/head_appinfo.js already calls do_get_profile, and adding it to the test does not fix the problem.

Comment 31

[Richard Newman [:rnewman]]

OK, I see what you meant. FormHistoryStartup isn't in the manifest, so it doesn't listen to profile-after-change, so it doesn't install its cleanup hooks. We need to manually poke it.

Comment 32

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #28)
> > +let formHistoryStartup = Cc["@mozilla.org/satchel/form-history-startup;1"].
> > +                          getService(Ci.nsIObserver);
> > +formHistoryStartup.observe(null, "profile-after-change", null);
> 
> Simply invoking `do_get_profile();` from testing/xpcshell/head.js will cause
> this notification to be sent. That's the best approach.

I said do_get_profile() invokes profile-before-change, not profile-after-change. This is for the simple reason there are various components that are inited by profile-after-change and it would be crazy to always invoke it and load a bunch of useless components. profile-after-change must indeed be notified manually.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

https://developer.mozilla.org/en-US/docs/Observer_Notifications is a useful reference FWIW. The "profile-*" notifications are confusingly-named because they're remnants of when SeaMonkey supported same-session profile switching (one profile had to be "closed" and then another "opened", which essentially just maps to shutdown and startup in a non-profile switching world).

Comment 34

[Richard Newman [:rnewman]]

Let's see if this sticks.

https://hg.mozilla.org/services/services-central/rev/75763abd1bdf

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9126675b827a
https://hg.mozilla.org/mozilla-central/rev/75763abd1bdf
https://hg.mozilla.org/mozilla-central/rev/e91536052c99

Comment 36

[Richard Newman [:rnewman]]

Mark, y'missed a spot when you killed trackEntry!

      // Get the GUID on a delay so that it can be added to the DB first...
      Utils.nextTick(function() {
        this._log.trace("Logging form element: " + [name, el.value]);
        this.trackEntry(name, el.value);
      }, this);

Comment 37

[:Gavin Sharp [email: [e-mail]]]

Attachment: slowsql data from builds before the patch

Taras set me up with jydoop to confirm that this had the effect we hoped it would. I queried SlowSQL data for all formhistory.sqlite hits on Nightly during the period of June 21-24, both for builds with buildID > 20130622 (i.e. builds containing this fix) and buildID < 20130623 (i.e. builds without this fix).

This is the data from builds without this fix.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Attachment: slowsql data from builds after the patch

Here's the data from after. Fewer and smaller numbers, confirming that this change made a big difference, and eliminated most of the remaining main thread slow queries. Good work!

The remaining hits are mostly DB creation/migration, I filed bug 888784 for that.