363621 - [history sidebar] Delete doesn't work properly

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

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

[places] history sidebar, delete (when grouped by date, or date and site) doesn't work properly

spun off from bug #359462, which is just about the command enabling issues with the history sidebar.

details coming...

Comment 1

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

there's a partial patch for this in bug #359462, but it needs work.

to elaborate, there are two issues:

1) top level delete by date doesn't work

2) deleting a site (when grouped by day and site) or specific url (when grouped by date or date and site) can delete other references (from other days).

as pointed out by mconnor, ff 2.0 has the following issue: if you delete a url from the sidebar when grouped by date, it will delete it from all days (a refresh might be necessary to see that it has been removed.)

for the delete by date code, dmills points out that I could generalize
and re-use the nsNavHistoryExpire.cpp code, instead of rewriting the code.

Comment 3

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

Attachment: work in progress patch

things to do:

1)  handle "end of epoch" delete by date.  For last "bucket" (older than 6 days), we should be passing in 0 for endTime, and then adjusting the SQL query to just be visits greater than beginTime.

2)  figure out why after delete the history sidebar does not update / invalidate properly.

3)  move common / duplicated code from nsNavHistory.cpp into nsNavHistoryExpire.cpp.  possibly move nsNavHistory::RemovePages() and nsNavHistory::RemovePagesFromHost(), and rely on nsNavHistoryExpire to do all the work.  Already, nsNavHistory::RemovePages() and nsNavHistory::RemovePagesFromHost() have todo items for erasing annotations.  may need the same for erasing favicons.

4) remove my dump statements in controller.js

spin off bugs to worry about

1)  implement nsNavHistoryExpire::EraseAnnotations()  // FIXME bug 319455 expire annotations

Comment 4

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

> 2)  figure out why after delete the history sidebar does not update /
> invalidate properly.

