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

VERIFIED FIXED in Firefox 3 alpha6

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

({dataloss})

Trunk
Firefox 3 alpha6
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Status: NEW → ASSIGNED
Flags: blocking-firefox3?
OS: Windows XP → All
Summary: nsNavBookmarks::MoveItem() not resetting statement when trying to figure out if parent is not a folder or subfolder of folder → moving a folder to its granchild folder (or deeper) results in dataloss
Target Milestone: --- → Firefox 3 alpha6
Summary: moving a folder to its granchild folder (or deeper) results in dataloss → moving a folder to its grandchild folder (or deeper) results in dataloss
(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 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?
Attachment #268012 - Flags: review?(dietrich)
Created attachment 268021 [details] [diff] [review]
better patch, per shawn
Attachment #268012 - Attachment is obsolete: true
Attachment #268021 - Flags: review?(dietrich)
>  also, can you add tests to test_bookmarks.js for this?

working on tests now...
technically, the tests are what found this bug.
Created attachment 268028 [details] [diff] [review]
tests to explicitly test the parent / child / grandchild issue
Attachment #268028 - Flags: review?(dietrich)
> 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.)
Attachment #268021 - Flags: review?(dietrich) → review+
Comment on attachment 268028 [details] [diff] [review]
tests to explicitly test the parent / child / grandchild issue

thanks for adding these
Attachment #268028 - Flags: review?(dietrich) → review+
Keywords: dataloss
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Was the updated test run on this checkin?
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
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.