430442 - Undo and redo in the library are sometimes not correctly working

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Robert [minotaur 11]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.8.1.14) Gecko/20080418 Ubuntu/7.10 (gutsy) Firefox/2.0.0.14
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008042303 Minefield/3.0pre

Undo/ redo does not always work as expected, sometimes undo/ redo only partly reverts a change. The undo/ redo queue for bookmarks changes (status of the menu entries) is also sometimes incorrect, for example redo is not available after selecting undo, or it's possible to undo the same change several times. This can lead to a loss of bookmarks.

I can reliable reproduce this bug if i delete some bookmarks and undo/ redo these changes several times, but the step are not always identical. The test file used in the STR is for easy reproducibility.

Reproducible: Always

Steps to Reproduce:
1.Replace the places.sqlite file in the profile with the attached test file
2.Start firefox and open the library
3.Select all entries in the "Bookmarks Menu" and delete them
4.Select undo

Actual Results:  
Some bookmarks are missing, redo is not available and undo adds the same bookmarks again. The order of the restored bookmarks is changed (Bug 407424).


The STR above only work if the places.sqlite file is put in an already existing profile, or in an empty profile together with a prefs.js or user.js file with the line:
user_pref("browser.places.smartBookmarksVersion", 1);
Otherwise it's necessary to select redo after step 4 to reproduce the bug. In this case redo deletes only some bookmarks and undo is not available afterwards.

Comment 1

[Robert [minotaur 11]]

Attachment: places.sqlite file for testing

Comment 2

[Robert [minotaur 11]]

Since the check-in from bug Bug 404658 it's always necessary to use the fifth step (redo) to see the bug with the test file. I've also tried it again in a new profile with my normal bookmarks imported and it's still possible to loose bookmarks through undo/ redo.

Comment 3

[Robert [minotaur 11]]

Attachment: places.sqlite test file v2

This test file shows the bug again with the STR from comment 0, the folder "test" is missing after undo.

Comment 4

[Robert [minotaur 11]]

The second test file now also needs the fifth step to see the bug. So the change in the STR is probably not related to bug 404658, but to the time since creation.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get some solid STR or QA before making a blocking decision? This is concerning, but I would have expected more dupes.

Robert: can you reproduce this with a fresh profile? is it related to the size of the file / number of bookmarks?

Comment 6

[Robert [minotaur 11]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008043015 Minefield/3.0pre

I could reproduce the bug multiple times with a new profile, it doesn't seem to be related to the size of the places.sqlite file or the number of bookmarks.

The following steps reproduce the bug for me on Ubuntu 7.10 and Windows XP:

1.Create a new profile and start firefox (the smart bookmarks should be created before step 2, this requires a restart with a non relative profile location)
2.Visit a site and create one bookmark in a new folder in the "Bookmarks Menu"
3.Visit a second site and create one bookmark in the "Bookmarks Menu"
4.Open the library, select all entries in the "Bookmarks Menu" and delete them
5.Select undo
6.Select redo

Comment 7

[Luke Iliffe (Harlequin99)]

I can confirm on XP with the STR in Comment 6. Once I delete everything in the bookmarks menu and then try undo, the 'New Folder' I created containing  1 bookmark is not restored.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051011 Minefield/3.0pre ID:2008051011

Comment 8

[Robert [minotaur 11]]

I've tried to find a regression range, but i could reproduce the bug even in a build from 2007-06-01, which already used the places.sqlite file for bookmarks.

Comment 9

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

As in Comment #7, I confirm the bug. I noticed that if I delete all the bookmarks and the subfolders of a folder that has no separator, all works fine. I think the problem is related to separators.

Comment 10

[Robert [minotaur 11]]

I see the bug also after deleting some bookmarks without separators and selecting undo.

Since two people confirmed the bug in the meantime, i will nominate it for blocking‑firefox3+ again. A unreliable undo function is almost useless.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i confirm this, i'm going to take a look

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

i still want to do some testing but this should make the situation really better.
I've found some problem here and there about the order in which transactions are created and restored, internal indexes, some missing implementation.

I'll ask for a review after manual tests (i already updated the txn unit test to include some bookmark management redo and an aggregate transaction test).

Next step will be to revise the transaction manager to receive an nsIVariant instead of a simple itemId, so we will be able to pass the already known info avoiding a lot of sql traffic (but this will most probably be in bug 432706)

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the patch appear working fine and solving reported problems, still in some case i've seen some problem with command updating after a lot of undo redo work, mainly sometimes undo remains always active (but i've seen that sometimes also without the patch). I don't have valid steps to investigate on though, so i think we can take this and fix minor issues later when reported with good steps to reproduce.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 324806 [details] [diff] [review]
patch


