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.
(Assignee)

Comment 1

6 years ago
taking to try reproducing it
Assignee: nobody → mak77
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

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

Comment 7

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

Updated

6 years ago
Attachment #550466 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 9

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

Updated

6 years ago
Blocks: 656188
Keywords: regression
(Assignee)

Updated

6 years ago
Summary: nsINavBookmarkService.createFolder creates folder with incorrect title → Fetching bookmarks information during onBeforeItemRemove may break the bookmarks cache
(Assignee)

Comment 10

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

Comment 11

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

Comment 12

6 years ago
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+
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 14

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

Updated

6 years ago
Flags: in-testsuite+
OS: Windows 2000 → All
Hardware: x86 → All
(Assignee)

Comment 15

6 years ago
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+
(Assignee)

Comment 18

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

Comment 19

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/1933e939180e
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
status-firefox8: --- → affected
(Assignee)

Updated

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

Comment 21

6 years ago
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?
(Assignee)

Comment 23

6 years ago
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+
(Assignee)

Comment 25

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

Updated

6 years ago
Attachment #580761 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 30

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