478912 - bookmarks initialization is not fault tolerant

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect, P3)

Description

[Dietrich Ayala (:dietrich)]

see:

- https://bugzilla.mozilla.org/show_bug.cgi?id=478258#c6
- https://bugzilla.mozilla.org/show_bug.cgi?id=478258#c14
- https://bugzilla.mozilla.org/show_bug.cgi?id=478258#c16

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP v1

This addresses:

- bug 478258 comment 6
- bug 478258 comment 16
- bug 478258 comment 14 except for this part about tests:

> Going this way we should ensure roots cannot be removed, using tests, without
> having to check for them at every start. So solving this bug will give us an
> idea of what we should avoid.

Comment 2

[Marco Bonardo [:mak]]

basicly what is missing is the fact the root entry could exist in moz_bookmarks_roots, but the folder in moz_bookmarks could be missing, we should ensure both do exist and take corrective actions. If you want to refactor part of the init to try making it faster you're welcome to. I think at this point (After 3.5 release) we can also start discarding more migration code from 3.0 alphas/betas.

Comment 3

[Drew Willcoxon :adw]

Attachment: WIP v2

Marco, could you take a look at the changes to nsNavBookmarks::CreateRoot() to see if I'm on the right track?

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 376323 [details] [diff] [review]
WIP v2

Requesting first review to make sure I'm on the right track.  The substantial changes are to nsNavBookmarks::CreateRoot, so I'm mainly interested in that right now.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 376323 [details] [diff] [review]
WIP v2

approach looks fine. i'm not a fan of early returns unless they're extremely clear and not hidden away inside conditional blocks, so please remove them in CreateRoot.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 376323 [details] [diff] [review]
WIP v2

Really my idea for .next is to completely get rid of moz_bookmarks_roots, since it's a crazy and useless table.
But that again opens downgrade issues.

>@@ -110,19 +113,30 @@ nsNavBookmarks::Init()
>   nsNavHistory *history = History();
>   NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
>   mDBConn = history->GetStorageConnection();
>   mozStorageTransaction transaction(mDBConn, PR_FALSE);
> 
>   nsresult rv = InitStatements();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  //XXXadw hmm... this screws things up
>+//   PRUint16 dbStatus;
>+//   rv = history->GetDatabaseStatus(&dbStatus);
>+//   NS_ENSURE_SUCCESS(rv, rv);
>+
>+//   if (dbStatus != nsINavHistoryService::DATABASE_STATUS_OK) {
>+//     rv = InitRoots();
>+//     NS_ENSURE_SUCCESS(rv, rv);
>+//   }

i think you should init roots when status is DATABASE_STATUS_CREATE or _UPGRADE?
We should also think to the case when we want to add a new root... but probably we could consider that a schema upgrade (not completely correct though)


> nsresult
> nsNavBookmarks::CreateRoot(mozIStorageStatement* aGetRootStatement,

>+        if (aWasCreated)
>+          *aWasCreated = PR_FALSE;
>+        return NS_OK;
>+      }
>+
>+      // Else, the moz_bookmarks row is not a folder.  We'll create a new folder
>+      // below and update the moz_bookmarks_roots entry.

What happens to the wrong entry in moz_bookmarks in this case? will be a dead child of root?

> // nsNavBookmarks::FillBookmarksHash
> //
> //    This initializes the bookmarks hashtable that tells us which bookmark
>@@ -1190,20 +1292,25 @@ nsNavBookmarks::RemoveItem(PRInt64 aItem
>   mozStorageTransaction transaction(mDBConn, PR_FALSE);
> 
>   // First, remove item annotations
>   nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
>   NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
>   rv = annosvc->RemoveItemAnnotations(aItemId);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = ");
>-  buffer.AppendInt(aItemId);

could you fix also removeItem that looks like doing the same stupid append please?

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)

> > nsresult
> > nsNavBookmarks::CreateRoot(mozIStorageStatement* aGetRootStatement,
> 
> >+        if (aWasCreated)
> >+          *aWasCreated = PR_FALSE;
> >+        return NS_OK;
> >+      }
> >+
> >+      // Else, the moz_bookmarks row is not a folder.  We'll create a new folder
> >+      // below and update the moz_bookmarks_roots entry.
> 
> What happens to the wrong entry in moz_bookmarks in this case? will be a dead
> child of root?

Also, in this case you change a root id, do we ensure that children of the old broken root are reparented to this new one? or they'll become orphans.

Comment 8

[Drew Willcoxon :adw]

Attachment: WIP v3

