431558 - implement preventive maintenance for places.sqlite

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Dietrich Ayala (:dietrich)]

vlad experienced severe corruption, rendering his places.sqlite file invalid.

we should do periodic integrity checks, taking corrective action where possible.

Comment 1

[Marco Bonardo [:mak]]

and we should do that in a background thread

Comment 2

[Marco Bonardo [:mak]]

Attachment: wip

while looking at a profile with corrupt tags i started playing with this... is a module to do cleanup once per session, on idle or on browser close, in the background thread...
Don't bother looking at tags overlay UI changes since those are unrelated (i was working to avoid users set no title tags, and allow them to delete tag containers)

Hwv, my problems is that this crashes badly when onEndUpdateBatch calls QueryResultNode::Refresh that calls invalidateContainer. I'm crashing when treeView::invalidateContainer does selection.selectEventsSuppressed = false.

to repro the crash:
1. create a profile, open the library, add a tag to a bookmark
2. restart the browser, open the library, expand Tags in the left pane
3. wait about 15s for the module to start cleanup
4. crash!

Any idea on why that's crashing? is that due to the background thread? Notice that it crashes even if i don't do any change.

Comment 3

[Marco Bonardo [:mak]]

PS: idle and check timeouts are low to catch the crash quickly and don't have to wait too long

Comment 4

[Marco Bonardo [:mak]]

oh, the crash happens when we are callign refresh on a RESULTS_AS_TAG_QUERY resultNode (that is the open Library Tags container), does not crash if the container is closed.

Comment 5

[Shawn Wilsher :sdwilsh]

We really shouldn't be doing this on shutdown - we are trying to do away with things like this because of bug 453178

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> We really shouldn't be doing this on shutdown - we are trying to do away with
> things like this because of bug 453178

i've added the shutdown part for completion, hwv it's empty and we can get rid of it

Comment 7

[Marco Bonardo [:mak]]

well this was actually due to the background thread task sending notifications on the wrong thread :\ my fault

Comment 8

[Marco Bonardo [:mak]]

Attachment: much better

i've cleaned up the patch from foreign contents (that i'm going to split into other bugs), this is preventive maintainance only, actually this is a module that implements:

- a cleanup on idle timer, the check is done every 30 minutes, if the user is idle from 10 minutes we do a database cleanup task. This is done ONCE per day or session. So since it's done only once it should not prevent laptop from going standby, the low idle time is for the same reason. Shawn suggests to ask on dev.planning if this approach sounds ok or we should be more tight doing nothing on idle (though we should still try to avoid hitting the database while the user is navigating). Dietrich any thought about this?

- a database update every 5 minutes, this is a task that can be used to do common background operations like frecency calculations.

