Bug 1763577 - MR2-426 - Implement initial snapshots expiration. r=mossop

Differential Revision: https://phabricator.services.mozilla.com/D143144

browser/app/profile/firefox.js

@@ -2709,3 +2709,8 @@ pref("browser.snapshots.score.InNavigation", 3);
 pref("browser.snapshots.score.IsOverlappingVisit", 3);
 pref("browser.snapshots.score.IsUserPersisted", 1);
 pref("browser.snapshots.score.IsUsedRemoved", -10);
+
+// Expiration days for snapshots.
+pref("browser.places.snapshots.expiration.days", 210);
+// For user managed snapshots we use more than a year, to support yearly tasks.
+pref("browser.places.snapshots.expiration.userManaged.days", 420);

browser/components/places/SnapshotMonitor.jsm

@@ -20,16 +20,38 @@ XPCOMUtils.defineLazyModuleGetters(this, {
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "SNAPSHOT_ADDED_TIMER_DELAY",
-  "browser.places.snapshot.monitorDelayAdded",
+  "browser.places.snapshots.monitorDelayAdded",
   5000
 );
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "SNAPSHOT_REMOVED_TIMER_DELAY",
-  "browser.places.snapshot.monitorDelayRemoved",
+  "browser.places.snapshots.monitorDelayRemoved",
   1000
 );
 
+// Expiration days for automatic and user managed snapshots.
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "SNAPSHOT_EXPIRE_DAYS",
+  "browser.places.snapshots.expiration.days",
+  210
+);
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "SNAPSHOT_USERMANAGED_EXPIRE_DAYS",
+  "browser.places.snapshots.expiration.userManaged.days",
+  420
+);
+// We expire on the next idle after a snapshot was added or removed, and
+// idle-daily, but we don't want to expire too often or rarely.
+// Thus we define both a mininum and maximum time in the session among which
+// we'll expire chunks of snapshots.
+const EXPIRE_EVERY_MIN_MS = 60 * 60000; // 1 Hour.
+const EXPIRE_EVERY_MAX_MS = 120 * 60000; // 2 Hours.
+// The number of snapshots to expire at once.
+const EXPIRE_CHUNK_SIZE = 10;
+
 /**
  * Monitors changes in snapshots (additions, deletions, etc) and triggers
  * the snapshot group builders to run as necessary.
@@ -81,6 +103,15 @@ const SnapshotMonitor = new (class SnapshotMonitor {
    */
   testGroupBuilders = null;
 
