Bug 419170 ? The history includes too few days, browser.history_expire_days_min is ignored (for mak77@supereva.it, r=dietrich)
authordietrich@mozilla.com
Tue, 18 Mar 2008 11:03:34 -0700
changeset 13237 8d868c7d6aaf37d7a069e2791f9f2f20db7fdfaa
parent 13236 7b33524c41ae397e42331b89afad46071f408822
child 13238 484c482db80801282a2eaac982f8aa3be65c101b
push idunknown
push userunknown
push dateunknown
reviewersdietrich
bugs419170
milestone1.9b5pre
Bug 419170 ? The history includes too few days, browser.history_expire_days_min is ignored (for mak77@supereva.it, r=dietrich)
toolkit/components/places/src/nsNavHistory.cpp
toolkit/components/places/src/nsNavHistoryExpire.cpp
toolkit/components/places/src/nsNavHistoryExpire.h
toolkit/components/places/tests/unit/test_expiration.js
--- a/toolkit/components/places/src/nsNavHistory.cpp
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -1782,16 +1782,19 @@ PRBool nsNavHistory::IsURIStringVisited(
 nsresult
 nsNavHistory::LoadPrefs(PRBool aInitializing)
 {
   if (! mPrefBranch)
     return NS_OK;
 
   mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS_MAX, &mExpireDaysMax);
   mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS_MIN, &mExpireDaysMin);
+  // Cap max days to min days to prevent expiring pages younger than min
+  if (mExpireDaysMax < mExpireDaysMin)
+    mExpireDaysMax = mExpireDaysMin;
   if (NS_FAILED(mPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_SITES,
                                         &mExpireSites)))
     mExpireSites = EXPIRATION_CAP_SITES;
   
 #ifdef MOZ_XUL
   PRBool oldCompleteOnlyTyped = mAutoCompleteOnlyTyped;
   mPrefBranch->GetBoolPref(PREF_AUTOCOMPLETE_ONLY_TYPED,
                            &mAutoCompleteOnlyTyped);
--- a/toolkit/components/places/src/nsNavHistoryExpire.cpp
+++ b/toolkit/components/places/src/nsNavHistoryExpire.cpp
@@ -240,20 +240,20 @@ nsNavHistoryExpire::OnQuit()
 //    notifications are not a significant part of clear history time.
 
 nsresult
 nsNavHistoryExpire::ClearHistory()
 {
   mozIStorageConnection* connection = mHistory->GetStorageConnection();
   NS_ENSURE_TRUE(connection, NS_ERROR_OUT_OF_MEMORY);
 
-  PRBool keepGoing;
-  nsresult rv = ExpireItems(0, &keepGoing);
-  if (NS_FAILED(rv))
-    NS_WARNING("ExpireItems failed.");
+  // expire visits, then let the paranoid functions do the cleanup for us
+  nsresult rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
+      "DELETE FROM moz_historyvisits"));
+  NS_ENSURE_SUCCESS(rv, rv);
 
   rv = ExpireHistoryParanoid(connection, -1);
   if (NS_FAILED(rv))
     NS_WARNING("ExpireHistoryParanoid failed.");
 
   rv = ExpireFaviconsParanoid(connection);
   if (NS_FAILED(rv))
     NS_WARNING("ExpireFaviconsParanoid failed.");
@@ -347,17 +347,17 @@ nsNavHistoryExpire::ExpireItems(PRUint32
 
   *aKeepGoing = PR_TRUE;
 
   PRInt64 expireTime;
   if (aNumToExpire == 0) {
     // special case: erase all history
     expireTime = 0;
   } else {
-    expireTime = PR_Now() - GetExpirationTimeAgo();
+    expireTime = PR_Now() - GetExpirationTimeAgo(mHistory->mExpireDaysMax);
   }
 
   // find some visits to expire
   nsTArray<nsNavHistoryExpireRecord> expiredVisits;
   nsresult rv = FindVisits(expireTime, aNumToExpire, connection,
                            expiredVisits);
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -435,94 +435,96 @@ nsNavHistoryExpireRecord::nsNavHistoryEx
   erased = PR_FALSE;
 }
 
 
 // nsNavHistoryExpire::FindVisits
 //
 //    Find visits to expire, meeting the following criteria:
 //
