Closed Bug 415757 Opened 13 years ago Closed 13 years ago

RemovePagesFromHost could delete wrong items/annotation

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 beta4

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: dataloss)

Attachments

(1 file, 4 obsolete files)

splitted from Bug 363621

RemovePagesFromHost is doing work in reverse mode, it is selecting places with annotation set to expire never, not deleting all visits, and removing annotations for not selected places... some strange kind of things happens there, we should revert it to the correct ordered work:

- find places within host range
- delete all visits for those places
- expire annotation
- delete dangling items in moz_places
- fix frecency
- use a general batch observer call (for perf, since we are deleting a bunch of urls)
Flags: blocking-firefox3?
Attached patch patch (obsolete) — Splinter Review
this fixes the "wrong way" behaviour
onDeleteURI single calls have been removed for perf, since this is deleting urls in bunch we should only fire a Begin/EndUpdateBatch
Attachment #301483 - Flags: review?(dietrich)
Comment on attachment 301483 [details] [diff] [review]
patch

thanks. please add a test for this (or fix the current ones, if there are any).
Attachment #301483 - Flags: review?(dietrich)
Attached patch patch + test (obsolete) — Splinter Review
added a full dedicated test, FAIL with old function, PASS with new function

the test adds a bunch of visits, then checks that pages have been removed correctly and that annotations were not deleted by error
Attachment #301483 - Attachment is obsolete: true
Attachment #301646 - Flags: review?(dietrich)
Status: NEW → ASSIGNED
Attached patch patch + test (obsolete) — Splinter Review
- created a new internal method RemovePagesInternal
- RemovePage, RemovePages, RemovePagesFromHost will now end up into that method (RemovePage calls RemovePages), good for mantainability and code size win
Attachment #301646 - Attachment is obsolete: true
Attachment #301712 - Flags: review?(dietrich)
Attachment #301646 - Flags: review?(dietrich)
Attached patch patch + test (obsolete) — Splinter Review
too much code will make you forgot to include headers :\
Attachment #301712 - Attachment is obsolete: true
Attachment #301713 - Flags: review?(dietrich)
Attachment #301712 - Flags: review?(dietrich)
Comment on attachment 301713 [details] [diff] [review]
patch + test



>+// nsNavHistory::RemovePagesInternal
> //
>-//    Removes a bunch of uris from history.
>-//    Has better performance than RemovePage when deleting a lot of history.
>-//    Notice that this function does not call the onDeleteURI observers,
>-//    instead, if aDoBatchNotify is true, we call OnBegin/EndUpdateBatch.
>-//    We don't do duplicates removal, URIs array should be cleaned-up before.
>+//    Deletes a list of placeIds from history.
>+//    This is an internal method used by RemovePages and RemovePagesFromHost.
>+//    Takes a comma separated list of place ids.

Hrm, it's irksome that it takes a delimited string instead of an array. But that'd add yet another iteration over the set, so not worth it really.

Please add a comment that this method does not do notification.

r=me otherwise
Attachment #301713 - Flags: review?(dietrich) → review+
Keywords: dataloss
Attached patch for check-inSplinter Review
added comment. Ready for check-in
Attachment #301713 - Attachment is obsolete: true
Comment on attachment 301867 [details] [diff] [review]
for check-in

Drivers: this is an important fix, actual code can cause dataloss of annotations and privacy concern since some url is not really removed from history.
Attachment #301867 - Flags: approval1.9?
Comment on attachment 301867 [details] [diff] [review]
for check-in

thnx Marco!
Attachment #301867 - Flags: approval1.9? → approval1.9+
Flags: blocking-firefox3? → blocking-firefox3+
Checking in toolkit/components/places/public/nsIBrowserHistory.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsIBrowserHistory.idl,v  <--  nsIBrowserHistory.idl
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.245; previous revision: 1.244
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.134; previous revision: 1.133
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_415757.js,v
done
Checking in toolkit/components/places/tests/unit/test_415757.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_415757.js,v  <--  test_415757.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → Firefox 3 beta4
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.