I'm thinking of two functions: 1) sync moz_places 2) sync moz_historyvisits (1) should be done whenever we add a bookmark, and before moz_historyvisits is sync'd. Probably should be done with a bookmark observer (or done in C++ when the bookmark is added). (2) should be done every X time (TBD) We need tests for the following: (1) Ensuring we sync after bookmark add ?

Initial implementation - completely untested. Asking for review to make sure that this approach seems sound.

(In reply to comment #0) > (2) should be done every X time (TBD) if we had a pref to control the interval, testing this would be reasonably do-able as well

Comment on attachment 334363 [details] [diff] [review] v0.1 i'd prefer nsPlacesDBFlush or something like that. "sync" is an ambiguous and overloaded term. also, maybe nsP* to vaguely indicate that it's private.

Comment on attachment 334363 [details] [diff] [review] v0.1 yeah, approach looks fine.

This has one test that just adds a bookmark and ensures a sync happened. I had to post an extra empty event the the background thread to get the test to pass, which I don't full understand. I also trigger an assertion in the JS engine, which I will file a bug for shortly.

We just need one more test that ensures that we sync historyvisits. That one is arguably harder since it uses timers, but hey :)

Full test coverage now. Yay!

Comment on attachment 334927 [details] [diff] [review] v1.0 >+//// Constants >+ >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+const Cr = Components.results; >+ >+const kQuitApplication = "quit-application"; >+ >+const kSyncPrefName = "syncDBTableInterval"; please append "InSecs" or something like that so that it is self-documenting. >+ try { >+ // We want to silently fail if the preference does not exist, and use a >+ // default to fallback to. >+ this._syncInterval = this._prefs.getIntPref(kSyncPrefName); >+ } >+ catch (e) { >+ // The preference did not exist, so default to two minutes. >+ this._syncInterval = 120; >+ } make the default a const >+ else if (aTopic == "nsPref:changed" && aData == kSyncPrefName) { >+ // Get the new pref value, and then update our timer >+ this._syncInterval = aSubject.getIntPref(kSyncPrefName); >+ here and above, should validate the pref value is a usable number ////////////////////////////////////////////////////////////////////////////// >+ //// nsISupports >+ >+ classDescription: "Used to synchronize the temporary and permanent tables of Places", >+ classID: Components.ID("c1751cfc-e8f1-4ade-b0bb-f74edfb8ef6a"), >+ contractID: "@mozilla.org/places/sync;1", >+ _xpcom_categories: [{ >+ category: "app-startup", >+ service: true >+ }], the tryserver build will tell us how hard this hits Ts. if it's bad, we'll need to initiate it later, likely in delayedStartup. also, this file needs to be added to packages-static.