>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.235
>diff -u -8 -p -r1.235 controller.js
>--- browser/components/places/content/controller.js	2 May 2008 15:39:45 -0000	1.235
>+++ browser/components/places/content/controller.js	12 Jun 2008 15:12:38 -0000
>@@ -859,21 +859,19 @@ PlacesController.prototype = {
>   /**
>    * Creates a set of transactions for the removal of a range of items. 
>    * A range is an array of adjacent nodes in a view.
>    * @param   [in] range
>    *          An array of nodes to remove. Should all be adjacent. 
>    * @param   [out] transactions
>    *          An array of transactions.
>    */
>-  _removeRange: function PC__removeRange(range, transactions) {
>+  _removeRange: function PC__removeRange(range, transactions, removedFolders) {

add documentation for the parameter change

>@@ -901,20 +899,21 @@ PlacesController.prototype = {
>   /**
>    * Removes the set of selected ranges from bookmarks.
>    * @param   txnName
>    *          See |remove|.
>    */
>   _removeRowsFromBookmarks: function PC__removeRowsFromBookmarks(txnName) {
>     var ranges = this._view.getRemovableSelectionRanges();
>     var transactions = [];
>-    // Delete the selected rows. Do this by walking the selection backward, so
>-    // that when undo is performed they are re-inserted in the correct order.
>-    for (var i = ranges.length - 1; i >= 0 ; --i)
>-      this._removeRange(ranges[i], transactions);
>+    var removedFolders = [];
>+
>+    for (var i = 0; i < ranges.length; i++)
>+      this._removeRange(ranges[i], transactions, removedFolders);
>+

wait, is this parameter change not actually used yet? if not, please remove these changes, and add them to the bug where they're used.

r=me with these changes

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

needs patch for bug 452777 to work properly since now we are notifying the view with wrong itemId due to a regression

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

(In reply to comment #14)
> (From update of attachment 324806 [details] [diff] [review])

> >-  _removeRange: function PC__removeRange(range, transactions) {
> >+  _removeRange: function PC__removeRange(range, transactions, removedFolders) {
> 
> add documentation for the parameter change
> 
> wait, is this parameter change not actually used yet? if not, please remove
> these changes, and add them to the bug where they're used.
> 

the param is used to keep track of previously deleted folders, since i'm walking ranges forward.
Hwv i've made that optional now, so you can still call removeRange omitting it, if needed.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/9e6e421c6782

Comment 18

[Tracy Walker [:tracy]]

reporter, can you verify this is fixed?

It still seems broken to me with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1) Gecko/20081007 Firefox/3.1b1

also, repeated undo's keep adding more entries to the Bookmarks Menu. all entries deleted except the test folder, which is lost.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #18)
> 
> also, repeated undo's keep adding more entries to the Bookmarks Menu. 

see comment 13, probably we should file a new bug on the undo remains active issue if that does not exist.

Apart that, what steps are reproducing the dataloss issue for you (since in this reports there are many different STRs)?

Comment 20

[Tracy Walker [:tracy]]

I was using steps in comment #6, the data loss occurred at step 5. (undoing the delete didn't return the test folder).  However, I was testing with a profile created in 3.1a1.   I am not able to reproduce with a fresh profile created in b1.  And after further testing with an alpaha1 created profile I am not able to reliably reproduce this in b1.  undo/redo gets funky if you start doing multiple mixed operations. That is, it's hard to tell just what is supposed to happen, if you're not closely paying attention to the position in the operation stack.

VERIFIED

Comment 21

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv