Bug 663344 - Maintenance may cause history loss.

r=dietrich
2011-06-16 12:04:14 +02:00 · 2011-06-16 12:04:14 +02:00 · d70faead56
--- a/toolkit/components/places/PlacesDBUtils.jsm
+++ b/toolkit/components/places/PlacesDBUtils.jsm
@ -766,8 +766,8 @@ let PlacesDBUtils = {
      PlacesDBUtils._executeTasks(tasks);
    }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);

-    // Force a full expiration step.
-    expiration.observe(null, "places-debug-start-expiration", -1);
+    // Force an orphans expiration step.
+    expiration.observe(null, "places-debug-start-expiration", 0);
  },

  /**
--- a/toolkit/components/places/nsPlacesExpiration.js
+++ b/toolkit/components/places/nsPlacesExpiration.js
@ -527,8 +527,29 @@ nsPlacesExpiration.prototype = {
      }
    }
    else if (aTopic == TOPIC_DEBUG_START_EXPIRATION) {
-      this._debugLimit = aData || -1; // Don't limit if unspecified.
-      this._expireWithActionAndLimit(ACTION.DEBUG, LIMIT.DEBUG);
+      // The passed-in limit is the maximum number of visits to expire when
+      // history is over capacity.  Mind to correctly handle the NaN value.
+      let limit = parseInt(aData);
+      if (limit == -1) {
+        // Everything should be expired without any limit.  If history is over
+        // capacity then all existing visits will be expired.
+        // Should only be used in tests, since may cause dataloss.
+        this._expireWithActionAndLimit(ACTION.DEBUG, LIMIT.UNLIMITED);
+      }
+      else if (limit > 0) {
+        // The number of expired visits is limited by this amount.  It may be
+        // used for testing purposes, like checking that limited queries work.
+        this._debugLimit = limit;
+        this._expireWithActionAndLimit(ACTION.DEBUG, LIMIT.DEBUG);
+      }
+      else {
+        // Any other value is intended as a 0 limit, that means no visits
+        // will be expired.  Even if this doesn't touch visits, it will remove
+        // any orphan pages, icons, annotations and similar from the database,
+        // so it may be used for cleanup purposes.
+        this._debugLimit = -1;
+        this._expireWithActionAndLimit(ACTION.DEBUG, LIMIT.DEBUG);
+      }
    }
    else if (aTopic == TOPIC_IDLE_BEGIN) {
      // Stop the expiration timer.  We don't want to keep up expiring on idle
@ -840,15 +861,20 @@ nsPlacesExpiration.prototype = {
        baseLimit = this._debugLimit;
        break;
    }
-    if (this.status == STATUS.DIRTY && aLimit != LIMIT.DEBUG)
+    if (this.status == STATUS.DIRTY && aAction != ACTION.DEBUG &&
+        baseLimit > 0) {
      baseLimit *= EXPIRE_AGGRESSIVITY_MULTIPLIER;
+    }

    // Bind the appropriate parameters.
    let params = stmt.params;
    switch (aQueryType) {
      case "QUERY_FIND_VISITS_TO_EXPIRE":
        params.max_uris = this._urisLimit;
-        params.limit_visits = baseLimit;
+        // Avoid expiring all visits in case of an unlimited debug expiration,
+        // just remove orphans instead.
+        params.limit_visits =
+          aLimit == LIMIT.DEBUG && baseLimit == -1 ? 0 : baseLimit;
        break;
      case "QUERY_FIND_URIS_TO_EXPIRE":
        // We could run in the middle of adding a new visit or bookmark to
--- a/toolkit/components/places/tests/expiration/head_expiration.js
+++ b/toolkit/components/places/tests/expiration/head_expiration.js
@ -72,6 +72,10 @@ function force_expiration_start() {

 /**
 * Forces an expiration run.
+ *
+ * @param [optional] aLimit
+ *        Limit for the expiration.  Pass -1 for unlimited.
+ *        Any other non-positive value will just expire orphans.
 */
 function force_expiration_step(aLimit) {
  const TOPIC_DEBUG_START_EXPIRATION = "places-debug-start-expiration";
--- a/toolkit/components/places/tests/expiration/test_debug_expiration.js
+++ b/toolkit/components/places/tests/expiration/test_debug_expiration.js
@ -0,0 +1,141 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * What this is aimed to test:
+ *
+ * Expiration can be manually triggered through a debug topic, but that should
+ * only expire orphan entries, unless -1 is passed as limit.
+ */
+
+let gNow = Date.now() * 1000;
+
+add_test(function test_expire_orphans()
+{
+  // Add visits to 2 pages and force a orphan expiration. Visits should survive.
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page1.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page2.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  // Create a orphan place.
+  let id = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                                NetUtil.newURI("http://page3.mozilla.org/"),
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX, "");
+  PlacesUtils.bookmarks.removeItem(id);
+
+  // Observe expiration.
+  Services.obs.addObserver(function (aSubject, aTopic, aData)
+  {
+    Services.obs.removeObserver(arguments.callee, aTopic);
+
+    // Check that visits survived.
+    do_check_eq(visits_in_database("http://page1.mozilla.org/"), 1);
+    do_check_eq(visits_in_database("http://page2.mozilla.org/"), 1);
+    do_check_false(page_in_database("http://page3.mozilla.org/"));
+
+    // Clean up.
+    waitForClearHistory(run_next_test);
+  }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
+
+  // Expire now.
+  force_expiration_step(0);
+});
+
+add_test(function test_expire_orphans_optionalarg()
+{
+  // Add visits to 2 pages and force a orphan expiration. Visits should survive.
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page1.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page2.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  // Create a orphan place.
+  let id = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                                NetUtil.newURI("http://page3.mozilla.org/"),
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX, "");
+  PlacesUtils.bookmarks.removeItem(id);
+
+  // Observe expiration.
+  Services.obs.addObserver(function (aSubject, aTopic, aData)
+  {
+    Services.obs.removeObserver(arguments.callee, aTopic);
+
+    // Check that visits survived.
+    do_check_eq(visits_in_database("http://page1.mozilla.org/"), 1);
+    do_check_eq(visits_in_database("http://page2.mozilla.org/"), 1);
+    do_check_false(page_in_database("http://page3.mozilla.org/"));
+
+    // Clean up.
+    waitForClearHistory(run_next_test);
+  }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
+
+  // Expire now.
+  force_expiration_step();
+});
+
+add_test(function test_expire_limited()
+{
+  // Add visits to 2 pages and force a single expiration.
+  // Only 1 page should survive.
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page1.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page2.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  // Observe expiration.
+  Services.obs.addObserver(function (aSubject, aTopic, aData)
+  {
+    Services.obs.removeObserver(arguments.callee, aTopic);
+
+    // Check that visits to the more recent page survived.
+    do_check_false(page_in_database("http://page1.mozilla.org/"));
+    do_check_eq(visits_in_database("http://page2.mozilla.org/"), 1);
+
+    // Clean up.
+    waitForClearHistory(run_next_test);
+  }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
+
+  // Expire now.
+  force_expiration_step(1);
+});
+
+add_test(function test_expire_unlimited()
+{
+  // Add visits to 2 pages and force a single expiration.
+  // Only 1 page should survive.
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page1.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  PlacesUtils.history.addVisit(NetUtil.newURI("http://page2.mozilla.org/"),
+                               gNow++, null,
+                               PlacesUtils.history.TRANSITION_TYPED, false, 0);
+  // Observe expiration.
+  Services.obs.addObserver(function (aSubject, aTopic, aData)
+  {
+    Services.obs.removeObserver(arguments.callee, aTopic);
+
+    // Check that visits to the more recent page survived.
+    do_check_false(page_in_database("http://page1.mozilla.org/"));
+    do_check_false(page_in_database("http://page2.mozilla.org/"));
+
+    // Clean up.
+    waitForClearHistory(run_next_test);
+  }, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
+
+  // Expire now.
+  force_expiration_step(-1);
+});
+
+function run_test()
+{
+  // Set interval to a large value so we don't expire on it.
+  setInterval(3600); // 1h
+  // Set maxPages to a low value, so it's easy to go over it.
+  setMaxPages(1);
+
+  run_next_test();
+}
--- a/toolkit/components/places/tests/expiration/test_notifications_onDeleteURI.js
+++ b/toolkit/components/places/tests/expiration/test_notifications_onDeleteURI.js
@ -146,7 +146,7 @@ function run_next_test() {
    os.addObserver(observer, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);

    // Expire now, observers will check results.
-    force_expiration_step();
+    force_expiration_step(-1);
  }
  else {
    clearMaxPages();
--- a/toolkit/components/places/tests/expiration/test_pref_maxpages.js
+++ b/toolkit/components/places/tests/expiration/test_pref_maxpages.js
@ -160,7 +160,7 @@ function run_next_test() {

    setMaxPages(gCurrentTest.maxPages);
    // Expire now, observers will check results.
-    force_expiration_step();
+    force_expiration_step(-1);
  }
  else {
    clearMaxPages();
--- a/toolkit/components/places/tests/expiration/xpcshell.ini
+++ b/toolkit/components/places/tests/expiration/xpcshell.ini
@ -13,3 +13,4 @@ tail =
 [test_pref_interval.js]
 [test_pref_maxpages.js]
 [test_removeAllPages.js]
+[test_debug_expiration.js]
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@ -267,16 +267,17 @@ function dump_table(aName)

 /**
 * Checks if an address is found in the database.
- * @param aUrl
- *        Address to look for.
+ * @param aURI
+ *        nsIURI or address to look for.
 * @return place id of the page or 0 if not found
 */
-function page_in_database(aUrl)
+function page_in_database(aURI)
 {
+  let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
  let stmt = DBConn().createStatement(
    "SELECT id FROM moz_places WHERE url = :url"
  );
-  stmt.params.url = aUrl;
+  stmt.params.url = url;
  try {
    if (!stmt.executeStep())
      return 0;
@ -287,6 +288,30 @@ function page_in_database(aUrl)
  }
 }

+/**
+ * Checks how many visits exist for a specified page.
+ * @param aURI
+ *        nsIURI or address to look for.
+ * @return number of visits found.
+ */
+function visits_in_database(aURI)
+{
+  let url = aURI instanceof Ci.nsIURI ? aURI.spec : aURI;
+  let stmt = DBConn().createStatement(
+    "SELECT count(*) FROM moz_historyvisits v "
+  + "JOIN moz_places h ON h.id = v.place_id "
+  + "WHERE url = :url"
+  );
+  stmt.params.url = url;
+  try {
+    if (!stmt.executeStep())
+      return 0;
+    return stmt.getInt64(0);
+  }
+  finally {
+    stmt.finalize();
+  }
+}

 /**
 * Removes all bookmarks and checks for correct cleanup