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
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?
the test passes on beta too... I cannot reproduce the problem.
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.
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.
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.
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 :(
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)
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 on attachment 550539 [details] [diff] [review] beta patch v1.0 Approved for releases/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.
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.
I transplanted the backout to beta: ehsanakhgari:~/moz/beta [01:37:40]$ hg tip changeset: 73028:587a2159cf5f tag: tip user: Marco Bonardo <firstname.lastname@example.org> 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.
> 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
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.
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!
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.
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.
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.