discussed this with dietrich and dan yesterday.  After doing some debugging (and after working with mano on bug #366136), found the problem:

// nsNavHistoryResult::OnPageExpired (nsINavHistoryObserver)
//
//    Don't do anything when pages expire. Perhaps we want to find the item
//    to delete it.
NS_IMETHODIMP
nsNavHistoryResult::OnPageExpired(nsIURI* aURI, PRTime aVisitTime,
                                  PRBool aWholeEntry)
{
  return NS_OK;
}

Comment 5

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

actually, we might want to do this work in EraseHistory(), but only for pages without visits.

for pages with visits, we will something else.

continuing to investigate.

Comment 6

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

last visited also doesn't work properly, because we are deleting by uri, and not expiring a visit.  

when in "view by last visited" you can duplicate uris, so deleting one can cause several to disappear.

dietrich and I come across this, and some other weirdness (which I'll spin off to another bug) last week.

Comment 8

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

clearing tm for now, but I'd like to say a5 or a6.

Comment 9

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

> when in "view by last visited" you can duplicate uris, so deleting one can
cause several to disappear.

see bug #382026

> top level delete by date doesn't work

see bug #376706

> deleting a site (when grouped by day and site) or specific url (when grouped
by date or date and site) can delete other references (from other days).

see bug #384674

ugh.

Comment 10

[bomfog]

(In reply to comment #9)
> > when in "view by last visited" you can duplicate uris, so deleting one can
> cause several to disappear.
> 
> see bug #382026

which was WONTFIXed and refers to bug #385245 (non-obviously (for this issue) summarized as "history sidebar very slow (way slower than fx2)") as the open bug to watch. Is there now a better one? There seem to be several similar scattered about.

Since:

there are two very different deletion behaviors depending upon the (currently obscured from the user) underlying URLs, and the user's expectation may depend entirely upon which behavior he happened upon first, and there's no undelete in the history sidebar yet (hence data lost), and it doesn't seem unreasonable to expect to be able to keep recent history visible in the sidebar, yet be able to un-clutter it with predictable effect, and I'm impatient, and so on,

I wish this were already fixed.  Thanks for listening :).

Comment 11

[Martijn Wargers (dead)]

I guess this also covers the case when you try to delete all items from 4 days ago, for instance?

Comment 12

[Dietrich Ayala (:dietrich)]

pushing to M10.

Comment 16

[Marco Bonardo [:mak]]

actual behaviour:

- BY LAST VISITED
deleting one visit will delete all visits to that url (expected behaviour per Bug 382026)
working fine

- BY MOST VISITED
deleting one visit will delete all visits to that url (expected behaviour)
working fine

- BY DATE
deleting one visit will delete all visits to that url also in other days (expected behaviour per Bug 384674?)
deleting the day folder does not work and should be fixed

- BY SITE
works but it's slow

- BY DATE AND SITE
works on domain (slow as before), does not work on date

since deleting a visit is mostly a privacy fact, imho we should continue on the line of deleting all visits for an url when deleting an url, in all sorted views (also time-driven ones), as stated by bug 382026 and bug 384674

so only remaining issues should be:
- delete a day folder should work (Bug 376706): catch all urls into that timeframe and delete ALL visits for them

- delete by host is slow: try moving more work into subqueries

if that's ok i'm starting looking at this.

Comment 17

[Dietrich Ayala (:dietrich)]

fyi: bug 385245 may change how the query folders in the history sidebar are constructed, thereby changing how we need to do deletes from these views.

Comment 18

[Marco Bonardo [:mak]]

yes, but i think that we could get the first and last item time into the container and delete between them, so it should not be affected

still have to consider when grouped by date and site

we also have problems with annos queries into RemovePagesFromHost, some query is totally wrong (argh!) like i pointed out in IRC. 
will patch that similar to RemovePages, with a single onUpdateBatch call

Comment 19

[Marco Bonardo [:mak]]

splitted out removePagesByHost into bug 415757

Comment 20

[Marco Bonardo [:mak]]

i'm changing this into a sort of meta bug to track other sidebar delete bugs

Comment 21

[Ondrej Brablc]

With my patch for 385245 deleting by site does not work - I will fix this in the PlacesUtils.nodeIsHost and submit new patch. It may still be little dangerous to work on both bugs in parallel.

Comment 22

[Marco Bonardo [:mak]]

ondrej please note Bug 415757 that is probably landing soon

Comment 23

[Marco Bonardo [:mak]]

As a reminder: after landing of Bug 376706 we should ensure that time is always setup in day containers, as the max(visit_date) of items that it contains (particularly in Ondrej's patch for Bug 385245)

Comment 24

[Ondrej Brablc]

(In reply to comment #23)
> As a reminder: after landing of Bug 376706 we should ensure that time is always
> setup in day containers, as the max(visit_date) of items that it contains
> (particularly in Ondrej's patch for Bug 385245)
> 

This would not be that easy, because I do not know the time unless the day is expanded. With "Date and Site" I do not know it unless I would expand all sites. However, I can set the time to the endTime of the query which represents the day. This should be very much the same for your needs.

Comment 25

[Marco Bonardo [:mak]]

yes, that should be the same, i delete between the previous container time (excluded) and the actual container time (included)

Comment 26

[Jake]

I observe a related behavior on trunk builds (osx 10.4, WinXP). After a browsing session I will selectively delete SOME history entries via the history window/sidebar. They disappear from the history view but the location bar autocomplete will still present some of them when matching what I type even after browser restarts. This is the closest existing bug I found, so I post here. 

Comment 27

[Marco Bonardo [:mak]]

Jake: are those entries starred/bookmarked?

Comment 28

[Jake]

(In reply to comment #27)
> Jake: are those entries starred/bookmarked?
> 

No, they are not. Sorry, should have made that clear. Am still investigating to make reproducible.

Comment 29

[Marco Bonardo [:mak]]

have you imported bookmarks from a firefox 2 profile?

Comment 30

[Jake]

(In reply to comment #29)
> have you imported bookmarks from a firefox 2 profile?
> 

No. Brand new profile, created with the build in question. I first noticed this bug some weeks ago in nightlies, possibly correlated but not necessarily linked with work on the awesomebar. I know nothing of the inner workings, do the two have separate storage systems?

Installed extensions: AdblockPlus, NoScript. I have discovered that NoScript sometimes screws with FF3.0x UI functions such as preventing typing in the locationbar or blocking JS bookmarklets. Not sure if this is relevant here though.

Comment 31

[Marco Bonardo [:mak]]

are those items still there if you do a clear private data and clear browsing history?

Comment 32

[Jake]

(In reply to comment #31)
> are those items still there if you do a clear private data and clear browsing
> history?
> 

That removes all history and autocomplete items as expected.

Comment 33

[Marco Bonardo [:mak]]

please, install Sqlite Manager extension (https://addons.mozilla.org/en-US/firefox/addon/5817). The next time that you see a dangling item into the autocomplete open Sqlite Manager and execute this SQL query:

SELECT h.url, b.id, a.expiration, n.name FROM moz_places h
LEFT JOIN moz_historyvisits v ON h.id = v.place_id
LEFT JOIN moz_annos a ON h.id = a.place_id
LEFT JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id
LEFT JOIN moz_bookmarks b ON h.id = b.fk
WHERE
(b.id IS NOT NULL OR a.id IS NOT NULL)
AND SUBSTR(url, 0, 6) != 'place:'

identify the dangling url in the table, and paste here the values for id, expiration and name columns. thank you.

Comment 34

[Tracy Walker [:tracy]]

Please investigate the difference between location bar "typed in" visits compared to linked to visits.  My guess is the ones you're seeing in the location bar after sidebar history deletion are actually "typed in location bar" visits.  

It looks to me like history delete from the sidebar is not removing the entry from location bar typed in visits like Clear Private Data > Browsing History does.

Comment 35

[Marco Bonardo [:mak]]

delete in the sidebar really should delete ALL visits for items, and also delete pages that are not bookmarked/starred/annotated. All other pages should be deleted, typed urls should not be a problem until they are not falling into previous categories.

Comment 36

[Jake]

I have filed <a href=https://bugzilla.mozilla.org/show_bug.cgi?id=417298">Bug 417298</a> as this has moved in a slightly different direction. I have found out a few more things that are documented there.

Comment 37

[Marco Bonardo [:mak]]

actually i can't see other delete problems in the sidebar, but i'd prefer mantaining this open until Ondrej's patches for the sidebar land, so we can add dependant bugs if needed.

Comment 38

[Marco Bonardo [:mak]]

well, ondrej's patch for sidebar has landed, delete is working fine and it's quite fast too (apart in most visited and last visited but that is mostly due to huge treeview perf problems).

we can consider this fixed.

Comment 39

[Tracy Walker [:tracy]]

almost reopened due to case 2 in comment #1 'til I noticed the later reference to bug 363621.

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008022704 Minefield/3.0b4pre

Comment 41

[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