383678 - moving a folder to its grandchild folder (or deeper) results in dataloss

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

nsNavBookmarks::MoveItem() not resetting statement when trying to figure out if parent is not a folder or subfolder of folder 

thanks to sean for spotting this, with the places unit tests and his patch for bug  #381549

I've got a patch (which he suggested), but I want to see which bugs this might have caused before checking in.

Comment 1

[(not reading, please use [e-mail] instead)]

following up here, this bug is something the end user could see (and result in data loss).

the point of the code is to "do nothing" when attempting to move a folder into one of its child or grand child folders.

the current code works fine to "do nothing" when dropping into a child, but if attempt to drop into a grand child (or deeper), the existing code will fail since we are missing the Reset().

without shawn's patch for bug #381549, we'd proceed silently, return no results, and then attempt to move the folder.

this would result in data loss, so updating summary.  (but the patch is the same).

the attached patch makes the drop to grandchildren (and deeper) "do nothing", like we do in the child scenario.

I also think we should show the no-drop indicator if dragging a folder onto a child (or deeper) folder.  I'll check what firefox 2 does in this scenario as well.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: patch

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Created an attachment (id=268012) [details]
> patch
Actually, you should either move this line into the while loop, or remove it if you are calling reset:
http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavBookmarks.cpp#1447

Comment 4

[Dietrich Ayala (:dietrich)]

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

shawn's right - you should either scope inside the loop, or reset manually, but not both. i'd feel more comfortable with scoping, as there are early returns inside the loop. also, can you add tests to test_bookmarks.js for this?

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: better patch, per shawn

Comment 6

[(not reading, please use [e-mail] instead)]

>  also, can you add tests to test_bookmarks.js for this?

working on tests now...

Comment 7

[Shawn Wilsher :sdwilsh]

technically, the tests are what found this bug.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: tests to explicitly test the parent / child / grandchild issue

Comment 9

[(not reading, please use [e-mail] instead)]

> I also think we should show the no-drop indicator if dragging a folder onto a
> child (or deeper) folder.  I'll check what firefox 2 does in this scenario as
> well.

spun off to bug #384095.  (Note, trunk current has parity with fx 2:  on drop, we do nothing.)

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 268028 [details] [diff] [review]
tests to explicitly test the parent / child / grandchild issue

thanks for adding these

Comment 11

[(not reading, please use [e-mail] instead)]

fixed.

Thanks to shawn and his work on bug #381549 for exposing this bug (and for suggestion on a cleaner fix.)

Checking in src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavB
ookmarks.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in tests/bookmarks/test_bookmarks.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v
<--  test_bookmarks.js
new revision: 1.29; previous revision: 1.28
done

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Was the updated test run on this checkin?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

I tested around moving folders with grandchild, great-grandchild, etc. folders. This looks fixed. I saw no issues.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/200708100404 Minefield/3.0a8pre

Comment 14

[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