387573 - ExpireHistoryParanoid does not cleanup all unused values

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marcia Knous [:marcia]]

Seen using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/2007071004 Minefield/3.0a7pre. I first noticed this bug in yesterday's nightly.

STR:
1. With an existing profile that has only been used in Minefield, add a bunch of sites to build up a history. I surfed to ~10 sites. While I was doing this, I had the History sidebar open to verify sites were being added to History.
2. Clear Private Data from the menu.
3. Observe that some sites are still listed in the History menu, but all sites are removed from the sidebar.

Note that this does not happen with a new profile, and restarting the browser does not eliminate the menu listings.

I have sent my places.sqlite file to seth for investigation.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Do you see any error or warning inside the Error Console?

Comment 2

[Marcia Knous [:marcia]]

I did not see anything in the console of note.
(In reply to comment #1)
> Do you see any error or warning inside the Error Console?
> 

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I tested with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007071017 Minefield/3.0a7pre and I can see that one entry inside the history menu stays forever.

Comment 4

[Marco Bonardo [:mak]]

can this be due to Bug 381898?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't think so. I used sqlite browser to have a look at the sqlite database. There is an inconsistency which let some history entries stay forever.

I used "Clear Private Data" to get rid off all history entries which are stored correctly. Afterwards I opened the table moz_historyvisits. It's empty but one history entry is still visible inside the menu. So I opened moz_places and search for that URL. It's listed as id 42 - thanks goes to Douglas Adam for the answer! ;)

Seth, where can I find the SQL query for the history menu? I could have a look why that happens.

Comment 6

[Marcia Knous [:marcia]]

Attachment: Screenshot before clearing private data, new profile

This is a screenshot of my History menu after building up some history and before clearing private data.

Comment 7

[Marcia Knous [:marcia]]

Attachment: Screenshot after clearing private data, new profile

Note some of the sites that were there did not go away.

Comment 8

[Marco Bonardo [:mak]]

function should be nsNavHistoryExpire::clearHistory

it calls ExpireItems and expire paranoid functions (they do not delete anything in historyvisits table), so problem could be in ExpireItems

ExpireItems calls eraseVisits, that really don't do anything fancy (delete where id=? with list of id) eraseVisits can be rewrited (see Bug 386711) but it's not the problem here.

To find history items it uses Findvisits, but it should take everything from history visits, don't see problems there

You have an entry in moz_places, but have some entry in moz_historyvisits that links to that moz_places entry?

it could really be Bug 381898, Seth said that in current menu query "we are not ignoring items that are "inner content" visits (visit_type == 4,
or TRANSITION_EMBED see nsINavHistoryService.idl)"

Comment 9

[Marco Bonardo [:mak]]

Henrik Skupin could you check if hidden=1 for your place id 42?

Comment 10

[Marco Bonardo [:mak]]

or the problem could be in nsNavHistory.cpp in the query

        "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
          "(SELECT MAX(visit_date) FROM moz_historyvisits WHERE place_id = h.id),"
          "f.url, null, null "
        "FROM moz_places h "
        "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
        "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");

should probably be JOIN moz_historyvisits v ON h.id = v.place_id 
and not a LEFT JOIN, but this is again covered by Bug 381898

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #8)
> You have an entry in moz_places, but have some entry in moz_historyvisits that
> links to that moz_places entry?

No. The table moz_historyvisits is empty but moz_places contains the entry which is displayed inside the history menu. The complete entry looks like:

INSERT INTO moz_places VALUES(42,'http://www.heise.de/','heise online',NULL,'ed.esieh.www.',1,0,0,9);

What means hidden?

> it could really be Bug 381898, Seth said that in current menu query "we are not
> ignoring items that are "inner content" visits (visit_type == 4,
> or TRANSITION_EMBED see nsINavHistoryService.idl)"

As I already mentioned this table is empty.

The select issue is solved with Seth comment in bug 381898 comment 1 before the fix of bug 372025 went in. But I don't think that this is our problem. IMO the entry wasn't removed from moz_places by while clearing the history. If this is the cause it won't be a dupe.

Comment 12

[Marco Bonardo [:mak]]

The new query from last Seth patch in bug 381898 will solve the problem of showing, because it use a simple JOIN instead of a wrong left join.

the problem here is moz_places content that should have been removed, in this case the historyvisits table is void, so ExpireItem has deleted all entries from there (and that is right behaviour).

ExpireHistoryParanoid should delete every entry from moz_places that is not bookmarked and that is not a special "place:" uri, and that has not an active "link" to historyvisits table...

let's see the query

      "DELETE FROM moz_places WHERE id IN (SELECT h.id FROM moz_places h "
      "LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id "
      "LEFT OUTER JOIN moz_bookmarks b ON h.id = b.fk "
      "WHERE v.id IS NULL "
      "AND b.type = ?1 AND b.fk IS NULL "
      "AND SUBSTR(h.url,0,6) <> 'place:')"),

where ?1 binds to TYPE_BOOKMARK = 1. 

this query is not correct because of 
(b.type = ?1 AND b.fk IS NULL)
i don't have any item with b.type=1 and b.fk=NULL, if an item is bookmarked (type=1) it IS in moz_places (fk is linked to place_id)

so i think that the correct query should be
b.type IS NULL
that should select all moz_places items that do not appear in moz_bookmarks table, while previous was selecting bookmarked (type=1) items with a missing fk (that should not exists if there are no bugs in other functions)...

this, with the fix of Bug 381898 solves the problem completely on my system.

Adding a simple patch 

Comment 13

[Marco Bonardo [:mak]]

Attachment: ExpireHistoryParanoid does not delete all unneeded items

this patched query DELETE from moz_places where:
1. has not entries in moz_historyvisits
2. has not entries in moz_bookmarks
3. is not a special "place:" url

this does not require bind, so it's a ExecuteSimpleSQL instead of a CreateStatement

Comment 14

[Marco Bonardo [:mak]]

Attachment: ExpireHistoryParanoid does not delete all unneeded items

maybe we should use b.id IS NULL, id is indexed and COULD be faster (not so much but it is always better to check non existance on id)

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, I patched my trunk sources and rebuild my debug version. Your patch doesn't work. The entry I talked about in comment 11 doesn't get deleted. Please add this to you moz_places table:

INSERT INTO moz_places VALUES(42,'http://www.heise.de/','heise
online',NULL,'ed.esieh.www.',1,0,0,9);

Comment 16

[Marco Bonardo [:mak]]

is that linked to something in moz_bookmarks (something in moz_bookmarks has a fk=42)?

Try this query:

SELECT v.id, b.id 
FROM moz_places h
LEFT JOIN moz_historyvisits v ON v.place_id=h.id
LEFT JOIN moz_bookmarks b ON b.fk=h.id
WHERE h.id = 42

it should return NULL, NULL

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

You are right. There is an entry with fk=42:

"16","1","42","6","2","Minefield Start Page","0","","1183926046108642","1183926066175106"

But that's not my start page.

Comment 18

[Marco Bonardo [:mak]]

Humm, that is probably a total different problem... That entry is linked to bookmarks and will never get deleted from clear history

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the question is where does it come from? I never set this website as start page. Ok, I'll try to get it reproduced and file a separate bug than. Thanks. Marcia, do you have the same situation?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Oh I forgot... But why is it listed inside the history menu with all the other visited websites?

Comment 21

[Marco Bonardo [:mak]]

because the menu select query is wrong as per Comment #12, that is covered in Bug  381898

using a left join it takes items from moz_places even if they have no entries in moz_historyvists, it should use a join to avoid that.

Comment 22

[Marco Bonardo [:mak]]

you entry has parent id=6, it should be Bookmark Toolbar Folder... do you see that item on the bookmark toolbar?

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, your last comment was a really good note! Now I know what happens here. It's just simple. The only thing you have to do is adding a bookmark from the currently opened website. Put it in the BTF or at any other place. At this point this entry has two meanings. It's now a bookmark but also existent within the history. When you start the "Clear Private Data" function, only that pages are deleted from the history menu which weren't added as bookmark. Any other page is not removed from the menu and stays forever. Even if you delete the newly added bookmark you won't get rid of these history entries.

Comment 24

[Marco Bonardo [:mak]]

if you delete the bookmark and then clear history data they should go away

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, as I already described in my last comment, deleting the bookmark doesn't help. Please try it yourself with the above steps and you will see it.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I think that these are two different issues we have to fix:

1. The sql query for the history menu has to be updated. Only entries which exist inside the table moz_historyvisits should be listed inside the menu.

2. Unused entries in moz_places have to be removed completely if there is no history or bookmark entry with the same id anymore.

Comment 27

[Marco Bonardo [:mak]]

Yes, that is exactly what i'm talking about:

1. will be fixed in Bug 381898
2. should be fixed by patch in Comment #14

from my tests after patching and clearing i don't see any item in moz_places that is not linked to historyvisits or bookmarks. 
i see problems in the menu but they will be patched elsewhere

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so let's make this bug depend on bug 381898. Sorry for confusion. ;)

Which remaining problems you are talking about? Is it worth to immediately file a new bug for that?

Comment 29

[Marco Bonardo [:mak]]

no, there are only those 2 problems, i've seen another trivial problem but i need to test it better, i'll open another report if necessary. thx

updating title

comment #12 is a brief description of the problem
short version: 
the actual query deletes where (b.type = ?1 AND b.fk IS NULL) but this is never true because a bookmark type = 1 has always a b.fk. Correct query should delete where b.id IS NULL, so it will remove from places items that have no visits and are not in bookmarks

comment #14 contains a possible fix

waiting for input on this

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marco, does your latest patch still need review? I really would like to have this bug fixed. My history menu clutters up with unwanted entries more and more. :/

Comment 31

[Dietrich Ayala (:dietrich)]

Comment on attachment 272092 [details] [diff] [review]
ExpireHistoryParanoid does not delete all unneeded items

this fix makes sense, thanks for catching this marco.

Comment 32

[Dietrich Ayala (:dietrich)]

checked in with bug 393472.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Checked with todays nightly build. Works fine. Thanks.

Comment 34

[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