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???

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?

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! :-)

This needs a double plus, right?

nav triage team: we wanted this in beta3, but too late now. nsbeta3-

Vera: release note

PDT marking [rtm need info] since we'd like a fix for this after the patch has had code reviews.

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!!!

the latest fix is r=ben.

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 "++".

PDT marking [rtm need info] because the only reviewer is an sr. Need to have two reviews for all patches.

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 ...

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.

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.

rtm++ for this patch on branch only, as scc said.

fix checked in to branch only. marking rtm-, leaving open to find real fix once drag of multiple items is repaired.

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.

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.

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

*** 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. ***

Fix checked in (this is ben, using hyatt's machine, oops)

*** 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 )

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

clearing status/keyword cruft.

nav triage team: Marking nsbeta1+

ack! this explains my problems in the personal toolbar. setting up dependancies.

alas, this is ongoing. tossing into .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.

fix checked in

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

*** 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

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.

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

sure. r=ben@netscape.com

Fix checked in.

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.

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.