Closed Bug 53403 Opened 22 years ago Closed 21 years ago

Dragging folder into child folder deletes folder

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.1

People

(Reporter: devsin, Assigned: morse)

References

Details

(Keywords: dataloss, Whiteboard: waiting for 42080 to verify)

Attachments

(6 files)

Mozilla M18 2000092008, Mac OS 9.0.4 -

1) Open Manage Bookmarks window
2) Select a folder
3) Drag the folder onto itself (like you were going to move the folder inside
another folder)

The entire folder is deleted. Hopefully you have a backup of your Bookmarks file.
Holy data loss batman! Such a small gesture, such a completly gone, nonrecoverable
bookmarks file!

don who gets this???
Assignee: slamm → don
Severity: normal → major
Keywords: nsbeta3, rtm
OS: Mac System 9.0 → All
Hardware: Macintosh → All
That's right Robin!  It looks like the Joker is up to something evil again!

Please tell me we can fix this in RTM?!?  Claudius, would you hold beta 3 for this?
Assignee: don → ben
Priority: P3 → P1
Whiteboard: [nsbeta3-][rtm+]
Based on the patch being so simple and safe, and the bug being so devastating, I 
would recommend getting this into beta3.  But I'll leave that for ben and don to 
argue for.
Shouldn't that return be return false; instead?  That's what the final return in 
the function does.  I can't tell, due to the lamely screwy indentation and tabs 
in the file, where the for loop ends, at least when reading the patch via 
bugzilla.  But if you just want to exit the loop, and run whatever's after the 
loop body, then break; rather than return false; might be even better.

/be
Yes, return false is the more correct thing to do.  But this routine never 
returns anything but false so apparently the return value is never used.

As far as using a break instead of a return, that's even better.  The code after 
the loop does some processing if "dirty" is true.  And "dirty" will be true if 
the loop had been executed at least once.  So if several items were selected to 
be dragged and at least one of them was not being dragged onto itself, then 
"dirty" will get set and we would want to do the final processing.

Finally, there's another test just after which probably needs the same fix.  It 
tests for dropping a node onto its parent container.  I'm not sure what problem 
the current coding for that test is causing, but it is incorrect.

Attaching a revised patch.
Forget preceding patch -- it doesn't work.  Seems like the deletion occurs 
unless we actually do a return.  (I posted the patch before I tested it -- 
sorry.)