The biggest doubt i have about this is if it should be a js module at all (it is now), or a service (don't think), or simply included into PlacesUtils.

PS: clearly the cleanup tasks are not complete, they can be added later though, for now i'm only cleaning corrupt tag containers that have been generated by us allowing creating folders inside them (in alpha stage).

Comment 9

[Marco Bonardo [:mak]]

Attachment: much much better

oops, forgot to hg add the module :\

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 341250 [details] [diff] [review]
much much better

Shawn, as requested, it would be good if you could take a look and explain your doubts, ideas on the direction i could move this on.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 341250 [details] [diff] [review]
much much better

I need to move this work to async queries

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v1

completely revised as a component, this for now uses async queries, executed in FIFO order through handleCompletion.
I've put placeholders for future maintainance tasks, i'll add those queries after the skeleton will be solid.

asking a first-pass review to start moving this toward better directions.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 341970 [details] [diff] [review]
patch v1

Looks good for the most part.  I think we should probably wait for the async multiple statements stuff, but at the same time, it looks like that'd be easy to do with the given code.

There's a better way to do smart getters though.  See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/PlacesBackground.jsm#102

Also, for code clarity, you should consider using the params object on mozIStorageStatement and using named parameters:
http://mxr.mozilla.org/mozilla-central/source/storage/public/mozIStorageStatement.idl#165

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> (From update of attachment 341970 [details] [diff] [review])
> Looks good for the most part.  I think we should probably wait for the async
> multiple statements stuff, but at the same time, it looks like that'd be easy
> to do with the given code.

i've retained a local array to be able to switch faster to that... however we don't need the multiple statements transaction everywhere, so we could mix-up the two approaches, when executing the next statement we could detect if it's a statement or an array. if it's an array we could use multiple statements transaction on the contained queries, if it's a query simply do executeAsync... so we could limit the transaction use and lasting time to certain tasks.

If so, we could take this as a first step and add multiple statements when we will move frecency here. I'd like to take this first to clenaup tag containers that are hanging around and don't allow users to backup/restore correctly.

i'll attach an updated patch following your suggestion and moving review to dietrich to have his suggestions. then we will see how far are multiple statements.

Comment 15

[Marco Bonardo [:mak]]

Shawn, for frecency updates if we can't get a transaction, we could use a temp table and a delete trigger like:

CREATE TEMP TABLE frecency_temp (
  id INTEGER PRIMARY KEY,
  frecency INTEGER NOT NULL DEFAULT 0
);

CREATE TEMP TRIGGER frecency_temp_update_on_delete
BEFORE DELETE ON frecency_temp FOR EACH ROW
BEGIN
   UPDATE moz_places SET frecency = OLD.frecency WHERE id = OLD.id;
END;

So on every timer tick we would do 50 (TBD) inserts like:
INSERT INTO frecency_temp VALUES(our_place_id, new_frecency_value);

and we will do an async
DELETE FROM frecency_temp;

i suppose (but can't be sure, th docs says for deletes an automatic transaction is created) the delete with the trigger would happen in an automatic transaction created for us by sqlite.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> i suppose (but can't be sure, th docs says for deletes an automatic transaction
> is created) the delete with the trigger would happen in an automatic
> transaction created for us by sqlite.
Yes, in fact the database synchronization code uses this very fact.  The trigger happens as a part of the transaction as well.

Comment 17

[Marco Bonardo [:mak]]

(In reply to comment #16)
> (In reply to comment #15)
> > i suppose (but can't be sure, th docs says for deletes an automatic transaction
> > is created) the delete with the trigger would happen in an automatic
> > transaction created for us by sqlite.
> Yes, in fact the database synchronization code uses this very fact.  The
> trigger happens as a part of the transaction as well.

so we could leave multiple-statements-with-transaction patch for future and starting from here, everything will work for now.

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Actually this implements a bunch of coherence checks and appear quite fast. i've been able to implement every update as a single statements so there's no problem actually without multiple-statements support.
It's a lot of queries, but notice that the most (ideally all of them) will return/delete/update empty resultsets.
I tested it on my default profile that i use every day and has been migrated from FX2, and it fixed a positions error, no problems so far and fix appear correct. i also tested this on some corrupt places.sqlite files i had around and some issue was mitigated (empty library).

additional tasks should be added in future imho, i would not expand this further for now.

Comment 19

[Marco Bonardo [:mak]]

This includes Shawn's suggestions from comment 13
For a possible way to implement frecency updates see comment 15

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 342248 [details] [diff] [review]
patch v1.1


>+
>+  // Delayed initialization of PlacesDBUtils.
>+  // This component checks for database coherence once per session or day, on
>+  // an idle timer, and executes update tasks on a repeating timer.
>+  setTimeout(function() PlacesUtils.startPlacesDBUtils(), 15000);

once per session sounds like a bit much. i install/uninstall a few extensions a day, constituting multiple sessions.

>+// Do cleanup after 10 minutes of idle.
>+// We choose a small idle time because we must not prevent laptops from going
>+// to standby, also we don't want to hit cpu/disk while the user is doing other
>+// activities on the computer, like watching a movie.
>+// So, we suppose that after 10 idle minutes the user is moving to another task
>+// and we can hit without big troubles.
>+const IDLE_TIMEOUT = 10 * 60 * 1000;

i don't understand the comment - this could hit while 10 mins into a movie...

i think that's the cost of doing this work on idle, which i'm not opposed to as long as it's not continuous. my complaint is that the comment is misleading.

>+  try {
>+    let lastCleanup = this._prefs.getIntPref(PREF_LAST_CLEANUP);
>+    let lastCleanupDate = new Date(lastCleanup * 1000);
>+    let dayOfCleanup = new Date(lastCleanupDate.getFullYear(),
>+                                lastCleanupDate.getMonth(),
>+                                lastCleanupDate.getDate());
>+    let now = new Date();
>+    let today = new Date(now.getFullYear(),
>+                         now.getMonth(),
>+                         now.getDate());
>+    // If we already did a cleanup for today or the date is set in the future
>+    // (disable cleanup) we won't do anything.
>+    if (today - dayOfCleanup < CLEANUP_DAYS_REPEAT)
>+      return;
>+  }

iirc, you can use nsIUpdateTimerManager to do once-per-day update checks, removing the need for the pref and all this.

//////////////////////////////////////////////////////////////////////////////
>+  //// Properties required for XPCOM registration:
>+
>+  classDescription: "Executes timer-activated maintainance tasks on Places database",

spelling nit: maintenance

>+  handleCompletion: function(aReason) {
>+    switch (aReason) {
>+      case Ci.mozIStorageStatementCallback.REASON_ERROR:
>+      case Ci.mozIStorageStatementCallback.REASON_CANCELED:
>+      case Ci.mozIStorageStatementCallback.REASON_FINISHED:
>+        if (this._statements.length) {
>+          // execute the next statement
>+          this._statements.shift().executeAsync(this);
>+        }
>+        else {
>+          // finished executing all statements.
>+          // Sending Begin/EndUpdateBatch notification will ensure that the UI
>+          // is correctly refreshed.
>+          this._history.runInBatchMode({runBatched: function(aUserData){}},
>+                                       null);
>+          this._bms.runInBatchMode({runBatched: function(aUserData){}}, null);
>+        }
>+        break;
>+    }
>+  },
>+
>+  

nit: whitespace

>+  //// Tasks
>+
>+  cleanUpOnIdle: function PDBU_cleanUpOnIdle() {
>+    // update the last-cleanup preference
>+    this._prefs.setIntPref(PREF_LAST_CLEANUP, Math.round(Date.now() / 1000));
>+
>+    var cleanupStatements = [];
>+

for each of these tasks, please add the relevant bug # to the comment. given how particular these are, it'd help to be able to point to more info.

>+  updateOnTimer: function PDBU_updateOnTimer() {
>+    // Update tasks (eg. frecency, expiration) go here
>+  }

i think that this should no-op if we're idle when it hits, to avoid bugs like 449124.

>+  /**
>+   * Starts the database coherence check and executes update tasks on a timer,
>+   * this method is called by browser.js in delayed startup.
>+   */
>+  startPlacesDBUtils: function PU_startPlacesDBUtils() {
>+    let placesDBUtils = Cc["@mozilla.org/places/DBUtils;1"].
>+                        getService().wrappedJSObject;
>   }
> };

why is this a component? it has no specialized interfaces, and we'd avoid xpcom registration penalty if it's a module.

also, can you test this with a very large database, and see:

1) how long it takes
2) how bad it hits cpu while active?

finally, given the amount of database manipulation, this needs to have a test that confirms that each targeted corruption/integrity issue is properly detected and resolved, and that non-matching data is still intact.

Comment 21

[Marco Bonardo [:mak]]

(In reply to comment #20)
> (From update of attachment 342248 [details] [diff] [review])
> >+  // Delayed initialization of PlacesDBUtils.
> >+  // This component checks for database coherence once per session or day, on
> >+  // an idle timer, and executes update tasks on a repeating timer.
> >+  setTimeout(function() PlacesUtils.startPlacesDBUtils(), 15000);
> 
> once per session sounds like a bit much. i install/uninstall a few extensions a
> day, constituting multiple sessions.

it's once per day, probably the comment needs to be better, it's per session if the session is longer than one day... but using nsIUpdateTimerManager, as you suggest, i could solve the problem.

> >+// Do cleanup after 10 minutes of idle.
> >+// We choose a small idle time because we must not prevent laptops from going
> >+// to standby, also we don't want to hit cpu/disk while the user is doing other
> >+// activities on the computer, like watching a movie.
> >+// So, we suppose that after 10 idle minutes the user is moving to another task
> >+// and we can hit without big troubles.
> >+const IDLE_TIMEOUT = 10 * 60 * 1000;
> 
> i don't understand the comment - this could hit while 10 mins into a movie...

yes need to fix comment, what i want to do is having a small idle time, not too small because the user could have leaved the computer for some minute, not too high because the user is maybe seeing a long movie. And the computer wants to go standby!

> 
> >+  updateOnTimer: function PDBU_updateOnTimer() {
> >+    // Update tasks (eg. frecency, expiration) go here
> >+  }
> 
> i think that this should no-op if we're idle when it hits, to avoid bugs like
> 449124.

actually this can completely go away since we don't have tasks to do here, frecency is probably going to its own module, and the same for expiration. so i'll get rid of this and we will reinsert it in future if needed.

> >+  /**
> >+   * Starts the database coherence check and executes update tasks on a timer,
> >+   * this method is called by browser.js in delayed startup.
> >+   */
> >+  startPlacesDBUtils: function PU_startPlacesDBUtils() {
> >+    let placesDBUtils = Cc["@mozilla.org/places/DBUtils;1"].
> >+                        getService().wrappedJSObject;
> >   }
> > };
> 
> why is this a component? it has no specialized interfaces, and we'd avoid xpcom
> registration penalty if it's a module.

right, i had some doubt about this, but i agree, will do as an importable module.

> 
> also, can you test this with a very large database, and see:
> 
> 1) how long it takes
> 2) how bad it hits cpu while active?