-//    * With a visit date greater than (now - browser.history_expire_days_min)
-//    * With a visit date less than (now - browser.history_expire_days)
-//    * With a visit date greater than the minimum, and less than the maximum,
-//      and over the visit cap of browser.history_expire_sites.
+//    * With a visit date older than browser.history_expire_days ago.
+//    * With a visit date older than browser.history_expire_days_min ago
+//      if we have more than browser.history_expire_sites unique urls.
 //
 //    aExpireThreshold is the time at which we will delete visits before.
-//    If it is zero, we will not use a threshold and will match everything.
+//    If it is zero, we will match everything.
 //
 //    aNumToExpire is the maximum number of visits to find. If it is 0, then
 //    we will get all matching visits.
 
 nsresult
 nsNavHistoryExpire::FindVisits(PRTime aExpireThreshold, PRUint32 aNumToExpire,
                                mozIStorageConnection* aConnection,
                                nsTArray<nsNavHistoryExpireRecord>& aRecords)
 {
-  // Select moz_places records, including whether visited and whether the URI
-  // is bookmarked or not.
-  nsCAutoString sqlBase;
-  sqlBase.AssignLiteral(
-    "SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, "
-    "(SELECT fk FROM moz_bookmarks WHERE fk = h.id) "
-    "FROM moz_places h JOIN moz_historyvisits v ON h.id = v.place_id ");
+  // Select a limited number of visits older than a time
+  nsCOMPtr<mozIStorageStatement> selectStatement;
+  nsresult rv = aConnection->CreateStatement(NS_LITERAL_CSTRING(
+      "SELECT v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, "
+        "(SELECT fk FROM moz_bookmarks WHERE fk = h.id) "
+      "FROM moz_places h JOIN moz_historyvisits v ON h.id = v.place_id "
+      "WHERE v.visit_date < ?1 "
+      "ORDER BY v.visit_date ASC LIMIT ?2"),
+    getter_AddRefs(selectStatement));
+    NS_ENSURE_SUCCESS(rv, rv);
 
-  // 1. Expire records older than the max-age cap
-  nsCAutoString sqlMaxAge;
-  sqlMaxAge.Assign(sqlBase);
-
-  if (aNumToExpire) {
-    // Select records older than the max-age cap
-    sqlMaxAge.AppendLiteral("WHERE v.visit_date < ?1 "
-                            "ORDER BY v.visit_date ASC LIMIT ?2");
-  }
-
-  nsCOMPtr<mozIStorageStatement> selectStatement;
-  nsresult rv = aConnection->CreateStatement(sqlMaxAge, getter_AddRefs(selectStatement));
+  // browser.history_expire_days || match all visits
+  PRTime expireMaxTime = aExpireThreshold ? aExpireThreshold : LL_MAXINT;
+  rv = selectStatement->BindInt64Parameter(0, expireMaxTime);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  if (aNumToExpire) {
-    rv = selectStatement->BindInt64Parameter(0, aExpireThreshold);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = selectStatement->BindInt64Parameter(1, aNumToExpire);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
+  // use LIMIT -1 to not limit
+  PRInt32 numToExpire = aNumToExpire ? aNumToExpire : -1;
+  rv = selectStatement->BindInt64Parameter(1, numToExpire);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool hasMore = PR_FALSE;
   while (NS_SUCCEEDED(selectStatement->ExecuteStep(&hasMore)) && hasMore) {
     nsNavHistoryExpireRecord record(selectStatement);
     aRecords.AppendElement(record);
   }
 
-  // 2. if no over-max-age records are found, select records older than the min-age cap AND over the sites cap.
-  if (!aRecords.Length()) {
-    nsCAutoString sqlMinAge;
-    sqlMinAge.Assign(sqlBase);
-
-    // Select records older than the max-age cap
-    // Setting the visit cap as the OFFSET value selects the next aNumToExpire
-    // records above the cap.
-    sqlMinAge.AppendLiteral("WHERE v.visit_date < ?1 "
-                            "ORDER BY v.visit_date DESC LIMIT ?2 OFFSET ?3");
-
-    nsCOMPtr<mozIStorageStatement> selectMinStatement;
-    nsresult rv = aConnection->CreateStatement(sqlMinAge, getter_AddRefs(selectMinStatement));
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    PRInt64 minDaysAgo = mHistory->mExpireDaysMin * 86400 * PR_USEC_PER_SEC;
-    PRTime minThreshold = PR_Now() - minDaysAgo;
-    rv = selectMinStatement->BindInt64Parameter(0, minThreshold);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = selectMinStatement->BindInt64Parameter(1, aNumToExpire);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = selectMinStatement->BindInt32Parameter(2, mHistory->mExpireSites);
+  // If we have found less than aNumToExpire over-max-age records, and we are
+  // over the unique urls cap, select records older than the min-age cap .
+  if (aRecords.Length() < aNumToExpire) {
+    // check the number of visited unique urls in the db.
+    nsCOMPtr<mozIStorageStatement> countStatement;
+    rv = aConnection->CreateStatement(NS_LITERAL_CSTRING(
+        "SELECT count(*) FROM moz_places WHERE visit_count > 0"),
+      getter_AddRefs(countStatement));
     NS_ENSURE_SUCCESS(rv, rv);
 
     hasMore = PR_FALSE;
-    while (NS_SUCCEEDED(selectMinStatement->ExecuteStep(&hasMore)) && hasMore) {
-      nsNavHistoryExpireRecord record(selectMinStatement);
+    // initialize to mExpiresites to avoid expiring if something goes wrong
+    PRInt32 pageCount = mHistory->mExpireSites;
+    if (NS_SUCCEEDED(countStatement->ExecuteStep(&hasMore)) && hasMore) {
+      rv = countStatement->GetInt32(0, &pageCount);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    // Don't find any more pages to expire if we have not reached the urls cap
+    if (pageCount <= mHistory->mExpireSites)
+        return NS_OK;
+
+    rv = selectStatement->Reset();
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // browser.history_expire_days_min
+    PRTime expireMinTime = PR_Now() -
+                           GetExpirationTimeAgo(mHistory->mExpireDaysMin);
+    rv = selectStatement->BindInt64Parameter(0, expireMinTime);
+    NS_ENSURE_SUCCESS(rv, rv);
+    
+    numToExpire = aNumToExpire - aRecords.Length();
+    rv = selectStatement->BindInt64Parameter(1, numToExpire);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    hasMore = PR_FALSE;
+    while (NS_SUCCEEDED(selectStatement->ExecuteStep(&hasMore)) && hasMore) {
+      nsNavHistoryExpireRecord record(selectStatement);
       aRecords.AppendElement(record);
     }
   }
 
   return NS_OK;
 }
 
 
@@ -1020,17 +1022,17 @@ nsNavHistoryExpire::ComputeNextExpiratio
 
   PRBool hasMore;
   rv = statement->ExecuteStep(&hasMore);
   if (NS_FAILED(rv) || !hasMore)
     return; // no items, we'll leave mNextExpirationTime = 0 and try to expire
             // again next time
 
   PRTime minTime = statement->AsInt64(0);
-  mNextExpirationTime = minTime + GetExpirationTimeAgo();
+  mNextExpirationTime = minTime + GetExpirationTimeAgo(mHistory->mExpireDaysMax);
 }
 
 
 // nsNavHistoryExpire::StartTimer
 
 nsresult
 nsNavHistoryExpire::StartTimer(PRUint32 aMilleseconds)
 {
@@ -1054,24 +1056,20 @@ nsNavHistoryExpire::TimerCallback(nsITim
   that->mTimerSet = PR_FALSE;
   that->DoPartialExpiration();
 }
 
 
 // nsNavHistoryExpire::GetExpirationTimeAgo
 
 PRTime
-nsNavHistoryExpire::GetExpirationTimeAgo()
+nsNavHistoryExpire::GetExpirationTimeAgo(PRInt32 aExpireDays)
 {
-  PRInt64 expireDays = mHistory->mExpireDaysMax;
-
   // Prevent Int64 overflow for people that type in huge numbers.
   // This number is 2^63 / 24 / 60 / 60 / 1000000 (reversing the math below)
-  const PRInt64 maxDays = 106751991;
-  if (expireDays > maxDays)
-    expireDays = maxDays;
+  const PRInt32 maxDays = 106751991;
+  if (aExpireDays > maxDays)
+    aExpireDays = maxDays;
 
   // compute how long ago to expire from
-  const PRInt64 secsPerDay = 24*60*60;
-  const PRInt64 usecsPerSec = 1000000;
-  const PRInt64 usecsPerDay = secsPerDay * usecsPerSec;
-  return expireDays * usecsPerDay;
+  // seconds per day = 86400 = 24*60*60
+  return (PRTime)aExpireDays * 86400 * PR_USEC_PER_SEC;
 }
--- a/toolkit/components/places/src/nsNavHistoryExpire.h
+++ b/toolkit/components/places/src/nsNavHistoryExpire.h
@@ -105,10 +105,10 @@ protected:
   nsresult ExpireAnnotationsParanoid(mozIStorageConnection* aConnection);
   nsresult ExpireInputHistoryParanoid(mozIStorageConnection* aConnection);
 
   PRBool ExpireForDegenerateRuns();
 
   nsresult StartTimer(PRUint32 aMilleseconds);
   static void TimerCallback(nsITimer* aTimer, void* aClosure);
 
-  PRTime GetExpirationTimeAgo();
+  PRTime GetExpirationTimeAgo(PRInt32 aExpireDays);
 };
--- a/toolkit/components/places/tests/unit/test_expiration.js
+++ b/toolkit/components/places/tests/unit/test_expiration.js
@@ -451,17 +451,17 @@ function run_test() {
 function startIncrementalExpirationTests() {
   startExpireNeither();
 }
 
 var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
 var ghist = Cc["@mozilla.org/browser/global-history;2"].getService(Ci.nsIGlobalHistory2);
 
 /*
-test 1: NO EXPIRATION CRITERIA MET
+test 1: NO EXPIRATION CRITERIA MET (INSIDE SITES CAP)
 
 1. zero visits > {browser.history_expire_days}
 2. zero visits > {browser.history_expire_days_min}
    AND total visited site count < {browser.history_expire_sites}
 
 steps:
   - clear history
   - reset observer
@@ -481,17 +481,17 @@ function startExpireNeither() {
   // setup
   histsvc.removeAllPages();
   observer.expiredURI = null;
 
   // add data
   histsvc.addVisit(testURI, Date.now() * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
 
-  // set visit cap to 2
+  // set sites cap to 2
   prefs.setIntPref("browser.history_expire_sites", 2);
   // set date minimum to 2
   prefs.setIntPref("browser.history_expire_days_min", 2);
   // set date maximum to 3
   prefs.setIntPref("browser.history_expire_days", 3);
 
   // trigger expiration
   ghist.addURI(triggerURI, false, true, null); 
@@ -541,17 +541,17 @@ function startExpireDaysOnly() {
 
   // add expirable visit
   histsvc.addVisit(testURI, (Date.now() - (86400 * 2 * 1000)) * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
 
   // add un-expirable visit
   histsvc.addVisit(uri("http://unexpirable.com"), (Date.now() - (86400 * 1000)) * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
 
-  // set visit cap to 2
+  // set sites cap to 2
   prefs.setIntPref("browser.history_expire_sites", 2);
   // set date minimum to 2
   prefs.setIntPref("browser.history_expire_days_min", 2);
   // set date maximum to 3
   prefs.setIntPref("browser.history_expire_days", 3);
 
   // trigger expiration
   ghist.addURI(triggerURI, false, true, null); 
@@ -577,55 +577,116 @@ test 3: MIN-AGE+VISIT-CAP CRITERIA MET
 
 1. zero visits > {browser.history_expire_days}
 2. some visits > {browser.history_expire_days_min}
    AND total visited sites count > {browser.history_expire_sites}
 
 steps:
   - clear history
   - reset observer
-  - add a visit, 2 days old
-  - add a visit, 2 days old
+  - add a visit to an url, 2 days old
+  - add a visit to another url, today
   - set browser.history_expire_days to 3
   - set browser.history_expire_days_min to 1
   - set browser.history_expire_sites to 1
   - kick off incremental expiration
 
 confirmation:
-  - check onPageExpired, confirm our visit was expired
-  - query for the visit, confirm it's not there
+  - check onPageExpired, confirm our oldest visit was expired
+  - query for the oldest visit, confirm it's not there
 */
 function startExpireBoth() {
   // setup
   histsvc.removeAllPages();
   observer.expiredURI = null;
   dump("starting expiration test 3: both criteria met\n");
 
   // add visits
   // 2 days old, in microseconds
   var age = (Date.now() - (86400 * 2 * 1000)) * 1000;
   dump("AGE: " + age + "\n");
   histsvc.addVisit(testURI, age, null, histsvc.TRANSITION_TYPED, false, 0);
-  histsvc.addVisit(testURI, age, null, histsvc.TRANSITION_TYPED, false, 0);
   annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
 
-  // set visit cap to 1
+  // set sites cap to 1
   prefs.setIntPref("browser.history_expire_sites", 1);
   // set date max to 3
   prefs.setIntPref("browser.history_expire_days", 3);
   // set date minimum to 1
   prefs.setIntPref("browser.history_expire_days_min", 1);
 
   // trigger expiration
-  ghist.addURI(triggerURI, false, true, null);
+  histsvc.addVisit(triggerURI, Date.now() * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
+  annosvc.setPageAnnotation(triggerURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
 
   // setup confirmation
   do_timeout(3600, "checkExpireBoth();"); // incremental expiration timer is 3500
 }
 
 function checkExpireBoth() {
   try {
     do_check_eq(observer.expiredURI, testURI.spec);
     do_check_eq(annosvc.getPageAnnotationNames(testURI, {}).length, 0);
+    do_check_eq(annosvc.getPageAnnotationNames(triggerURI, {}).length, 1);
   } catch(ex) {}
   dump("done expiration test 3\n");
+  startExpireNeitherOver()
+}
+
+/*
+test 4: NO EXPIRATION CRITERIA MET (OVER SITES CAP)
+
+1. zero visits > {browser.history_expire_days}
+2. zero visits > {browser.history_expire_days_min}
+   AND total visited site count > {browser.history_expire_sites}
+
+steps:
+  - clear history
+  - reset observer
+  - add a visit to an url, w/ current date
+  - add a visit to another url, w/ current date
+  - set browser.history_expire_days to 3
+  - set browser.history_expire_days_min to 2
+  - set browser.history_expire_sites to 1
+  - kick off incremental expiration
+
+confirmation:
+  - check onPageExpired, confirm nothing was expired
+  - query for the visit, confirm it's there
+
+*/
+function startExpireNeitherOver() {
+  dump("startExpireNeitherOver()\n");
+  // setup
+  histsvc.removeAllPages();
+  observer.expiredURI = null;
+
+  // add data
+  histsvc.addVisit(testURI, Date.now() * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
+  annosvc.setPageAnnotation(testURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
+
+  // set sites cap to 1
+  prefs.setIntPref("browser.history_expire_sites", 1);
+  // set date minimum to 2
+  prefs.setIntPref("browser.history_expire_days_min", 2);
+  // set date maximum to 3
+  prefs.setIntPref("browser.history_expire_days", 3);
+
+  // trigger expiration
+  histsvc.addVisit(triggerURI, Date.now() * 1000, null, histsvc.TRANSITION_TYPED, false, 0);
+  annosvc.setPageAnnotation(triggerURI, testAnnoName, testAnnoVal, 0, annosvc.EXPIRE_WITH_HISTORY);
+
+  // setup confirmation
+  do_timeout(3600, "checkExpireNeitherOver();"); // incremental expiration timer is 3500
+}
+
+function checkExpireNeitherOver() {
+  dump("checkExpireNeitherOver()\n");
+  try {
+    do_check_eq(observer.expiredURI, null);
+    do_check_eq(annosvc.getPageAnnotationNames(testURI, {}).length, 1);
+    do_check_eq(annosvc.getPageAnnotationNames(triggerURI, {}).length, 1);
+  } catch(ex) {
+    do_throw(ex);
+  }
+  dump("done incremental expiration test 4\n");
   do_test_finished();
 }