+  /**
+   * The time of the last snapshots expiration.
+   */
+  #lastExpirationTime = 0;
+  /**
+   * How many snapshots to expire per chunk.
+   */
+  #expirationChunkSize = EXPIRE_CHUNK_SIZE;
+
   /**
    * Internal getter to get the builders used.
    *
@@ -169,6 +200,81 @@ const SnapshotMonitor = new (class SnapshotMonitor {
     this.#removedUrls.clear();
   }
 
+  /**
+   * Triggers expiration of a chunk of snapshots.
+   * We differentiate snapshots depending on whether they are user managed:
+   *  1. manually created by the user
+   *  2. part of a group
+   *     TODO: evaluate whether we want to consider user managed only snapshots
+   *           that are part of a user curated group, rather than any group.
+   * User managed snapshots will expire if their last interaction is older than
+   * browser.snapshots.expiration.userManaged.days, while others will expire
+   * after browser.snapshots.expiration.days.
+   * Snapshots that have a tombstone (removed_at is set) should not be expired.
+   *
+   * @param {boolean} onIdle
+   *   Whether this is running on idle. When it's false expiration is
+   *   rescheduled for the next idle.
+   */
+  async #expireSnapshotsChunk(onIdle = false) {
+    let now = Date.now();
+    if (now - this.#lastExpirationTime < EXPIRE_EVERY_MIN_MS) {
+      return;
+    }
+    let instance = (this._expireInstance = {});
+    let skip = false;
+    if (!onIdle) {
+      // Wait for the next idle.
+      skip = await new Promise(resolve =>
+        ChromeUtils.idleDispatch(deadLine => {
+          // Skip if we couldn't find an idle, unless we're over max waiting time.
+          resolve(
+            deadLine.didTimeout &&
+              now - this.#lastExpirationTime < EXPIRE_EVERY_MAX_MS
+          );
+        })
+      );
+    }
+    if (skip || instance != this._expireInstance) {
+      return;
+    }
+
+    this.#lastExpirationTime = now;
+    let urls = (
+      await Snapshots.query({
+        includeUserPersisted: false,
+        includeTombstones: false,
+        group: null,
+        lastInteractionBefore: now - SNAPSHOT_EXPIRE_DAYS * 86400000,
+        limit: this.#expirationChunkSize,
+      })
+    ).map(s => s.url);
+    if (instance != this._expireInstance) {
+      return;
+    }
+
+    if (urls.length < this.#expirationChunkSize) {
+      // If we couldn't find enough automatic snapshots, check if there's any
+      // user managed ones we can expire.
+      urls.push(
+        ...(
+          await Snapshots.query({
+            includeUserPersisted: true,
+            includeTombstones: false,
+            lastInteractionBefore:
+              now - SNAPSHOT_USERMANAGED_EXPIRE_DAYS * 86400000,
+            limit: this.#expirationChunkSize - urls.length,
+          })
+        ).map(s => s.url)
+      );
+    }
+    if (instance != this._expireInstance) {
+      return;
+    }
+
+    await Snapshots.delete([...new Set(urls)], true);
+  }
+
   /**
    * Sets a timer ensuring that if the new timeout would occur sooner than the
    * current target time, the timer is changed to the sooner time.
@@ -188,10 +294,10 @@ const SnapshotMonitor = new (class SnapshotMonitor {
     }
 
     this.#currentTargetTime = targetTime;
-    this.#timer = setTimeout(
-      () => this.#triggerBuilders().catch(console.error),
-      timeout
-    );
+    this.#timer = setTimeout(() => {
+      this.#expireSnapshotsChunk().catch(console.error);
+      this.#triggerBuilders().catch(console.error);
+    }, timeout);
   }
 
   /**
@@ -203,12 +309,24 @@ const SnapshotMonitor = new (class SnapshotMonitor {
    * @param {nsISupports} data
    */
   async observe(subject, topic, data) {
-    if (topic == "places-snapshots-added") {
-      this.#onSnapshotAdded(JSON.parse(data));
-    } else if (topic == "places-snapshots-deleted") {
-      this.#onSnapshotRemoved(JSON.parse(data));
-    } else if (topic == "idle-daily") {
-      await this.#triggerBuilders(true);
+    switch (topic) {
+      case "places-snapshots-added":
+        this.#onSnapshotAdded(JSON.parse(data));
+        break;
+      case "places-snapshots-deleted":
+        this.#onSnapshotRemoved(JSON.parse(data));
+        break;
+      case "idle-daily":
+        await this.#expireSnapshotsChunk(true);
+        await this.#triggerBuilders(true);
+        break;
+      case "test-expiration":
+        this.#lastExpirationTime =
+          subject.lastExpirationTime || this.#lastExpirationTime;
+        this.#expirationChunkSize =
+          subject.expirationChunkSize || this.#expirationChunkSize;
+        await this.#expireSnapshotsChunk(subject.onIdle);
+        break;
     }
   }
 

browser/components/places/Snapshots.jsm

@@ -396,33 +396,58 @@ const Snapshots = new (class Snapshots {
   }
 
   /**
-   * Deletes a snapshot, creating a tombstone. Note, the caller is expected
-   * to take account of the userPersisted value for a Snapshot when appropriate.
+   * Deletes one or more snapshots.
+   * By default this creates a tombstone rather than removing the entry, so that
+   * heuristics can take into account user removed snapshots.
+   * Note, the caller is expected to take account of the userPersisted value
+   * for a Snapshot when appropriate.
    *
-   * @param {string} url
-   *   The url of the snapshot to delete.
+   * @param {string|Array<string>} urls
+   *   The url of the snapshot to delete, or an Array of urls.
+   * @param {boolean} removeFromStore
+   *   Whether the snapshot should actually be removed rather than tombston-ed.
    */
-  async delete(url) {
-    url = this.stripFragments(url);
-    await PlacesUtils.withConnectionWrapper("Snapshots: delete", async db => {
-      let placeId = (
-        await db.executeCached(
+  async delete(urls, removeFromStore = false) {
+    if (!Array.isArray(urls)) {
+      urls = [urls];
+    }
+    urls = urls.map(this.stripFragments);
+
+    let placeIdsSQLFragment = `
+    SELECT id FROM moz_places
+    WHERE url_hash IN (${PlacesUtils.sqlBindPlaceholders(
+      urls,
+      "hash(",
+      ")"
+    )}) AND url IN (${PlacesUtils.sqlBindPlaceholders(urls)})`;
+    let queryArgs = removeFromStore
+      ? [
+          `DELETE FROM moz_places_metadata_snapshots
+         WHERE place_id IN (${placeIdsSQLFragment})
+         RETURNING place_id`,
+          [...urls, ...urls],
+        ]
+      : [
           `UPDATE moz_places_metadata_snapshots
-           SET removed_at = :removedAt
-           WHERE place_id = (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url)
-           RETURNING place_id`,
-          { removedAt: Date.now(), url }
-        )
-      )[0].getResultByName("place_id");
+         SET removed_at = ?
+         WHERE place_id IN (${placeIdsSQLFragment})
+         RETURNING place_id`,
+          [Date.now(), ...urls, ...urls],
+        ];
+
+    await PlacesUtils.withConnectionWrapper("Snapshots: delete", async db => {
+      let placeIds = (await db.executeCached(...queryArgs)).map(r =>
+        r.getResultByName("place_id")
+      );
       // Remove orphan page data.
       await db.executeCached(
         `DELETE FROM moz_places_metadata_snapshots_extra
-         WHERE place_id = :placeId`,
-        { placeId }
+         WHERE place_id IN (${PlacesUtils.sqlBindPlaceholders(placeIds)})`,
+        placeIds
       );
     });
 
-    this.#notify("places-snapshots-deleted", [url]);
+    this.#notify("places-snapshots-deleted", urls);
   }
 
   /**
@@ -479,10 +504,15 @@ const Snapshots = new (class Snapshots {
    * @param {number} [options.type]
    *   Restrict the snapshots to those with a particular type of page data available.
    * @param {number} [options.group]
-   *   Restrict the snapshots to those within a particular group.
+   *   Restrict the snapshots to those within a particular group. Pass null
+   *   to get all the snapshots that are not part of a group.
    * @param {boolean} [options.includeHiddenInGroup]
    *   Only applies when querying a particular group. Pass true to include
    *   snapshots that are hidden in the group.
+   * @param {boolean} [options.includeUserPersisted]
+   *   Whether to include user persisted snapshots.
+   * @param {number} [options.lastInteractionBefore]
+   *   Restrict to snaphots whose last interaction was before the given time.
    * @param {boolean} [options.sortDescending]
    *   Whether or not to sortDescending. Defaults to true.
    * @param {string} [options.sortBy]
@@ -497,6 +527,8 @@ const Snapshots = new (class Snapshots {
     type = undefined,
     group = undefined,
     includeHiddenInGroup = false,
+    includeUserPersisted = true,
+    lastInteractionBefore = undefined,
     sortDescending = true,
     sortBy = "last_interaction_at",
   } = {}) {
@@ -511,12 +543,26 @@ const Snapshots = new (class Snapshots {
       clauses.push("removed_at IS NULL");
     }
 
+    if (!includeUserPersisted) {
+      clauses.push("user_persisted = :user_persisted");
+      bindings.user_persisted = this.USER_PERSISTED.NO;
+    }
+    if (lastInteractionBefore) {
+      clauses.push("last_interaction_at < :last_interaction_before");
+      bindings.last_interaction_before = lastInteractionBefore;
+    }
+
     if (type) {
       clauses.push("type = :type");
       bindings.type = type;
     }
 
-    if (group) {
+    if (group === null) {
+      clauses.push("group_id IS NULL");
+      joins.push(
+        "LEFT JOIN moz_places_metadata_groups_to_snapshots g USING(place_id)"
+      );
+    } else if (group) {
       clauses.push("group_id = :group");
       if (!includeHiddenInGroup) {
         clauses.push("g.hidden = 0");

browser/components/places/tests/unit/interactions/test_snapshots_expiration.js

@@ -0,0 +1,187 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests for the expiration of snapshots.
+ */
+
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "SNAPSHOT_EXPIRE_DAYS",
+  "browser.places.snapshots.expiration.days",
+  210
+);
+XPCOMUtils.defineLazyPreferenceGetter(
+  this,
+  "SNAPSHOT_USERMANAGED_EXPIRE_DAYS",
+  "browser.places.snapshots.expiration.userManaged.days",
+  420
+);
+
+// For each snapshot define its url, whether it should be expired, whether it
+// should be a tomstone and a type.
+// The type may be:
+//   - manual: user_persisted
+//   - auto: created automatically by some heuristic
+//   - group: part of a group
+let gSnapshots = [
+  {
+    url: "https://example.com/1",
+    type: "manual",
+    expired: true,
+    tombstone: false,
+  },
+  {
+    url: "https://example.com/2",
+    type: "manual",
+    expired: false,
+    tombstone: false,
+  },
+  {
+    url: "https://example.com/3",
+    type: "group",
+    expired: true,
+    tombstone: false,
+  },
+  {
+    url: "https://example.com/4",
+    type: "auto",
+    expired: true,
+    tombstone: false,
+  },
+  {
+    url: "https://example.com/5",
+    type: "auto",
+    expired: false,
+    tombstone: false,
+  },
+  {
+    url: "https://example.com/6",
+    type: "auto",
+    expired: true,
+    tombstone: true,
+  },
+];
+
+add_task(async function setup() {
+  let now = Date.now();
+  let interactions = gSnapshots.map(s => {
+    if (s.expired) {
+      s.created_at =
+        now -
+        (1 + s.type != "auto"
+          ? SNAPSHOT_USERMANAGED_EXPIRE_DAYS
+          : SNAPSHOT_EXPIRE_DAYS) *
+          86400000;
+    } else {
+      s.created_at =
+        now - (s.type != "auto" ? SNAPSHOT_EXPIRE_DAYS : 1) * 86400000;
+    }
+    return s;
+  });
+  await addInteractions(interactions);
+
+  let groupSerial = 1;
+  for (let snapshot of gSnapshots) {
+    if (snapshot.type == "manual") {
+      snapshot.userPersisted = Snapshots.USER_PERSISTED.MANUAL;
+    }
+    await Snapshots.add(snapshot);
+    if (snapshot.type == "group") {
+      snapshot.group = await SnapshotGroups.add(
+        {
+          title: `test-group-${groupSerial++}`,
+          builder: "test",
+        },
+        [snapshot.url]
+      );
+    }
+    if (snapshot.tombstone) {
+      await Snapshots.delete(snapshot.url);
+    }
+  }
+
+  Services.prefs.setBoolPref("browser.places.interactions.enabled", true);
+
+  SnapshotMonitor.init();
+});
+
+add_task(async function test_idle_expiration() {
+  await SnapshotMonitor.observe({ onIdle: true }, "test-expiration");
+
+  let remaining = await Snapshots.query({ includeTombstones: true });
+  for (let snapshot of gSnapshots) {
+    let index = remaining.findIndex(s => s.url == snapshot.url);
+    if (!snapshot.expired || snapshot.tombstone) {
+      Assert.greater(index, -1, `${snapshot.url} should not have been removed`);
+      remaining.splice(index, 1);
+    } else {
+      Assert.equal(index, -1, `${snapshot.url} should have been removed`);
+    }
+  }
+  Assert.ok(
+    !remaining.length,
+    `All the snapshots should be processed: ${JSON.stringify(remaining)}`
+  );
+});
+
+add_task(async function test_active_limited_expiration() {
+  // Add 2 expirable snapshots.
+  let now = Date.now();
+  let expiredSnapshots = [
+    {
+      url: "https://example.com/7",
+      created_at: now - (SNAPSHOT_USERMANAGED_EXPIRE_DAYS + 1) * 86400000,
+    },
+    {
+      url: "https://example.com/8",
+      created_at: now - (SNAPSHOT_USERMANAGED_EXPIRE_DAYS + 1) * 86400000,
+    },
+  ];
+  for (let snapshot of expiredSnapshots) {
+    await addInteractions([snapshot]);
+    await Snapshots.add(snapshot);
+  }
+
+  let snapshots = await Snapshots.query({ includeTombstones: true });
+
+  info("expire again without setting lastExpirationTime, should be a no-op");
+  let expirationChunkSize = 1;
+  await SnapshotMonitor.observe(
+    {
+      expirationChunkSize,
+    },
+    "test-expiration"
+  );
+
+  // Since expiration just ran, nothing should have been expired.
+  Assert.equal(
+    (await Snapshots.query({ includeTombstones: true })).length,
+    snapshots.length,
+    "No snapshot should have been expired."
+  );
+
+  info("expire again, for real");
+  await SnapshotMonitor.observe(
+    {
+      expirationChunkSize,
+      lastExpirationTime: now - 24 * 86400000,
+    },
+    "test-expiration"
+  );
+
+  let remaining = await Snapshots.query({ includeTombstones: true });
+
+  let count = 0;
+  for (let snapshot of expiredSnapshots) {
+    let index = remaining.findIndex(s => s.url == snapshot.url);
+    if (index == -1) {
+      count++;
+    }
+  }
+  Assert.equal(
+    count,
+    expiredSnapshots.length - expirationChunkSize,
+    "Check the expected number of snapshots have been expired"
+  );
+});

browser/components/places/tests/unit/interactions/xpcshell.ini

@@ -20,6 +20,7 @@ skip-if = toolkit == 'android' # bug 1730213
 [test_snapshots_common_referrer_queries.js]
 [test_snapshots_create_allow_protocols.js]
 [test_snapshots_create_criteria.js]
+[test_snapshots_expiration.js]
 [test_snapshots_fragments.js]
 [test_snapshots_overlapping_queries.js]
 [test_snapshots_pagedata.js]