i'll take some timing, not easy due to the use of async statement (i can't know when they start) but i'll take the global time, however it has been quite fast on all my dbs for now, since, as said, ideally all of these queries should return nothing, so no writes and no resultsets to retain in memory.

> finally, given the amount of database manipulation, this needs to have a test
> that confirms that each targeted corruption/integrity issue is properly
> detected and resolved, and that non-matching data is still intact.

yes, finally yes

Comment 22

[Marco Bonardo [:mak]]

Attachment: patch v1.2 with test

This is one of those cases where the test is much bigger than the code, and this is huge, so, this is not a funny review, hwv i'd like at least a first pass to evaluate the new approach.

I've disabled some cleanup task that is more complex or is touching up critical areas, we can enable them later to have a better review of those parts too, queries should be correct but still missing specific tests.

Every test is specific and i've checked that it fails if the cleanup task is not executed.

As for perf, i've tried on a big random db (130000 visits, 4000 bookmarks, 18000 places) with and the full task from call to maintenanceOnIdle to finish notification has last for about 800ms, enabling all tasks (critical too) that raised to 2.5s, cpu utilization and time varies based on how much the db is corrupted (hoping it is never all queries will return empty rsultsets).

Comment 23

[Marco Bonardo [:mak]]

PS: i haven't used dbConn.executeAsync due to the sqlite bug, so this is still using the old array way of executing, that should also ensure a more distributed load.

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 346892 [details] [diff] [review]
patch v1.2 with test

