Last Comment Bug 675416 - Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache
: Fetching bookmarks information during onBeforeItemRemove may break the bookma...
Status: VERIFIED FIXED
[qa!]
: regression, verified-aurora, verified-beta
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: 6 Branch
: All All
: -- normal (vote)
: Firefox 9
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on:
Blocks: 656188
  Show dependency treegraph
 
Reported: 2011-07-30 02:04 PDT by Usman Latif
Modified: 2011-12-12 05:39 PST (History)
8 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed
+
fixed


Attachments
test (2.10 KB, patch)
2011-08-02 06:00 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
test v1.1 (2.08 KB, patch)
2011-08-02 06:03 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
Places Bookmarks Database Backup (4.64 KB, text/plain)
2011-08-02 17:00 PDT, Usman Latif
no flags Details
Test with an observer that fails on my computer (2.07 KB, text/plain)
2011-08-03 12:30 PDT, Usman Latif
no flags Details
patch v1.0 (24.38 KB, patch)
2011-08-03 15:41 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
beta patch v1.0 (4.32 KB, patch)
2011-08-03 15:54 PDT, Marco Bonardo [::mak]
dietrich: review+
christian: approval‑mozilla‑beta+
Details | Diff | Review
patch v1.1 (29.35 KB, patch)
2011-08-05 08:45 PDT, Marco Bonardo [::mak]
dietrich: review+
blizzard: approval‑mozilla‑aurora+
Details | Diff | Review
Test with an observer that fails on my computer under firefox 8 (2.44 KB, text/plain)
2011-12-11 09:28 PST, Usman Latif
no flags Details

