478258 - [PPC] Bookmark root folder gets deleted after some minutes or by user action (opening/closing tabs, reload live bookmark)

Status: RESOLVED FIXED

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: places.sqlite without bookmarks root folder

Even with bug 478218 fixed I can see that the places.sqlite for fresh profiles doesn't has a root bookmark folder. Means backups end up in 0kb and we will probably fail on other places.

See the attached places.sqlite which was created by adding a new profile.

I've tested a hourly build too which still shows the problem:
ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-1.9.1-macosx/1234432979/

Should this be a P1?

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> Should this be a P1?
Per discussion with beltzner, yes.

Comment 2

[Dietrich Ayala (:dietrich)]

taking

Comment 3

[Marco Bonardo [:mak]]

could also be that the root exist, but since we have a live bookmark in default bookmarks it gets deleted as soon as the live bookmark updates

Comment 4

[Marco Bonardo [:mak]]

oh but if you tested on an hourly with the previous fix, that's clearly a different issue :\ bad

Comment 5

[Dietrich Ayala (:dietrich)]

This needs to block B3. It basically disables bookmarks for PPC users, and trashes their database.

Comment 6

[Marco Bonardo [:mak]]

sorry i can't connect to IRC anymore (dunno why).
btw looking at bookmarks service i found

1196   buffer.AssignLiteral("DELETE FROM moz_bookmarks WHERE id = ");
1197   buffer.AppendInt(aItemId);

we use this in both removeItem and removeFolder. I suppose here should be better using bind parameter.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, this regressed between 08101202 and 08101513. I hope that helps you folks to get it fixed.

Comment 8

[Marco Bonardo [:mak]]

appendInt has also a PRInt64 overload, so could not be our issue... but in case, we use it in other places to build up queries, we should notice much more issues then.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> appendInt has also a PRInt64 overload, so could not be our issue... but in
> case, we use it in other places to build up queries, we should notice much more
> issues then.
not necessarily.  They could be small enough that nobody notices, but in this one case it matters.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: test

this should be enough to test that the record exists in the db.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Probably this prevents us from creating json backups, which is covered by bug 478447.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, after some deeper investigation of this problem I can swear that this issue isn't triggered by our initialization code. Instead the bookmarks root folder gets deleted whenever you open or close a tab.

Those steps I've used:
1. Created a new profile and copied over a working places.sqlite file
2. Open or close a tab
3. Shutdown Firefox

After step 3 no bookmarks root folder isn't there anymore.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dietrich, this doesn't happen with 10.4 on the other ppc box. So it seems like to be 10.5 only.

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 362247 [details] [diff] [review]
test

>diff --git a/browser/components/places/tests/browser/Makefile.in b/browser/components/places/tests/browser/Makefile.in
>--- a/browser/components/places/tests/browser/Makefile.in
>+++ b/browser/components/places/tests/browser/Makefile.in
>@@ -47,6 +47,7 @@
> 	browser_425884.js \
> 	browser_423515.js \
> 	browser_457473_no_copy_guid.js \
>+	browser_478258.js \

what about adding a _check_bookmarks_roots.js to the test name

>diff --git a/browser/components/places/tests/browser/browser_478258.js b/browser/components/places/tests/browser/browser_478258.js
>new file mode 100644

>+ * Portions created by the Initial Developer are Copyright (C) 2008

dunno why but we hate 2009 :)