(In reply to comment #23)
> PS: i haven't used dbConn.executeAsync due to the sqlite bug, so this is still
> using the old array way of executing, that should also ensure a more
> distributed load.
That bug is blocking 3.1, but you stmt.executeAsync's are serialized now, so you can get rid of that complexity.

>diff --git a/toolkit/components/places/src/PlacesDBUtils.jsm b/toolkit/components/places/src/PlacesDBUtils.jsm
>+  handleError: function PDBU_handleError(aError) {
>+    Components.utils.reportError("Async statement execution returned with '" +
>+                                 aError.result + "', '" + aError.message + "'");
nit: declare Cu please and use it.

Otherwise r=me.  Dietrich should look at this too.

Also note, this will leak madly until bug 464202 and bug 457743 get checked in (both have reviews and are blocking).

Comment 25

[Marco Bonardo [:mak]]

Attachment: patch v1.3

addressed Shawn's comments, ready for final review.

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 349766 [details] [diff] [review]
patch v1.3


>diff --git a/toolkit/components/places/src/PlacesDBUtils.jsm b/toolkit/components/places/src/PlacesDBUtils.jsm
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/src/PlacesDBUtils.jsm
>@@ -0,0 +1,559 @@

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+let EXPORTED_SYMBOLS = [ "PlacesDBUtils" ];
>+

nit: do this after you declare the C* consts, so you can use Cu

>+const FINISHED_MAINTANANCE_NOTIFICATION_TOPIC = "places-maintenance-finished";

spelling nit: maintenance

>+// Do maintenance after 10 minutes of idle.
>+// We choose a small idle time because we must not prevent laptops from going
>+// to standby, also we don't want to hit cpu/disk while the user is doing other
>+// activities on the computer, like watching a movie.
>+// So, we suppose that after 10 idle minutes the user is moving to another task
>+// and we can hit without big troubles.
>+const IDLE_TIMEOUT = 10 * 60 * 1000;

what exactly prevents standby? disk activity?

>+  //// nsITimerCallback
>+
>+  notify: function PDBU_notify(aTimer) {
>+    switch (aTimer) {
>+      case this._idleLookUpTimer:
>+        let idleTime = 0;
>+        try {
>+          idleTime = this._idlesvc.idleTime;
>+        } catch (ex) {
>+          return;

nit: the early return isn't necessary, since the default (0) will always be less than IDLE_TIMEOUT.

>+    // D.2 remove items that are not uri bookmarks from tag containers
>+    let deleteBogusTagChildren = this._dbConn.createStatement(
>+      "DELETE FROM moz_bookmarks WHERE id IN (" +
>+        "SELECT b.id FROM moz_bookmarks b " +
>+        "WHERE b.parent IN " +
>+          "(SELECT id FROM moz_bookmarks WHERE parent = :tags_folder) " +
>+          "AND b.type <> :bookmark_type " +
>+      ")");

does sqlite cache the results of this sub-select query? otherwise, it looks like it'd need to scan the whole bookmarks table many times over.

>+    // D.4 move orphan items to unsorted folder
>+    let fixOrphanItems = this._dbConn.createStatement(
>+      "UPDATE moz_bookmarks SET parent = :unsorted_folder WHERE id IN (" +
>+        "SELECT b.id FROM moz_bookmarks b " +
>+        "WHERE b.parent <> 0 " + // exclude root
>+        "AND NOT EXISTS " +
>+          "(SELECT id FROM moz_bookmarks WHERE id = b.parent) " +
>+      ")");
>+    fixOrphanItems.params["unsorted_folder"] = this._bms.unfiledBookmarksFolder;
>+    cleanupStatements.push(fixOrphanItems);

hm, maybe only do this for type=bookmark? i'm worried that previous corruption issues may have left orphaned left-pane hierarchies, etc. these would suddenly show up in Unfiled Bookmarks.

>+
>+    // D.5 fix wrong keywords
>+    let fixInvalidKeywords = this._dbConn.createStatement(
>+      "UPDATE moz_bookmarks SET keyword_id = NULL WHERE id IN ( " +
>+        "SELECT id FROM moz_bookmarks b " +
>+        "WHERE keyword_id NOT NULL " +
>+          "AND NOT EXISTS " +
>+            "(SELECT id FROM moz_keywords WHERE id = b.keyword_id) " +
>+      ")");
>+    cleanupStatements.push(fixInvalidKeywords);
>+
>+    // D.6 fix wrong item types
>+    //     Folders, separators and dynamic containers should not have an fk.
>+    //     If they have a valid fk convert them to bookmarks.
>+    // Later we will move eventual children to unsorted bookmarks.

about that last sentence: where does this happen? and how could it? or do you mean only the ones that have an invalid parent?

>+    let fixBookmarksAsFolders = this._dbConn.createStatement(
>+      "UPDATE moz_bookmarks SET type = :bookmark_type WHERE id IN ( " +
>+        "SELECT id FROM moz_bookmarks b " +
>+        "WHERE type IN (:folder_type, :separator_type, :dynamic_type) " +
>+          "AND fk NOTNULL " +
>+      ")");
>+    fixBookmarksAsFolders.params["bookmark_type"] = this._bms.TYPE_BOOKMARK;
>+    fixBookmarksAsFolders.params["folder_type"] = this._bms.TYPE_FOLDER;
>+    fixBookmarksAsFolders.params["separator_type"] = this._bms.TYPE_SEPARATOR;
>+    fixBookmarksAsFolders.params["dynamic_type"] = this._bms.TYPE_DYNAMIC_CONTAINER;
>+    cleanupStatements.push(fixBookmarksAsFolders);

how would this occur? is there a corresponding bug for this?

also, without knowing more about how this occurs, i'm not comfortable having these show up in random spots in the hierarchy. what if the parent is the tag root, or the left pane root.

>+
>+    // D.7 fix wrong item types
>+    //     Bookmarks should have an fk, if they don't have any, convert them to
>+    //     folders.
>+    let fixFoldersAsBookmarks = this._dbConn.createStatement(
>+      "UPDATE moz_bookmarks SET type = :folder_type WHERE id IN ( " +
>+        "SELECT id FROM moz_bookmarks b " +
>+        "WHERE type = :bookmark_type " +
>+          "AND fk IS NULL " +
>+      ")");
>+    fixFoldersAsBookmarks.params["bookmark_type"] = this._bms.TYPE_BOOKMARK;
>+    fixFoldersAsBookmarks.params["folder_type"] = this._bms.TYPE_FOLDER;
>+    cleanupStatements.push(fixFoldersAsBookmarks);

hm. again, is there a known bug causing this? i'm loathe to cause someone's bookmarks to suddenly sprout a million folders that have no name because they used to be separators.

>+    // D.11 remove old livemarks status items
>+    //      Livemark status items are now static but some livemark has still old
>+    //      status items bookmarks inside it. We should remove them.
>+    //      Note: This does not need to query the temp table.
>+    let removeLivemarkStaticItems = this._dbConn.createStatement(
>+      "DELETE FROM moz_bookmarks WHERE fk IN ( " +
>+        "SELECT id FROM moz_places WHERE url > :lmtoken1 AND url < :lmtoken2 " +
>+      ")");
>+    removeLivemarkStaticItems.params["lmtoken1"] = "about:livemark-a";
>+    removeLivemarkStaticItems.params["lmtoken2"] = "about:livemark-z";
>+    cleanupStatements.push(removeLivemarkStaticItems);

we know what about:livemark-* permutations were used, right? is this faster or slower than an exact comparison?

>+
>+    // MOZ_FAVICONS
>+    // E.1 remove orphan icons

this and the other orphan-culling statements currently occur in the expiration code, right? is there a particular reason why you're running this now?

>+    // MOZ_PLACES
>+    // L.1 fix wrong favicon ids
>+    let fixInvalidFaviconIds = this._dbConn.createStatement(
>+      "UPDATE moz_places SET favicon_id = NULL WHERE id IN ( " +
>+        "SELECT id FROM moz_places h " +
>+        "WHERE favicon_id NOT NULL " +
>+          "AND NOT EXISTS " +
>+            "(SELECT id FROM moz_favicons WHERE id = h.favicon_id) " +
>+      ")");
>+    cleanupStatements.push(fixInvalidFaviconIds);

would a "LIMIT 1" speed this up? and some of the other sub-selects too, iirc

Comment 27

[Marco Bonardo [:mak]]

(In reply to comment #26)
> >+// Do maintenance after 10 minutes of idle.
> >+// We choose a small idle time because we must not prevent laptops from going
> >+// to standby, also we don't want to hit cpu/disk while the user is doing other
> >+// activities on the computer, like watching a movie.
> >+// So, we suppose that after 10 idle minutes the user is moving to another task
> >+// and we can hit without big troubles.
> >+const IDLE_TIMEOUT = 10 * 60 * 1000;
> 
> what exactly prevents standby? disk activity?

our activity, disk/cpu

> >+    // D.2 remove items that are not uri bookmarks from tag containers
> >+    let deleteBogusTagChildren = this._dbConn.createStatement(
> >+      "DELETE FROM moz_bookmarks WHERE id IN (" +
> >+        "SELECT b.id FROM moz_bookmarks b " +
> >+        "WHERE b.parent IN " +
> >+          "(SELECT id FROM moz_bookmarks WHERE parent = :tags_folder) " +
> >+          "AND b.type <> :bookmark_type " +
> >+      ")");
> 
> does sqlite cache the results of this sub-select query? otherwise, it looks
> like it'd need to scan the whole bookmarks table many times over.

Sure, if you execute an EXPLAIN QUERY PLAN you see we are executing 2 queries, the internal subquery is executed first, the second one is executed using results table from the first.

> >+    // D.4 move orphan items to unsorted folder
> >+    let fixOrphanItems = this._dbConn.createStatement(
> >+      "UPDATE moz_bookmarks SET parent = :unsorted_folder WHERE id IN (" +
> >+        "SELECT b.id FROM moz_bookmarks b " +
> >+        "WHERE b.parent <> 0 " + // exclude root
> >+        "AND NOT EXISTS " +
> >+          "(SELECT id FROM moz_bookmarks WHERE id = b.parent) " +
> >+      ")");
> >+    fixOrphanItems.params["unsorted_folder"] = this._bms.unfiledBookmarksFolder;
> >+    cleanupStatements.push(fixOrphanItems);
> 
> hm, maybe only do this for type=bookmark? i'm worried that previous corruption
> issues may have left orphaned left-pane hierarchies, etc. these would suddenly
> show up in Unfiled Bookmarks.

We need to recover orphaned folders too, suppose there's a folder with bookmarks in it, if we don't move it will be lost.
I think recovering old left panes is not a big deal, user can delete them from unsorted. Also notice many left pane children are type bookmark (queries and folder shortcuts are), so there's not much we can do to filter them out.

> >+    // D.6 fix wrong item types
> >+    //     Folders, separators and dynamic containers should not have an fk.
> >+    //     If they have a valid fk convert them to bookmarks.
> >+    // Later we will move eventual children to unsorted bookmarks.
> 
> about that last sentence: where does this happen? and how could it? or do you
> mean only the ones that have an invalid parent?

in D.9, since children have now a bookmark type parent (invalid) they'll be moved to unsorted. fixed comment.
The idea is to move bogus entries to unsorted folder so the user can clean them up manually, we can't take a decision on them.

> >+    let fixBookmarksAsFolders = this._dbConn.createStatement(
> >+      "UPDATE moz_bookmarks SET type = :bookmark_type WHERE id IN ( " +
> >+        "SELECT id FROM moz_bookmarks b " +
> >+        "WHERE type IN (:folder_type, :separator_type, :dynamic_type) " +
> >+          "AND fk NOTNULL " +
> >+      ")");
> >+    fixBookmarksAsFolders.params["bookmark_type"] = this._bms.TYPE_BOOKMARK;
> >+    fixBookmarksAsFolders.params["folder_type"] = this._bms.TYPE_FOLDER;
> >+    fixBookmarksAsFolders.params["separator_type"] = this._bms.TYPE_SEPARATOR;
> >+    fixBookmarksAsFolders.params["dynamic_type"] = this._bms.TYPE_DYNAMIC_CONTAINER;
> >+    cleanupStatements.push(fixBookmarksAsFolders);
> 
> how would this occur? is there a corresponding bug for this?

No, and i hope we'll never hit such a thing, but preventive maintenance is to prevent issues, this could only happen if a user mess up the db manually. I've tried to cover uncoherences that could bring to wrong behaviour on our side.

> also, without knowing more about how this occurs, i'm not comfortable having
> these show up in random spots in the hierarchy. what if the parent is the tag
> root, or the left pane root.

If the parent is the tag root they will be cleaned up on next run, if the parent is left pane root we have an issue and i hope the user will report it to us. Hwv in such a case the user would already have something bad in the left pane and we would be aware of it.
This is mainly to recover lost bookmarks that for any reason has got a wrong type. Something that could happen only due to an extension or direct database editing.

Do you want to comment out the task?

> >+
> >+    // D.7 fix wrong item types
> >+    //     Bookmarks should have an fk, if they don't have any, convert them to
> >+    //     folders.
> >+    let fixFoldersAsBookmarks = this._dbConn.createStatement(
> >+      "UPDATE moz_bookmarks SET type = :folder_type WHERE id IN ( " +
> >+        "SELECT id FROM moz_bookmarks b " +
> >+        "WHERE type = :bookmark_type " +
> >+          "AND fk IS NULL " +
> >+      ")");
> >+    fixFoldersAsBookmarks.params["bookmark_type"] = this._bms.TYPE_BOOKMARK;
> >+    fixFoldersAsBookmarks.params["folder_type"] = this._bms.TYPE_FOLDER;
> >+    cleanupStatements.push(fixFoldersAsBookmarks);
> 
> hm. again, is there a known bug causing this? i'm loathe to cause someone's
> bookmarks to suddenly sprout a million folders that have no name because they
> used to be separators.

Imho better seeing those folders and being able to delete them than having them hidden around... same as above.

> 
> >+    // D.11 remove old livemarks status items
> >+    //      Livemark status items are now static but some livemark has still old
> >+    //      status items bookmarks inside it. We should remove them.
> >+    //      Note: This does not need to query the temp table.
> >+    let removeLivemarkStaticItems = this._dbConn.createStatement(
> >+      "DELETE FROM moz_bookmarks WHERE fk IN ( " +
> >+        "SELECT id FROM moz_places WHERE url > :lmtoken1 AND url < :lmtoken2 " +
> >+      ")");
> >+    removeLivemarkStaticItems.params["lmtoken1"] = "about:livemark-a";
> >+    removeLivemarkStaticItems.params["lmtoken2"] = "about:livemark-z";
> >+    cleanupStatements.push(removeLivemarkStaticItems);
> 
> we know what about:livemark-* permutations were used, right? is this faster or
> slower than an exact comparison?

Usually doing an exact comparison requires an OR that is slower than an AND because has to generate 2 resultsets and merge them. In this case however due to small amount of entries it's same speed, so i'm moving to exact comparison.

> 
> >+
> >+    // MOZ_FAVICONS
> >+    // E.1 remove orphan icons
> 
> this and the other orphan-culling statements currently occur in the expiration
> code, right? is there a particular reason why you're running this now?

it depends, some orphan culling code here is not in expiration, some is, doing it here could reduce work at shutdown. Maybe when we'll reimplement expiration to run async we could normalize tasks between the two.

So, generally, if you want to comment out any task give me the task code and i'll do.

Comment 28

[Marco Bonardo [:mak]]

Attachment: patch v1.4

fixed all other comments, see my comment above.

Comment 29

[Nochum Sossonko [:Natch]]

IMHO there should be a button in the Library (something like "fix my library" or some such) that an end user could hit should the database become corrupted, since, AFAICT, the maintenance is only triggered on an idle timeout trigger. Also there should be some notice sent to the user when there was corruption found, 1. just the fact that corruption was found and fixed, 2) where the corrupted bookmarks are to be found. Otherwise instead of getting a bunch of "places.sqlite corrupted" bugs you'll get "my bookmarks suddenly disappeared" or "why are there bookmarks in unfiled bookmarks when I put none there".

Comment 30

[Marco Bonardo [:mak]]

(In reply to comment #29)
> IMHO there should be a button in the Library (something like "fix my library"
> or some such) that an end user could hit should the database become corrupted,
> since, AFAICT, the maintenance is only triggered on an idle timeout trigger.

Could be a followup

> Also there should be some notice sent to the user when there was corruption
> found

would be cool, but it's not possible unless we duplicate all queries to execute an async select with a dedicated listener before the maintenance task and we serve every result... that will multiply the needed time by 2 or 3.
At a maximum we could see how many rows the statement has affected, that would still require a listener for EVERY statement, and i'm not sure if it is feasible with async though.

Comment 31

[Dietrich Ayala (:dietrich)]

Comment on attachment 351153 [details] [diff] [review]
patch v1.4

r=me. lets roll with it, and see how nightly testers respond to the items showing up in unsorted folder.

Comment 32

[Marco Bonardo [:mak]]

pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/5f7a0e667bc4

Comment 33

[Marco Bonardo [:mak]]

Attachment: followup

i forgot to remove the observer in the test

Comment 34

[Shawn Wilsher :sdwilsh]

Comment on attachment 352222 [details] [diff] [review]
followup

r=sdwilsh

Comment 35

[Marco Bonardo [:mak]]

followup pushed on trunk:
http://hg.mozilla.org/mozilla-central/rev/9952fee851da

Comment 36

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0f8d99712c92
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9ab06446588b

Comment 37

[Tracy Walker [:tracy]]

verified per code checkins

Comment 40

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv