403040 - Places killed all my history >2 days ago

Status: VERIFIED FIXED

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

Description

[Reed Loden [:reed]]

So, I noticed last night that I was missing three days or so in my history sidebar (I think days 3, 4, and 5), and now when I check today, the only thing in my history sidebar is today and yesterday. What happened to my history? :(

I have no idea how this happened, so I can't give good STR, sorry. I have had to kill Firefox several times lately, so it may be related to some type of shutdown sqlite save or expiration or something?

Comment 1

[Dietrich Ayala (:dietrich)]

What's your browser.history_expire_days value?

Comment 2

[Dietrich Ayala (:dietrich)]

Hrm, no other bugs reported about this. I also searched the build forums for the last few days, no comments about anything like this.

Killing Firefox shouldn't matter: the shutdown work would not have occurred if your force-killed it, and SQLite is (theoretically) immune to data corruption from unexpected shutdown given that we run it in the safest possible mode (synchronous = full).

Are all your bookmarks still there?

Comment 3

[Reed Loden [:reed]]

(In reply to comment #1)
> What's your browser.history_expire_days value?

Both browser.history_expire_days and browser.history_expire_days.mirror are 180 days.

(In reply to comment #2)
> Are all your bookmarks still there?

Yes, all my bookmarks are still there.

Comment 4

[Robert Kaiser]

I'm using places history with my self-built SeaMonkey builds, and lose my places history about daily in the last few days, though it worked perfectly before in older builds. I first thought it would be lost on shutdown/restart, but I noticed that I had a few visited links left from a last session a few times, so it at least isn't at every restart.

Comment 5

[Marco Bonardo [:mak]]

i'm not sure if that could be the problem, but nsNavHistoryExpire::FindVisits it's looking strange:

 sqlBase.AssignLiteral(
    "SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "
    "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
    "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk ");

[omissis]

  if (defaultExpireDays == mHistory->mExpireDays || !aNumToExpire) {
    if (aNumToExpire) {
    sqlVisits.Assign(sqlBase);
      sqlVisits.AppendLiteral("ORDER BY v.visit_date DESC LIMIT ?1 OFFSET ?2 ");

[omissis]

  if (aExpireThreshold && aRecords.Length() < aNumToExpire) {

[omissis]
    sqlDate.Assign(sqlBase);
    sqlDate.AppendLiteral("AND visit_date < ?1 LIMIT ?2");

I feel like the query is lacking a WHERE

Comment 6

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

> I feel like the query is lacking a WHERE

good catch marco.

I think this should be:  "WHERE visit_date < ?1 LIMIT ?2"

The query we are currently executing is:

"SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk 
FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id 
LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk AND visit_date < ?1 LIMIT ?2"

but we want:

"SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk 
FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id 
LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk WHERE visit_date < ?1 LIMIT ?2"

double checking now.

Comment 7

[Dietrich Ayala (:dietrich)]

Yes, this is a typo. The bad query can get executed if you have a non-default value for browser.history_expire_days, or if you have almost exactly 20000k history visits.

Unfortunately, this needs to get in M9.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Comment 9

[Dietrich Ayala (:dietrich)]

Note, the LIMIT clause is still in effect, so it will only delete up to the limit, and should not delete all history in one go. However, given that we delete larger chunks at idle time, if the app is open all night idle, it can go through a few thousand records.

Comment 10

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

Comment on attachment 288015 [details] [diff] [review]
fix v1

r=sspitzer, thanks dietrich (and marco and reed)

I'm not sure comment #7 is correct.

it looks to me that if you have any non-zero value for expire_days, we could execute this query whenever we haven't found "enough" visits to expire.

Comment 11

[Marco Bonardo [:mak]]

Seth is right, the first query deletes records > 20000, if it does not found enough of them then the second query deletes records older than threshold, till the expire limit.

mh even if this is a typo that should be corrected i'm not really sure that it is the actual problem, the query should work even without the where, as stated from "explain query plan"... 
maybe there were more than 20000 history entries in 2 days due to some automatic page refresh?

Comment 12

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

dietrich and I have been trying to figure out if that query is really the cause here, and we agree with marco, it doesn't appear to be.

from http://www.sqlite.org/cvstrac/wiki?p=PerformanceTuning

When SQLite sees this:
  SELECT * FROM a JOIN b ON a.x=b.y;
It translate it into the following before compiling it:
  SELECT * FROM a, b WHERE a.x=b.y;

from our tests, the "AND visit_date < ?1 LIMIT ?2" is working correctly, and limiting the results correctly.  (and this explains why there is no syntax error.)

we've also tested the other query (...ORDER BY v.visit_date DESC LIMIT ?1 OFFSET ?2) and it is doing the right thing as well.

We still think we should take this fix (s/AND/WHERE) for clarity, but it's not the cause of reed's bug.

Comment 13

[Damon Sicore (:damons)]

Comment on attachment 288015 [details] [diff] [review]
fix v1

a=release drivers

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: fix v1.1

minor change for column disambiguation.

Comment 15

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

Comment on attachment 288031 [details] [diff] [review]
fix v1.1

r=sspitzer

Comment 16

[Dietrich Ayala (:dietrich)]

note: reed says his history is basically capped at 2 days.

Comment 17

[Dietrich Ayala (:dietrich)]

checked in the query language change. keeping bug open since that's not the cause.

Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.22; previous revision: 1.21
done

Comment 18

[Dietrich Ayala (:dietrich)]

note: reed does in fact have over 20k visits in the last few days, due to some mochitest window being open and running.

this is a scenario where date-based expiration wins over a deterministic cap. however, this certainly qualifies as an edge case IMO.

kairo's cause is still undetermined, so keeping this open.

Comment 19

[Marco Bonardo [:mak]]

so, this is connected to Bug 330138 and Bug 399606. 
i have rechecked all the queries related to expiring from moz_historyvisits and i can't see other cause for that there.

Imho, if the browser gets a visit to an url less than a minute from the previous visit to the same url, it should upgrade visit_date in the last related historyvisits entry but not add another entry (and don't count in visit_count). I say a minute, but could be 5 minutes.

Comment 20

[Marco Bonardo [:mak]]

another way could be limit the number of entries in moz_historyvisits for a single url. for example retain a maximum of 1000 entries for a single place_id. Could be made after every INSERT (we know the place id) with a
DELETE FROM moz_historyvisits WHERE id IN (
 SELECT FROM moz_historyvisits WHERE place_id = ?1 
 ORDER BY visit_date DESC
 LIMIT 1 OFFSET 1000
)

this will solve the problem with endcap, but will not solve the problem with the predefined query "10 most visited addresses", suppose i visit 10 auto-refresh pages, and that clear history does not reset visit_count. i will end up with a useless query for the time being.

The best thing would be to not count refreshes at all (except for updating visit_date), also when the user click refresh, suppose i am a webmaster and i'm working on 10 websites, i will refresh them by hand very often (even more than 100 times) on a daily basis.

Comment 21

[Robert Kaiser]

As I made a custom build on a non-Firefox app, I may be missing prefs from default pref files so I fall back to values the code has internally. I noticed that about:config says I'm using a default value for browser.history_expire_days that is set to 9 (!) and I see history entires going away after only hours of non-usage, losing the entries at least every day, maybe multiple times a day. I'm surely under 20k URLs at those moments, and I can confirm that it does not matter at all if I restart the app or not, I also lose the entries if the app is running (just had it idle for a night).

Comment 22

[Robert Kaiser]

BTW, browser.history_expire_visits could not be found in my about:config at all - even if Firefox has it set and it's probably sane to have it, we should still work reasonably without it, I guess.

Comment 23

[Marco Bonardo [:mak]]

> I'm surely under 20k URLs at those moments,

the problem is not related to 20000 urls, but to 20000 visits, so if you have a single url and you visit it 20000 times (for example with an automatic refresh script or meta) you have reached the limit with one address.

Comment 24

[Robert Kaiser]

I certainly don't have anything near that number of visits, but I also don't have the pref set to 20k, as pointed out in comment #22

Comment 25

[Marco Bonardo [:mak]]

Attachment: ensure mExpireVisits > 0

if (mExpireVisits == 0) then the history will expire with offset 0 (FindVisits will do a LIMIT 50 OFFSET 0), so everything will be expired from history... we could check that it is always > 0

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 288130 [details] [diff] [review]
ensure mExpireVisits > 0

Hm, I don't think this is the right approach, given that mExpireVisits is a pref that users can validly set to zero. Though, fixing bug 402880 should make this clearer from a UI perspective.

Comment 27

[Robert Kaiser]

We should at least make sure history_expire_visits is set to some sane default when it's not explicitely set in prefs, just like we do for history_expire_days. Now I have set both in my prefs and it looks like history is kept, so the issue seems to lie there - if it's not set, we assume we want to keep 0 entries, which is probably not the default we want.

Comment 28

[Marco Bonardo [:mak]]

Attachment: set mExpireVisits to default if not set in prefs

this is another approach, if the pref fails to load then set it to a safe default value

Comment 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 288823 [details] [diff] [review]
set mExpireVisits to default if not set in prefs

looks good, thanks.

Comment 30

[Dietrich Ayala (:dietrich)]

note: need to update the default in bug 403572, if that value changes.

Comment 31

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.196; previous revision: 1.195
done

I'll update the default in the patch for bug 403572.

Comment 32

[Dietrich Ayala (:dietrich)]

Kairo's scenario is fixed by this patch and Reed's scenario is being addressed in bug 403572, so marking fixed.

Comment 33

[Reed Loden [:reed]]

I *thought* it was mochitest, but it just happened again, and I haven't run mochitest in a long time. I took some screenshots this time (attachment 290364 [details] and attachment 290366 [details]), and I was pointed to bug 263160. So, it seems a core bug is what caused the huge number of refreshes / reloads that ate my history. :(

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