I changed my approach a little.  There's a big comment in the patch explaining things, but basically this approach focuses on the "folder pointer" pointing from moz_bookmarks_roots into moz_bookmarks.  If it doesn't point to what we expect, we can either update the pointer or update the (potentially innocent (we have no idea)) item it points to.  So let's update the pointer.  First try to find the missing root by looking it up in moz_bookmarks based on the expected parent and locale title.  If we find it, great, if not, make a new one.  Either way update the pointer.

I plan to follow the testing approach of Shawn's download manager schema migration tests.  Have a set of broken places.sqlites, load them one by one into profdir, init the bookmarks service, and check that everything went OK.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/schema_migration/

(In reply to comment #6)
> i think you should init roots when status is DATABASE_STATUS_CREATE or
> _UPGRADE?

The problem is that CreateRoot sets all the various root ID member variables even when it doesn't actually create anything.

> >-  buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = ");
> >-  buffer.AppendInt(aItemId);
> 
> could you fix also removeItem that looks like doing the same stupid append
> please?

A quick search shows that lots of places in this one file are doing similar things.  Fixing them all seems out of scope here.  New bug?

Comment 9

[Drew Willcoxon :adw]

Attachment: for review v1

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 378258 [details] [diff] [review]
for review v1

notice i still have to read this deeper/better, just some quick comment on a first look

Do we have a measure if this makes createRoot slower than actual one?
I'm still not sure we should call initRoots at every startup, or rather _change it_ so that i can be called only for create/upgrade/downgrade/corrupt cases (so when db status is not OK). We should still try to gain on Ts as much as possible. At the same time if we don't check roots we could be initializing badly. maybe we should split the work, in case DB status is OK simply execute a join between moz_bookmarks and moz_bookmarks_roots to get data we need, if something is wrong (the join does not return all we expect) fallback to the other case.

So i think would be great to revise the roots code if we can make it faster than actual one in the most common case.

>+nsresult
>+nsNavBookmarks::FindMissingRoot(const nsCString &aRootName,
>+                                PRInt64 aParentID,
>+                                PRInt64 *aRootID)
>+{
>+  NS_ENSURE_ARG_POINTER(aRootID);
>+  *aRootID = -1;
>+
>+  // Get the locale's title for the root in rootLocaleTitle.  All roots except
>+  // "places" have a title.
>+  nsIStringBundle *bundle = History()->GetBundle();
>+  NS_ENSURE_TRUE(bundle, NS_ERROR_OUT_OF_MEMORY);
>+
>+  nsAutoString rootLocaleKey;
>+  nsXPIDLString rootLocaleTitle;
>+  if (aRootName.Equals("menu"))
>+    rootLocaleKey.AssignLiteral("BookmarksMenuFolderTitle");
>+  else if (aRootName.Equals("toolbar"))
>+    rootLocaleKey.AssignLiteral("BookmarksToolbarFolderTitle");
>+  else if (aRootName.Equals("tags"))
>+    rootLocaleKey.AssignLiteral("TagsFolderTitle");
>+  else if (aRootName.Equals("unfiled"))
>+    rootLocaleKey.AssignLiteral("UnsortedBookmarksFolderTitle");
>+  else if (!aRootName.Equals("places"))
>+    NS_NOTREACHED("Unexpected root name");

maybe this could use an helper like ::GetTitleForRoot? could be used in initDefaults too (that method has a bad name and bad comments, could you look at it?)

>+  // Execute a query that will return the target.  If the root has a title,
>+  // use it in the query to narrow the search.
>+  nsCAutoString sql(
>+    "SELECT b.id "
>+    "FROM moz_bookmarks b "
>+    "WHERE b.type = 2 "
>+      "AND b.fk IS NULL "
>+      "AND b.parent = ?1 ");
>+  if (!rootLocaleTitle.IsEmpty())
>+    sql.AppendLiteral(
>+      "AND b.title = ?2 ");
>+  sql.AppendLiteral(
>+      "AND NOT EXISTS ("
>+        "SELECT r.folder_id "
>+        "FROM moz_bookmarks_roots r "
>+        "WHERE b.id = r.folder_id"
>+      ")");
>+  nsCOMPtr<mozIStorageStatement> stmt;
>+  mozStorageStatementScoper scoper(stmt);

what happens if i change locale? titles will be different.

>+    // Uh oh, we found more than one candidate folder.  Give up.
>+    if (hasResult)
>+      *aRootID = -1;

when could this happen? Does our code leave space for this case?

> // nsNavBookmarks::CreateRoot
> //
> //    This gets or creates a root folder of the given type. aWasCreated
> //    (optional) is true if the folder had to be created, false if we just used
> //    an old one. The statement that gets a folder ID from a root name is
> //    passed in so the DB only needs to parse the statement once, and we don't
> //    have to have a global for this. Creation is less optimized because it
> //    happens rarely.

do we have a javadoc in header? if not it should be here.

> nsresult
> nsNavBookmarks::CreateRoot(mozIStorageStatement* aGetRootStatement,
>-                           const nsCString& name, PRInt64* aID,
>-                           PRInt64 aParentID, PRBool* aWasCreated)
>+                           const nsCString& name,
>+                           PRInt64* aID,
>+                           PRInt64 aParentID,
>+                           PRBool* aWasCreated)
> {
>+  enum MozBookmarksRootsAction { NONE, CREATE, UPDATE };
>+  MozBookmarksRootsAction mozBookmarksRootsAction = NONE;
>+
>   PRBool hasResult = PR_FALSE;
>   nsresult rv = aGetRootStatement->BindUTF8StringParameter(0, name);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = aGetRootStatement->ExecuteStep(&hasResult);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  if (hasResult) {
>+
>+  // The folder has a row in moz_bookmarks_roots.
>+  if (NS_SUCCEEDED(rv) && hasResult) {
>+    rv = aGetRootStatement->GetInt64(0, aID);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Attempt to get the parent and type of the folder in moz_bookmarks.

you don't know if it has a row in moz_bookmarks yet.

>+    nsCOMPtr<mozIStorageStatement> stmt;
>+    mozStorageStatementScoper scoper(stmt);
>+    rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+        "SELECT parent, type FROM moz_bookmarks WHERE id = ?1"),
>+      getter_AddRefs(stmt));

you can use mDBGetItemProperties here.

>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->BindInt64Parameter(0, *aID);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->ExecuteStep(&hasResult);
>+
>+    // The folder has a row in moz_bookmarks.  Now get parent and type.

this comment should be inside the if

>+    PRInt64 existingParentId;
>+    PRInt32 existingType;
>+    if (NS_SUCCEEDED(rv) && hasResult) {
>+      rv = stmt->GetInt64(0, &existingParentId);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = stmt->GetInt32(1, &existingType);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+    // The folder does not have a row in moz_bookmarks.  ExecuteStep could have
>+    // failed, so make certain hasResult is false.

comment should be inside the else

>+    else
>+      hasResult = PR_FALSE;

would not be better to throw if we fail instead of thinking we don't have an entry?

>+    // If we're wrong either way, the real root and all its children remain
>+    // lost to the user.  Approach 1 doesn't screw up an innocent item if it's
>+    // wrong.  Choose it.
>+    if (!hasResult ||

hasResult should have better more explicative name

>+  // mozBookmarksRootsAction == NONE iff everything looks OK.

nit: iff

>+  // Now act on moz_bookmarks_roots appropriately.
>+  nsCOMPtr<mozIStorageStatement> stmt;
>+  if (mozBookmarksRootsAction == UPDATE) {
>+    rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+        "UPDATE moz_bookmarks_roots SET folder_id = ?1 WHERE root_name = ?2"),
>+      getter_AddRefs(stmt));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->BindInt64Parameter(0, *aID);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->BindUTF8StringParameter(1, name);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->Execute();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+  else if (mozBookmarksRootsAction == CREATE) {
>+    rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+        "INSERT INTO moz_bookmarks_roots (root_name, folder_id) "
>+        "VALUES (?1, ?2)"),
>+      getter_AddRefs(stmt));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->BindUTF8StringParameter(0, name);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->BindInt64Parameter(1, *aID);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = stmt->Execute();
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

you could probably use an INSERT OR REPLACE here, the unique index on root_name should help.

>diff --git a/toolkit/components/places/tests/bookmarks/head_createRoot.js b/toolkit/components/places/tests/bookmarks/head_createRoot.js

i doubt this head_ file will be added to all tests in tests/bookmarks/ folder

will check tests on next pass.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 378258 [details] [diff] [review]
for review v1

clearing request till next version of the patch

Comment 12

[Samuel Sidler (old account; do not CC)]

Dietrich/Drew: Is this still wanted for 1.9.1.x?

Comment 13

[Marco Bonardo [:mak]]

I'm taking this bug to see if I can recover some of the original patch and actually make this work as expected, we still have reports of users losing roots (no reason identified) and the product should keep working, even if there will clearly be a dataloss.

Comment 14

[Marco Bonardo [:mak]]

not actively working on this atm

Comment 15

[Marco Bonardo [:mak]]

I think the current situation is good enough, we removed years ago the separate root tables, we expose virtual nodes in the ui, and on databse initialization we have a method ensuring roots are in their place.