275250 - Undo + Sort ends up delete bookmark [@ RDFContainerImpl::RemoveElementAt]

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

[O. Atsushi (Torisugari)]

Combination of undo and sort deletes bookmarks accidentally.

Steps to reproduce.
1. Open bookmarks manager.
1. Create a folder which has 2 bookmarks.
2. Sort the folder.
3. Reorder these bookmarks with drag and drop.
4. Sort the folder
5. Undo

Actual Result:
The folder has only one bookmark. The other is gone.
Expected result:
Nothing happens.

I'm not sure which should be fixed: transaction or sort function.
Seamonkey's sort calls transactionmanager.clear(), btw.

Comment 1

[Ravenous Bugblatter Beast]

I can confirm this on firefox build:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041219
Firefox/1.0+

Furthermore, I called my two bookmarks 1 & 2. At the end of the steps to
reproduce above, 1 got removed as described. I then re-created 1 and sorted the
folder again and I ended up with three bookmarks - 1 followed by two copies of 2.

Comment 2

[Kevin Brosnan]

confirming - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5)
Gecko/20041107 Firefox/1.0

I can reproduce with the stated steps in comment 0.

Comment 3

[O. Atsushi (Torisugari)]

When testing the step to reproduce again,
the browser crashed.
Talkback ID is tk2815010

Comment 4

[timeless]

Incident ID: 2815010
Stack Signature	RDFContainerImpl::RemoveElementAt 4169f7f0
Product ID	Firefox10
Build ID	2004110711
Trigger Time	2004-12-29 16:10:34.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (000b0cd4)
URL visited	
User Comments	Undo + Sort crash
Since Last Crash	354527 sec
Total Uptime	3255185 sec
Trigger Reason	Access violation
Source File, Line No.
d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/rdf/base/src/nsRDFContainer.cpp,
line 387
Stack Trace 	
RDFContainerImpl::RemoveElementAt 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/rdf/base/src/nsRDFContainer.cpp,
line 387]
XPTC_InvokeByIndex 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
XPCWrappedNative::CallMethod 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,
line 2034]
XPC_WN_CallMethod 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1287]
js_Invoke 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 941]
js_Interpret 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 2978]
js_Invoke 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c,
line 958]
nsXPCWrappedJSClass::CallMethod 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,
line 1339]
nsXPCWrappedJS::CallMethod 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp,
line 450]
SharedStub 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 147]
nsTransactionItem::UndoTransaction 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/editor/txmgr/src/nsTransactionItem.cpp,
line 198]
nsTransactionManager::`vftable'
nsTransactionManager::AddRef 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/editor/txmgr/src/nsTransactionManager.cpp,
line 105]
0x8b560c45

Comment 5

[Mike Connor [:mconnor]]

Note that Sort by Name can't be undone, so the last transaction was creating the
bookmark.  This is sort of INVALID, since its undoing the last non-sorting
action.  The crash is bug 268996.

Not sure why you think moving the transactions to a JS service would help here.

Comment 6

[Jay Patel [:jay]]

It does look like an edge case that might not be high priority. As mconner said,
the behavior itself is as expected (undoing after creating 2 bookmarks will
remove the last bookmark added).  I saw the correct behavior with Firefox 1.0.1
but could not reproduce the crash.

The crash is another matter and if someone can put together a patch, we can have
a look at a fix.

Comment 7

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

Comment 8

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20060915 BonEcho/2.0

I'm still seeing the dataloss bug where the bookmark gets deleted, but I'm not crashing.

Comment 9

[O. Atsushi (Torisugari)]

(In reply to comment #5)
> Note that Sort by Name can't be undone,

That is exactly the reason of this dataloss. So what we can do is either

(1.) Make sorting undoable (Now that bug 168411 is fixed, it's easy).
(2.) Clear transaction on every sort, as Seamonkey does.

This is a very simple bug. Incorrect situation for undoing DnD,
totally due to sorting.

(2.) is not a real fix but a workaround, however, this workaround 
is extreamely handy, just adding only one line at the end of JS
sorting function. In that case, we lose the transaction history. So
it's not a very good solution, but "losing tx history" seems less
harmful than "losing a bookmark".

Comment 10

[Shawn Wilsher :sdwilsh]

The bookmark backend has been completely replaced as of Firefox 3.  Firefox 2 is no longer supported, so this bug is now INVALID.