>+
>+function test() {
>+  // get the id that a sub-root thinks is root
>+  var rootId = PlacesUtils.bookmarks.getFolderIdForItem(PlacesUtils.bookmarksMenuFolderId);
>+  ok(rootId > 0, "root id should be greater than zero");
>+
i would say to check for all the roots, check that all have the same parent, and that parent is == to bmsvc.placesRoot

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
>--- a/toolkit/components/places/src/nsNavBookmarks.cpp
>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>@@ -22,6 +22,9 @@
>  * Contributor(s):
>  *   Brian Ryner <[e-mail]> (original author)
>  *   Drew Willcoxon <[e-mail]>
>+ *   Dietrich Ayala <[e-mail]>
>+ *   Asaf Romano <[e-mail]>
>+ *   Marco Bonardo <[e-mail]>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>@@ -524,6 +527,7 @@
>   rv = CreateRoot(getRootStatement, NS_LITERAL_CSTRING("toolbar"), &mToolbarFolder, mRoot, &createdToolbarFolder);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // XXX remove me at some point when we won't support migration from pre-Fx3 final
>   // Once toolbar was not a root, we may need to move over the items and
>   // delete the custom folder
>   if (!createdPlacesRoot && createdToolbarFolder) {
>@@ -644,14 +648,13 @@
>   nsresult rv = aGetRootStatement->BindUTF8StringParameter(0, name);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = aGetRootStatement->ExecuteStep(&hasResult);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-  if (hasResult) {
>-    if (aWasCreated)
>-      *aWasCreated = PR_FALSE;
>+  if (NS_SUCCEEDED(rv) && hasResult) {
>     rv = aGetRootStatement->GetInt64(0, aID);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    NS_ASSERTION(*aID != 0, "Root is 0 for some reason, folders can't have 0 ID");
>-    return NS_OK;
>+    if (NS_SUCCEEDED(rv) && *aID != 0) {
>+      if (aWasCreated)
>+        *aWasCreated = PR_FALSE;
>+      return NS_OK;
>+    }

this is more a workaround to the issue, right? if the root does not exist we create it... but this does not work as expected, because this path looks only through moz_bookmarks_roots, and we don't have corruption there, we instead lose the real folder in moz_bookmarks.
I didn't follow all the discussion about this on IRC, but i doubt we have corruption in that table.
I second this explicit change since the assertion has no reason to be there, so please retain the change in next patches.

btw, i think for 3.2 we should call initRoots only in some case, looking at history->databaseStatus and going to check them only if status != OK, that would save some bit from Ts.
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 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> btw, i think for 3.2 we should call initRoots only in some case, looking at
> history->databaseStatus and going to check them only if status != OK, that
> would save some bit from Ts.
> 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.
or whenever we add a new root (not likely, but possible)

Comment 16

[Marco Bonardo [:mak]]

and while there i noticed this:

118   rv = FillBookmarksHash();
119   NS_ENSURE_SUCCESS(rv, rv);
120 
121   rv = InitRoots();
122   NS_ENSURE_SUCCESS(rv, rv);

personally i think having roots should come before filling bookmarks hash

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's really bad. While I was seeing this the whole morning today, each test after lunch failed. The root folder has never been deleted. I'll try again on Tuesday.

Comment 18

[Marco Bonardo [:mak]]

btw, iirc this also happened opening the browser and not doing anything for some minute, so we probably need to change the title.

Comment 19

[Dietrich Ayala (:dietrich)]

(In reply to comment #18)
> btw, iirc this also happened opening the browser and not doing anything for
> some minute, so we probably need to change the title.

livemarks refresh? that's the only action i can think of that both 1) acts w/o user interaction and 2) does a DELETE on the bookmarks table.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #19)
> livemarks refresh? that's the only action i can think of that both 1) acts w/o
> user interaction and 2) does a DELETE on the bookmarks table.

Mmh, so we could also manual trigger this via the context menu of a livemark folder?

Comment 21

[Marco Bonardo [:mak]]

if that's the case, reload live bookmark from the context menu... but last things we tried was to find an issue in removeFolderChildren (that the function uses) and we weren't able.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry, but there is no way to reproduce it. Meanwhile I think I got the wrong hourly build without the patch on bug 478218. I tried everything which could have been raised this problem but all is still fine. Seems like it has been solved.

Comment 23

[Dietrich Ayala (:dietrich)]

I'd like to resolve this INVALID per comment #22. However, comment #7 gives us a really specific regression range, indicating that this is a problem for PPC builds from that point, until whenever the problem was no longer reproducible. If this is still reproducible with those older builds, then we need to add code that detects and fixes that broken state.

Comment 24

[Dietrich Ayala (:dietrich)]

Resolving INVALID. Please re-open if you have valid steps, and if it occurs on older builds.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dietrich, I strongly believe that this bug has been fixed by Marcos patch on bug 477739. The checkin happened on the same day.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Henrik, are you saying that you can't reproduce (as per Dietrich's comment 23)? If so, can we please close this as FIXED with the current whiteboard marking indicating that the fix came from bug 477739.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It was already set. But I forgot the fixed/verified1.9.1 keyword. Tested again on 1.9.1 this morning and everything if fine. A missing bookmarks root folder gets restored via the maintenance after under an hour.

Comment 28

[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