So now I'm attaching the third (and hopefully final) patch.
Plussed again for beta 3! :-)
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3+][rtm+]
This needs a double plus, right?
Keywords: dataloss
nav triage team:
we wanted this in beta3, but too late now.  nsbeta3-
Whiteboard: [nsbeta3+][rtm+] → [nsbeta3-][rtm+]
Keywords: relnote3
Vera: release note
PDT marking [rtm need info] since we'd like a fix for this after the patch has 
had code reviews.
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm need info]
Nav triage team: Serious data loss; remains P1.
Poking around looking at release note items, I found a similar bug that seems to 
have slipped through the cracks. See bug 47146.
*** Bug 47146 has been marked as a duplicate of this bug. ***
Can someone review and approve this, please!!!
Whiteboard: [nsbeta3-][rtm need info] → [nsbeta3-][rtm need info]Fix in hand
the latest fix is r=ben.
Status: NEW → ASSIGNED
Keywords: approval
PDT, this fix from Steve Morse has now been reviewed and approved by Ben (even
though he will check in Steve's patch).  Please mark this "++".
Whiteboard: [nsbeta3-][rtm need info]Fix in hand → [nsbeta3-][rtm+]Fix in hand, reviewed and approved
PDT marking [rtm need info] because the only reviewer is an sr. Need to have two
reviews for all patches.
Whiteboard: [nsbeta3-][rtm+]Fix in hand, reviewed and approved → [nsbeta3-][rtm need info]Fix in hand, reviewed and approved
I've got problems with this code.  The patch itself is pretty simple, but the
loop is big and hairy and living in an even bigger and hairier function.  I
understand how the patch stops the crash, but in the case of a drag of multiple
items it can leave the drag half-done.  On close inspection of the function with
help from Ben Goodger, it seems that there is another more promising site within
the function that may be causing the bug... I'll let him comment further.  In
the meantime, I don't think we can let this patch in as it stands.
Nice catch, working on new fix ...
Whiteboard: [nsbeta3-][rtm need info]Fix in hand, reviewed and approved → [nsbeta3-][rtm need info]Working on new fix
Drag of multiple bookmarks isn't possible.  If you select multiple bookmarks and 
try to drag and drop them, only one of the bookmarks actually gets dropped.  
(This is probably some other bug.)

So the patch presented here is perfectly safe and is the simplest, lowest-risk 
patch to fix the current bug.  Anything more involved (such as restructuring the 
code to prepare for the day when drag-and-drop of multiple bookmarks works, 
should be considered for the tip but not the branch.
I just opend bug 55678 on the inability to drag and drop items in the 
manage-bookmark dialog.
correction to above comment.  I meant to say "inability to drag and drop 
MULTIPLE items in the manage-bookmarks dialog."
See bug 42080 for problems with drag and drop of multiple bookmarks/folders.
Steve, the bug number you cite (the one you say you wrote) is incorrect. Regardless, that
bug, wherever it may be is a dupe of bug 42080 (can't drag mult. bm's)
Oops, I meant bug 55768, not 55678
Miunsing this.  Sorry.  Ben just doesn't think he can find the right fix for
this in time for N6 RTM.
Whiteboard: [nsbeta3-][rtm need info]Working on new fix → [nsbeta3-][rtm-]Working on new fix
Can we stop the crash on the branch with a hack, and then leave the bug open for 
the trunk, dependent on the multi drag/drop bug? Then the forward-looking trunk 
doesn't get polluted by the hack. What say, Scott?
isn't there at least a dirty hack to disallow a drop of this kind?
OK, given that drag of multiple items currently doesn't work (_and_ that a bug
has been filed on that) I can give this a cautious sr=scc for the branch _only_.
 I hope this helps.
In that case, moving back to rtm+ so PDT can approve.
Whiteboard: [nsbeta3-][rtm-]Working on new fix → [nsbeta3-][rtm+]Fix (hack) in hand for branch-only, reviewed/approved
rtm++ for this patch on branch only, as scc said.
Whiteboard: [nsbeta3-][rtm+]Fix (hack) in hand for branch-only, reviewed/approved → [nsbeta3-][rtm++]Fix (hack) in hand for branch-only, reviewed/approved
fix checked in to branch only. 

marking rtm-, leaving open to find real fix once drag of multiple items is 
repaired. 
Whiteboard: [nsbeta3-][rtm++]Fix (hack) in hand for branch-only, reviewed/approved → [nsbeta3-][rtm-] Need to find real fix
Updated status whiteboard to show that a hack was put into NS6 to make sure that
dragging a folder on top of itself did not delete the folder.

Personally, I think we should get rid of the rtm keywords and the status
whiteboard markings in these kinds of situations after we've made the rtm fix.
Whiteboard: [nsbeta3-][rtm-] Need to find real fix → [nsbeta3-][rtm-] Hacked fix checked into NS6, Need to find real fix
This no longer needs a relnote, right? It's fixed on the branch...

Just in case, here it is again. All sing:

It seems unclear to me whether this bug requires either of a "developer" or 
"user" release note for Netscape 6 RTM. If anyone feels it does, can they please 
draft one and then nominate with the relnote-user or relnote-devel strings in 
the Status Whiteboard.

Thanks :-)

Gerv
*** Bug 58400 has been marked as a duplicate of this bug. ***
This is an edge case for the release notes... but I'll keep it in. Marking
"relnote-user" in whiteboard.
Whiteboard: [nsbeta3-][rtm-] Hacked fix checked into NS6, Need to find real fix → [nsbeta3-][rtm-][relnote-user] Hacked fix checked into NS6, Need to find real fix
vera, there's nothing to release note.  There is no way that the user can run 
into this problem with the current branch build.
*** Bug 59175 has been marked as a duplicate of this bug. ***
*** Bug 60716 has been marked as a duplicate of this bug. ***
nav triage team:

Nominate for nsbeta1. Check patch onto trunk, then wait for multiple bm drags to 
fix real problem
Depends on: 42080
Keywords: nsbeta1
*** Bug 62850 has been marked as a duplicate of this bug. ***
*** Bug 64779 has been marked as a duplicate of this bug. ***
this bug is fixed correctly by my patches to 64723 and 64722
*** Bug 62172 has been marked as a duplicate of this bug. ***
Target Milestone: --- → mozilla0.8
Depends on: 64722, 64723
Fix checked in (this is ben, using hyatt's machine, oops)
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 71636 has been marked as a duplicate of this bug. ***
Reopening
It happens again in 2001031008 win98
and on win 2000 ( see bug 71636 )
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I didn't see this with dragging a folder onto itself, but I do see it dragging a 
folder onto a child folder. Accepting and retargeting at .9
Status: REOPENED → ASSIGNED
Summary: Dragging folder onto itself deletes folder → Dragging folder into child folder deletes folder
Target Milestone: mozilla0.8 → mozilla0.9
clearing status/keyword cruft.
Whiteboard: [nsbeta3-][rtm-][relnote-user] Hacked fix checked into NS6, Need to find real fix
Keywords: nsbeta1nsbeta1+
nav triage team:

Marking nsbeta1+
ack! this explains my problems in the personal toolbar. setting up dependancies.
Blocks: 46456
alas, this is ongoing. tossing into .9.1
Target Milestone: mozilla0.9 → mozilla0.9.1
This one has been around too long.  Attaching patch.
ben and alecf, please review and super-review.  Thanks.
Thanks for the patch, steve, I'll take a look at this tomorrow. 
If I understand this correctly, this only fires if the item dropped on is the 
container that we're going to insert the bookmarks into? (the case in which 
containerItem == dropItem, or rTarget == rContainer). 

