68547 - D&D of bookmarks to the top of their folder sometimes deletes them

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[Fabian Guisset]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win95; en-US; 0.8) Gecko/20010210
BuildID:    2001021020

This is BAD dataloss. In the bookmarks manager, dragging and dropping bookmarks
to the top of their own folder sometimes deletes them. I have not yet found a
way to reproduce this 100% but it's happened pretty often. Boris Zbarsky also
could reproduce it on linux.


Reproducible: Always
Steps to Reproduce:
1) Launch Mozilla
2) Bookmarks|Bookmarks Manager
3) Expand Mozilla Project folder
4) Expand Developer Information subfolder
5) Drag the items in that subfolder to the top of the "Developer Information"
folder (their own folder). 
If you do this a number of times (sometimes it takes another folder to
reproduce), the folder or bookmarks disappears.

Actual Results:  The bookmark/folder is deleted

Expected Results:  successfully moved to the top of the folder

Happens on linux and win at least, suspect xp

Comment 1

[Boris Zbarsky [:bzbarsky]]

*** Bug 69797 has been marked as a duplicate of this bug. ***

Comment 2

[Matthew Wilson]

The problem seems to occur when we go through the code marked

// XXX magical fix for bug # 33546: handle dropping after open container

in bookmarksDD.js.

Comment 3

[Matthew Wilson]

JavaScript error:
uncaught exception ... NS_ERROR_ILLEGAL_VALUE ...
nsIRDFContainer.InsertElementAt ... location ... bookmarksDD.js :: anonymous ::
line 256 data:no

Lines 255/256 of bookmarksDD.js are
 var dropIx = RDFC.IndexOf(rTarget);
 RDFC.InsertElementAt(rSource, dropAction == "after" ? ++dropIx : dropIx, true);

When the error occurs dropIx is -1, i.e. rTarget is not in the RDFC container.

If I add a line to set dropIx to 0 if it is -1, then the bookmark moves to the
first item of the parent folder.

Hope that helps.

Comment 4

[Matthew Wilson]

Attachment: Proposed patch

Comment 5

[Matthew Wilson]

The above patch fixes the dataloss problem, I think. I'm now seeing a problem
where the bookmark disappears in the Bookmarks Manager window, but is shown as
moving correctly in the Sidebar. If I close and reopen Bookmarks Manager, it's
OK again. Tree display problem?

Comment 6

[Gervase Markham [:gerv]]

There's a band-aid patch here to fix some serious dataloss, but the underlying 
problem needs looking into.

Gerv

Comment 7

[Matthew Wilson]

The problem of the tree not refreshing seems not to occur on WinNT.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

What is the effect of moving the rContainer line? I'm curious because the patch 
you attached is not what you describe in your previous comment. 

Comment 9

[Matthew Wilson]

I think this is what happens:
In the original code, we set ContainerItem, then set rContainer based on the
value of ContainerItem, and then possibly change ContainerItem (in the bug 33546
fix section of the code). In this case rContainer does not really point to the
container we are trying to put the item in, so the IndexOf fails, and then the
paste fails.

Comment 10

[Paul Chen]

Marking nsbeta1+, p1, mozilla0.9

Comment 11

[Kevin Buhr]

Richard:  I'm CCing you on this because of your patch for bug 33546.  Please
verify that Matthew's patch to your bugfix makes it do what you intended all along.

...

Matthew's patch (attachment 02 [details] [diff] [review]/22/01 14:51) is correct.  It is not a band-aid
patch.  Richard's fix for bug 33546 was meant to handle the case where the drop
target is the bottom quarter or so of a folder.  In this case, the drop action
variables are calculated as:

   dropAction = "after"
   dropItem = target folder
   containerItem = parent of container folder

Normally, this would insert the dropped item after the folder at the same level
as the folder.  The "magical fix" changes this to:

   dropAction = "before"
   dropItem = first item of target folder
   containerItem = target folder

if the target folder is open, so the dragged item gets stuck at the beginning of
the target folder, as expected.

Now, "dropItem" and "containerItem" become "rTarget" and "rContainer" like so:

    var rContainer = this.getResource(NODE_ID(containerItem));
    var rTarget = this.getResource(NODE_ID(dropItem));

HOWEVER, there's a bug (either in the original fix to introduced later). 
"rContainer" is assigned before the magical fix changes the value of
"containerItem" whose value is not used again.  Therefore, the algorithm
proceeds with the corrected values of "dropItem" and "rTarget" and the wrong
value of "rContainer".  Chaos ensues!  (That is, the dropped bookmark is lost
forever.)

This patch is obviously more correct than what's there.  Is it too late to
keyword this with mozilla0.8.1?

Note that the redraw problem (where items disappear but return when you close
and reopen the bookmark manager) is a separate issue.  It happens in other
contexts, too, and shouldn't stop us from getting this fix in.

Comment 12

[Fabian Guisset]

r=fabian tested on windows95 on multiple mozilla versions, makes sense, and
comments by kevin were very useful :-)

Comment 13

[Kai Stukenbrock]

This bug is still there in 2001041104 (win98). In sidebar D&D of a subfolder
from one folder to another resulted in losing the whole subfolder.

Comment 14

[Ben Goodger (use ben at mozilla dot org for email)]

nerp

Comment 15

[Claudius Gayle]

aside from the aforementioned separate redraw issues this works. VERIFIED Fixed with 2001042608
builds on all platforms.

Comment 16

[xyzzy]

Reposting my comments from bug 56349, as it is more appropriate to this bug. 
I'd hate for one to slip through the cracks...

This might be caused by the redraw issues, except I don't have to close Bookmark
Manager to fix it.  Can I get a bug number for that bug?  I either need to mark
this as depending on it, or re-open this one.

Win98 build 2001080603:

1)Open Bookmark Manager
2)Create a Folder
3)Create or Move 3 bookmarks into Folder
4)Drag and drop second bookmark onto gap between Folder and first bookmark
(after it highlights)

Actual Results: Bookmark disappears until Folder is closed, then reopened.

Expected Results: Bookmark moves to first position within Folder, no refresh needed.