Closed Bug 675416 Opened 12 years ago Closed 12 years ago
Fetching bookmarks information during on
Before Item Remove may break the bookmarks cache
4.32 KB, patch
|Details | Diff | Splinter Review|
29.35 KB, patch
|Details | Diff | Splinter Review|
2.44 KB, text/plain
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
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
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.
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.
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.
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 :(
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.
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)
this just disables the cache with an #if 0, I included the test for added safety.
Attachment #550539 - Flags: review?(dietrich)
Attachment #550539 - Flags: review?(dietrich) → review+
Attachment #550539 - Flags: approval-mozilla-beta?
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.
OS: Windows 2000 → All
Hardware: x86 → All
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.
I transplanted the backout to beta: ehsanakhgari:~/moz/beta [01:37:40]$ hg tip changeset: 73028:587a2159cf5f tag: tip user: Marco Bonardo <email@example.com> date: Thu Aug 04 01:06:35 2011 +0200 summary: Bug 675416 - Disable recent bookmarks cache in beta, since it's bogus.
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.
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/1933e939180e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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+
qa+ for verification in Firefox 7 and 8
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
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.
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.