Description Usman Latif 2011-07-30 02:04:26 PDT
User Agent: Mozilla/5.0 (Windows NT 5.0; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110721152715

Steps to reproduce:

Using nsINavBookmarkService I did the following:

1. Created a folder titled "Bookmarks" under the bookmarks menu
2. Added a bookmark titled "Wired Science" to the folder created in step 1
3. Removed the folder created in step 1
4. Created a new folder titled "Science" under the bookmarks menu
5. Created a new folder titled "Blogs" under the folder created in step 4

The following code snippet duplicates the steps above:

var bs  = Components.classes["@mozilla.org/browser/nav-bookmarks-service;1"].getService( Components.interfaces.nsINavBookmarksService );
var ios = Components.classes["@mozilla.org/network/io-service;1"].getService( Components.interfaces.nsIIOService );
    
var folder = bs.createFolder( bs.bookmarksMenuFolder, "Bookmarks", bs.DEFAULT_INDEX );
bs.insertBookmark( folder, ios.newURI( "http:/www.wired.com/wiredscience", null, null), bs.DEFAULT_INDEX , "Wired Science" );

bs.removeItem( folder );

var folder1 = bs.createFolder( bs.bookmarksMenuFolder, "Science", bs.DEFAULT_INDEX );
var folder2 = bs.createFolder( folder1, "Blogs", bs.DEFAULT_INDEX );

// folder2 title should be "Blogs" but is "Wired Science" instead
if (bs.getItemTitle( folder2 ) != "Blogs") alert( "Folder name incorrect" );



Actual results:

The folder created in step 5 gets the title "Wired Science"


Expected results:

The folder should have received the title "Blogs" that was specified in the call to createFolder. In fact, this is what happens under Firefox 5 but not under Firefox 6 betas.
Comment 1 Marco Bonardo [::mak] 2011-08-01 05:06:55 PDT
taking to try reproducing it
Comment 2 Marco Bonardo [::mak] 2011-08-02 06:00:18 PDT
Created attachment 550057 [details] [diff] [review]
test

I've converted your code snippet into an xpcshell test, I can say it passes correctly on Nightly, I'm going to test it on Aurora and Beta
Comment 3 Marco Bonardo [::mak] 2011-08-02 06:03:22 PDT
Created attachment 550059 [details] [diff] [review]
test v1.1

ehr, small correction, the third folder is created inside the second one.
Is this test correct according to your report?
Comment 4 Marco Bonardo [::mak] 2011-08-02 07:18:46 PDT
the test passes on beta too... I cannot reproduce the problem.
Comment 5 Usman Latif 2011-08-02 17:00:42 PDT
Created attachment 550256 [details]
Places Bookmarks Database Backup

The test works for me under Firefox 5 but not under Firefox 6 betas. I am attaching a backup of the bookmarks database that I am running the test code under.
Comment 6 Usman Latif 2011-08-03 04:30:59 PDT
Actually, in my environment I am making some additional changes via an observer that watches bookmark changes. When that observer is disabled the test works. I will try to figure out precisely what changes the observer is making and incorporate that into the test.
Comment 7 Marco Bonardo [::mak] 2011-08-03 05:08:49 PDT
the observer may explain that, if it's doing changes to the db while being notified.
Comment 8 Usman Latif 2011-08-03 12:30:21 PDT
Created attachment 550466 [details]
Test with an observer that fails on my computer

I have changed the test to add an observer. The observer does not make any changes to the bookmarks database. However, it causes the test to fail under Firefox 6 betas.
Comment 9 Marco Bonardo [::mak] 2011-08-03 12:47:22 PDT
Thank you!

So here is my theory:
- we load the bookmarks info and invalidate cache
- we notify that we are about to remove the bookmark
- in the observer you ask for its info and this reloads the bookmark in the cache
- at this point we remove the bookmark, but the cache keeps its info
- when you create a new bookmark SQLite reuses the id of the bookmark, that now has a zombie entry in the cache
- when fetching info you are actually fetching from the zombie cache

This is pretty bad indeed :(
Comment 10 Marco Bonardo [::mak] 2011-08-03 12:54:27 PDT
notice the folder is created with the correct title, but fetching its information will give you the wrong one. This means we may return any broken information about this bookmark to other important consumers like Sync, if any add-on does something like this.
Comment 11 Marco Bonardo [::mak] 2011-08-03 15:41:25 PDT
Created attachment 550536 [details] [diff] [review]
patch v1.0

So, this is the patch, but I feel like it's not nice for a beta we are about to release, so I suggest in the beta we rather just disable the cache (patch coming)
Comment 12 Marco Bonardo [::mak] 2011-08-03 15:54:26 PDT
Created attachment 550539 [details] [diff] [review]
beta patch v1.0

this just disables the cache with an #if 0, I included the test for added safety.
Comment 13 christian 2011-08-03 16:05:40 PDT
Comment on attachment 550539 [details] [diff] [review]
beta patch v1.0

Approved for releases/mozilla-beta
Comment 14 Marco Bonardo [::mak] 2011-08-03 16:10:28 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/6eafc1d64af9

I'll do final checks on the full patch for FF7+ and then go to review with that one too, probably not today though.
Comment 15 Marco Bonardo [::mak] 2011-08-05 08:45:15 PDT
Created attachment 551046 [details] [diff] [review]
patch v1.1

There's nothing like an aborting assert to make me feel better.

So this defines a couple macros that wrap points where a bookmark information changes (provided is an information we cache). The first macro removes the entry from the cache, the second one fires an aborting assert if the cache has an entry, since that means something has repopulated it during the critical section.
The sooner we may land this (central and aurora), the better I'd feel.
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2011-08-16 10:38:23 PDT
I transplanted the backout to beta:

ehsanakhgari:~/moz/beta [01:37:40]$ hg tip
changeset:   73028:587a2159cf5f
tag:         tip
user:        Marco Bonardo <mbonardo@mozilla.com>
date:        Thu Aug 04 01:06:35 2011 +0200
summary:     Bug 675416 - Disable recent bookmarks cache in beta, since it's bogus.
Comment 17 Dietrich Ayala (:dietrich) 2011-08-20 13:16:57 PDT
Comment on attachment 551046 [details] [diff] [review]
patch v1.1

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

hm, i don't like the begin/end macro approach from a maintenance perspective - but not sure what cleaner way to do it without major refactoring. regardless, better to get this fixed sooner than later, r=me.
Comment 18 Marco Bonardo [::mak] 2011-08-22 05:36:09 PDT
> hm, i don't like the begin/end macro approach from a maintenance perspective
> - but not sure what cleaner way to do it without major refactoring.
> regardless, better to get this fixed sooner than later, r=me.

I'm not sure how to handle it more efficiently even with a refactoring, unless we add an abstraction layer in the middle, that may be unwanted for perf and maintenance reasons. Btw I don't see refactorings in the near future, at least till bookmarks become async.
Pushing, then will ask aurora approval, all betas have cache disabled instead.
Comment 20 Mounir Lamouri (:mounir) 2011-08-22 10:56:55 PDT
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/1933e939180e
Comment 21 Marco Bonardo [::mak] 2011-08-22 12:59:32 PDT
Comment on attachment 551046 [details] [diff] [review]
patch v1.1

Asking for Aurora approval, this is a pretty important fix for bookmarks coherence, beta is unaffected since Ehsan transplanted cache disabling from FF6 to FF7.
Comment 22 Johnathan Nightingale [:johnath] 2011-08-23 14:40:27 PDT
Discussed in triage, but we're not sure why we're being asked to take the fix, rather than the backout we took on 6 and 7. Can you comment more on the relative risks and rewards?
Comment 23 Marco Bonardo [::mak] 2011-08-23 15:11:17 PDT
I asked for the patch on 8, since the original patch implementing the cache has baked for a long time on 6, 7 and 8 (and now on 9), showing only this issue in onBeforeItemRemoval notification, that, for bad that it is, brought to a safer fix.  This patch is not conceptually different from the original, apart restricting the code parts where things can't be cached, that by itself is safer than it was.

I you want to be extremely careful I won't oppose to taking the backout on 8, my primary target is to fix this bug everywhere asap. , The reward is mostly IO removal from main-thread, that helps Sync, Mobile and a bunch of addons, I guess we may delay perf improvements without too much regret?
Comment 24 Christopher Blizzard (:blizzard) 2011-08-25 14:15:34 PDT
Comment on attachment 551046 [details] [diff] [review]
patch v1.1

Approved for Aurora (Update 8.)  Please land as soon as possible.  Thanks!
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 16:39:44 PDT
qa+ for verification in Firefox 7 and 8
Comment 27 Maniac Vlad Florin (:vladmaniac) 2011-09-26 12:57:04 PDT
Verified on 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a1) Gecko/20110925 Firefox/9.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0a2) Gecko/20110925 Firefox/8.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110925 Firefox/9.0a1
Mozilla/5.0 (Windows NT 6.1; rv:8.0a2) Gecko/20110925 Firefox/8.0a2
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0

Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110926 Firefox/9.0a1
Mozilla/5.0 (X11; Linux i686; rv:8.0a2) Gecko/20110926 Firefox/8.0a2
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0

Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110926 Firefox/9.0a1
Mozilla/5.0 (Windows NT 5.1; rv:8.0a2) Gecko/20110926 Firefox/8.0a2
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Comment 28 Usman Latif 2011-12-11 09:24:53 PST
The bug is not fully fixed. I have managed to create a test case under which the bookmarks cache returns the info of a deleted folder.
Comment 29 Usman Latif 2011-12-11 09:28:58 PST
Created attachment 580761 [details]
Test with an observer that fails on my computer under firefox 8

This test indicates that the bug is not fully fixed. The test removes a folder and creates additional bookmarks, one of which reuses the id of the removed folder. Afterwards, the bookmark service returns the info of the deleted folder when given the id of the bookmark.
Comment 30 Marco Bonardo [::mak] 2011-12-12 05:39:58 PST
new bugs should be filed apart, this bug fixes a reported case with a testcase, that is fixed and tested, you case would be different.

That said, your test looks strange, where is the part that creates the Mozilla Firefox folder? How did you run it?
Please file a bug apart and specify there better how you ran the test, since as a simple xpcshell-test it wouldn't work.

Note You need to log in before you can comment on or make changes to this bug.