Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache

VERIFIED FIXED in Firefox 6

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Usman Latif, Assigned: mak)

Tracking

({regression, verified-aurora, verified-beta})

6 Branch
Firefox 9
regression, verified-aurora, verified-beta
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5 unaffected, firefox6+ fixed, firefox7+ fixed, firefox8+ fixed)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
taking to try reproducing it
Assignee: nobody → mak77
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
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?
Attachment #550057 - Attachment is obsolete: true
the test passes on beta too... I cannot reproduce the problem.
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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.
the observer may explain that, if it's doing changes to the db while being notified.
(Reporter)

Comment 8

6 years ago
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.
Attachment #550466 - Attachment mime type: application/octet-stream → text/plain
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 :(
Status: UNCONFIRMED → ASSIGNED
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
Ever confirmed: true
Blocks: 656188
Keywords: regression
Summary: nsINavBookmarkService.createFolder creates folder with incorrect title → Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache
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.
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)
Attachment #550059 - Attachment is obsolete: true
Attachment #550256 - Attachment is obsolete: true
Attachment #550466 - Attachment is obsolete: true
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.
Attachment #550539 - Flags: review?(dietrich)

Updated

6 years ago
status-firefox5: --- → unaffected
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox6: ? → +
tracking-firefox7: ? → +
Attachment #550539 - Flags: review?(dietrich) → review+
Attachment #550539 - Flags: approval-mozilla-beta?

Comment 13

6 years ago
Comment on attachment 550539 [details] [diff] [review]
beta patch v1.0

Approved for releases/mozilla-beta
Attachment #550539 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
status-firefox6: affected → fixed
Flags: in-testsuite+
OS: Windows 2000 → All
Hardware: x86 → All
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.
Attachment #550536 - Attachment is obsolete: true
Attachment #551046 - Flags: review?(dietrich)
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.
status-firefox7: affected → unaffected
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.
Attachment #551046 - Flags: review?(dietrich) → 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.

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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/1933e939180e
Whiteboard: [inbound]
status-firefox8: --- → affected
tracking-firefox8: --- → ?
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/1933e939180e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
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.
Attachment #551046 - Flags: approval-mozilla-aurora?
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?
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 on attachment 551046 [details] [diff] [review]
patch v1.1

Approved for Aurora (Update 8.)  Please land as soon as possible.  Thanks!
Attachment #551046 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/2385df82b3ed
status-firefox7: unaffected → fixed
status-firefox8: affected → fixed

Updated

6 years ago
tracking-firefox8: ? → +
qa+ for verification in Firefox 7 and 8
Whiteboard: [qa+]
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
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [qa+] → [qa!]
(Reporter)

Comment 28

6 years ago
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.
(Reporter)

Comment 29

6 years ago
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.
Attachment #580761 - Attachment mime type: application/octet-stream → text/plain
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.
You need to log in before you can comment on or make changes to this bug.