I can see how this would prevent you from dropping items onto the toplevel 
container, but imagine this scenario:

Take a deeply nested folder structure: say three deep. Add several dummy 
bookmarks, or just use the default bookmarks file provided with new profiles, I 
think there are cases in that. Drag a toplevel folder into a subfolder nested 
several levels down. 

I see you doing what appears to be a climb up the document, which is probably 
what is required, but only in the case where the dropItem == containerItem. 

Am I missing something? 
Do you have a test case that demonstrates the problem you are now describing?  
Is so, then can we simply remove the "if (rTarget == rContainer)" part of the 
patch to cover that case?
ok, I'll sr=alecf that for moz 0.9 but not for the trunk - we need to fix the
real problem and it's pretty evil not to allow you to drop something into one if
it's own subfolders..
Alec, I don't understand.  What is the "real problem" as opposed to the problem 
that my patch (without the enclosing "if") fixes?
oh I misread this - I thought this was that dragging bookmarks into a subfolder
at the same or lower level was causing destruction (because I am seeing that in
the personal toolbar)
nevermind, I'll defer to ben on this.
Yes, without the check it works. How about this as a simplification?:

// Climb up the document to ensure that we're not going to
// drop this item anywhere into itself.
do {
  var targetAncestor = NODE_ID(dropItem);
  dropItem = dropItem.parentNode;
}
while (targetAncestor != "NC:BookmarksRoot" && targetAncestor != sourceID);
if (targetAncestor == sourceID)
  continue;

It looks like its equivalent.  If you've tested it out and it works in all 
cases, then it probably is.  r=morse
alecf, please super-review ben's modification to the patch.  Thanks.
much clearer... sr=alecf
Great, thanks for debugging this steve!
taking.
Assignee: ben → morse
Status: ASSIGNED → NEW
fix checked in
Status: NEW → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
it's kinda lame that we get no cursor feedback for drops that are disallowed, but that is for a different bug.
I tried dropping a folder into itself, into one if its children and dropping the child into the folder that contained
it. It all worked.
This bug is VERIFIED Fixed on all platforms with the 2001050304 builds
Status: RESOLVED → VERIFIED
*** Bug 79406 has been marked as a duplicate of this bug. ***
This is not exactly fixed yet. As pointed out in the dupe bug 79406, you can
still drag and drop onto itself if you select two folders.
reopening.
2001050714 trunk for MacOS9
Severity: major → critical
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
So this remaining problem is caused by reappearance of bug 42080.
Change "continue" to "break" in bookmarkDD.js where the onDrop handler tests for 
dropping a folder on itself and also where it tests for dropping a folder into 
one of its own subfolders (the latter is the patch that was presented here 
(4/21/01 10:14).  That will solve half the problem.  Namely it will catch the 
case where we are dropping folders A and B (where A precedes B) onto A.  However 
it will not catch the case of dropping A and B onto B.  The reason this one and 
not the other is obvious.

Still investigating to come up with a simple patch that will fix the whole 
problem.
Status: REOPENED → ASSIGNED
OK, here's a patch that solves the problem.  Basically what I had to do was to 
break up the loop into two loops -- one that tests each folder being dragged to 
make sure it is not being dropped on itself or on one of its children.  The 
second loop does the actual dropping.  This way if one of the folders fails the 
drop test, we can do a return before doing any of the drops in the second loop.

In order to avoid code duplication between the loops, some of the simple 
variables were changed into arrays.
alecf, ben:  Will one of you please review this and the other superreview it.  
Thanks.
I'm looking forward to this last patch :)
sr=alecf
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
Hmmm, i don't think I can properly check this bug until bug 42080 is fixed. Nonetheless , although I can't
reproduce it yet i did lose one of my folders while trying to verify this bug so I bet there's still something wrong.
Whiteboard: waiting for 42080 to verify
VERIFIED Fixed with 2001053108 builds.
I paid special attention to the case where "we are dropping folders A and B
(where A precedes B) onto A...[and]...the case of dropping A and B onto B." As
well as dropping a folder onto one of its (grand)children. Whew.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.