(In reply to comment #8) > here and above, should validate the pref value is a usable number Er, how could it ever not be a usable number? I'm not sure what I'd be validating against here... > the tryserver build will tell us how hard this hits Ts. if it's bad, we'll need > to initiate it later, likely in delayedStartup. > > also, this file needs to be added to packages-static. You can't start a service with the category manager in delayed startup, AFAIK. Besides, the only thing we do at startup is register a bookmark observer, which will init history, which is already in the Ts hotpath. The later we wait, the more likely we are to miss a new bookmark being added, and opens up a period of time for us to not have a database that has the proper foreign key requirements.

(In reply to comment #9) > (In reply to comment #8) > > here and above, should validate the pref value is a usable number > Er, how could it ever not be a usable number? I'm not sure what I'd be > validating against here... Nevermind - negative or zero would be bad.

Addresses review comments.

Comment on attachment 334953 [details] [diff] [review] v1.1 >+ /** >+ * Synchronizes the moz_{aName} and moz_{aName}_temp by copying all the data >+ * from the temporary table into the permanent one. It then deletes the data >+ * in the temporary table. All of this is done in a transaction that is >+ * rolled back upon failure at any point. >+ */ >+ _doSyncMozX: function PlacesBackground_doSyncX(aName) nit: s/doSyncX/doSyncMozX/ >+ { >+ // We try to get a transaction, but if we can't don't worry >+ let ourTransaction = false; >+ try { >+ this._db.beginTransaction(); >+ ourTransaction = true; >+ } >+ catch (e) { } why would we not be able to get a transaction? do we want to do this w/o one, since at one point some ids will be duplicated in both tables? also, should we use TRANSACTION_IMMEDIATE here to be safe?

(In reply to comment #12) > why would we not be able to get a transaction? do we want to do this w/o one, > since at one point some ids will be duplicated in both tables? also, should we > use TRANSACTION_IMMEDIATE here to be safe? There could already be one open (as in the case when we sync both tables), or sqlite could return busy (we should arguably retry in that case). I'm also not so sure on what happens with two different threads and transactions. I've sent a question to sqlite-users to get a clarification so I can figure out exactly what we should be doing in this situation: http://www.mail-archive.com/sqlite-users@sqlite.org/msg36424.html

So here's the deal. Transactions are per connection, so we have a few options here: 1) wait in while until we do not have an open transaction, try to get one (we'd still have to wrap in a try catch because someone else on another thread could open one up on us) 2) Continue to do what we do and try to get the transaction, but basically say "oh well" if we cannot get it The problem with those two is that we can end up executing the first query in one of those, and then a transaction closes, and then we execute the second one, which would leave us in an inconsistent state (ugh). On top of that, someone could come and add something to one of the temporary tables after we INSERT OR REPLACE, but before we DELETE from, so at that point, the data is lost completely :(. As a result, I think both of those options are off the table. So, what we really need is some way to do a few operations on the database atomically and make sure that *no* other consumers can read or write from the database on other threads during this time. I can't think of a solution immediately to that problem though :(

I'll note that this would likely be a much easier problem to solve if sqlite supported nested named transactions, but I don't think they plan on adding that any time soon.

how about a delete trigger on the temp table, that inserts that row into the permanent table? that way, deleting everything from the temp table in one query would also insert it all into the permanent one in a single step.

I think that approach has merit. As far as I can tell, that would make the each row's deletion atomic. Hurray! I'll cook up a patch for that momentarily.

OK. That suggestion has been implemented, and it passes the tests (I had the trigger wrong at first, and it failed the tests spectacularly!).

Comment on attachment 334983 [details] [diff] [review] v1.2 >+ //// nsINavBookmarkObserver >+ >+ onBeginUpdateBatch: function() this._inBatchMode = true, >+ onEndUpdateBatch: function() this._inBatchMode = false, i think we might want to pay attention to batch state, instead of just blindly syncing every time an item is added. having to sync between each item could severely slow down first-run upgraders from Fx2, as well as bookmark import and restore.

Comment on attachment 334983 [details] [diff] [review] v1.2 another question: a single write query is implicitly a transaction. given that, is the explicit transaction code in _doSyncMozX() necessary?

(In reply to comment #19) > i think we might want to pay attention to batch state, instead of just blindly > syncing every time an item is added. having to sync between each item could > severely slow down first-run upgraders from Fx2, as well as bookmark import and > restore. Ugh - I was worrying about state, but not using it when syncing. CRY! OK, that's an easy fix, and I'll make a test for that too. (In reply to comment #20) > (From update of attachment 334983 [details] [diff] [review]) > another question: a single write query is implicitly a transaction. given that, > is the explicit transaction code in _doSyncMozX() necessary? It might not be, but I'm not 100% how triggers work there with regards to the implicit transactions. I'd say it doesn't hurt since we want that to be atomic.

Comment on attachment 334983 [details] [diff] [review] v1.2 per previous comments

not to self - sync before shutdown :(

This is another iteration, but I'm not requesting review yet. What this does: Properly handles batching. Cancels the timer during batch mode so it doesn't update during a batch. Adds tests to test that adding a visit or a bookmark does not get synchronized until batch mode is completed What still needs to be done: Have the timer dispatched to the background thread so it doesn't do anything on the main thread. Test that the timer does not run during batch mode (this test is going to be a bit crazy, but it's doable). Flush on quit. This is a bit harder, and may mean that I am going to add an observer topic that will be dispatched just before we close down the background thread so we know to post any last events to it. I fully expect to get 100% test coverage on all of our behaviors added in this bug.

> Have the timer dispatched to the background thread so it doesn't do anything on > the main thread. Turns out this will trigger an assertion (one that we shouldn't be triggering), so I'm punting this to bug 451781. It's not terribly important anyway. > Test that the timer does not run during batch mode (this test is going to be a > bit crazy, but it's doable). Consequentially, this isn't important now either.

(In reply to comment #24) > Flush on quit. This is a bit harder, and may mean that I am going to add an > observer topic that will be dispatched just before we close down the background > thread so we know to post any last events to it. We actually do this now, but not in a way that I consider safe. I'm going to add an observer topic for places thread shutdown, and add a test to make sure we sync.

Addresses review comments, and accomplishes everything I said I needed to still.

(In reply to comment #20) > another question: a single write query is implicitly a transaction. given that, > is the explicit transaction code in _doSyncMozX() necessary? I just verified that this is in fact the case. That transaction has been removed.

Comment on attachment 335126 [details] [diff] [review] v1.5 looks good, r=me

This was actually being created to early in real builds, and we ended up not having a profile directory. We now get created during profile-after-change, so we have a profile, and everything is AOK to use. It's a small change, and I factored out a small bit of code into the head_background.js too, so I'm not requesting a new review.

Forgot to hg add my last test...

i'm not sure this works fine, i'm using a build with our latest patches in and i see corruption in bookmarks, i don't yet know if that's due to a wrong query or to wrong sync though. i see wrong moz_places ids in my table, for example most visited on the toolbar has fk = 1 but the correct query in the moz_places table has id = 3 while the last site i've visited has got id = 1...

I've got the non-batched and batched code paths covered in this bug with tests to ensure that the sync happens correctly. If you could get steps to reproduce the corruption, it'd be a big help.

This adds tests to an issue Marco and I found with the insert triggers.

(In reply to comment #34) > Created an attachment (id=338236) [details] > v1.8 > > This adds tests to an issue Marco and I found with the insert triggers. how can this be litmus tested from user's end? Please suggest some test areas.

(In reply to comment #35) > how can this be litmus tested from user's end? Please suggest some test > areas. Normal browsing would have completely hosed the places database after just a short while. I'm not sure we need litmus tests when we have automated coverage (better to spend human time on things that are hard to test, right?). Please correct me if I'm wrong.

Comment on attachment 338236 [details] [diff] [review] v1.8 +/** + * This trigger moves the data out of a temporary table into the permanent one + * before deleting from the temporary table. + */ +#define CREATE_TEMP_SYNC_TRIGGER_BASE(__table) NS_LITERAL_CSTRING( \ + "CREATE TEMPORARY TRIGGER " __table "_beforedelete_trigger " \ + "BEFORE DELETE ON " __table "_temp FOR EACH ROW " \ + "BEGIN " \ + "INSERT OR REPLACE INTO " __table " " \ + "SELECT * FROM " __table "_temp " \ + "WHERE id = OLD.id;" \ + "END" \ +) add a comment on the fact that in such a trigger INSERT OR REPLACE does not create a new id diff --git a/toolkit/components/places/tests/background/test_database_sync_after_quit_applicaiton.js b/toolkit/components/places/tests/background/test_database_sync_after_quit_applicaiton.js new file mode 100644 --- /dev/null +++ b/toolkit/components/places/tests/background/test_database_sync_after_quit_applicaiton.js typo: applicaiton apart that, r=me

Please don't let there by a v1.10...

i'm not sure if it's only my problem, but actually using patch 1.9 the build did not create head_background.js in toolkit\components\places\tests\background, looking at the patch i see a call to copy from/to, maybe my mozilla-build windows environment is missing some piece

Extremely minor in the global picture, but it seems unnecessary to have a lazy getter for _bh as it is used a few statements later. + this.__defineGetter__("_bh", function() { + delete this._bh; + return this._bh = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. + getService(Ci.nsINavBookmarksService); + }); + ... + // Register observers + this._bh.addObserver(this, false);

(In reply to comment #40) > Extremely minor in the global picture, but it seems unnecessary to have a lazy > getter for _bh as it is used a few statements later. that code has been replaced in bug 459491

(In reply to comment #41) > that code has been replaced in bug 459491 Marco, Shawn, Dietrich, the results of the Big Places Optimization Work live in patches filed to several bugs, which makes it a bit hard to the big picture. Is it possible to publish a link to a Mercurial repo or a Mercurial patchqueue repo with all related patches in a tracking bug like bug 442967?

We've been using a public patch queue all this time: http://hg.mozilla.org/users/sdwilsh_shawnwilsher.com/storage-async/

we have a bunch of automatic tests, and i don't see how a litmus test could be written to check this (would require the user to directly look at sqlite tables and temp tables).