Bug 1519058 - Some bookmark favicons disappear after clearing history. r=Standard8 a=lizzard
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 11 Feb 2019 17:27:29 +0000
changeset 513044 5da23f314b32
parent 513043 5bd55cb9bdd7
child 513045 f965e1e246ea
push id10696
push usernbeleuzu@mozilla.com
push dateThu, 14 Feb 2019 00:48:15 +0000
treeherdermozilla-beta@401c1404f3b0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8, lizzard
bugs1519058
milestone66.0
Bug 1519058 - Some bookmark favicons disappear after clearing history. r=Standard8 a=lizzard Differential Revision: https://phabricator.services.mozilla.com/D19154
toolkit/components/places/FaviconHelpers.cpp
toolkit/components/places/History.jsm
toolkit/components/places/PlacesDBUtils.jsm
toolkit/components/places/nsFaviconService.cpp
toolkit/components/places/nsPlacesTriggers.h
toolkit/components/places/tests/favicons/test_replaceFaviconData.js
toolkit/components/places/tests/favicons/test_root_icons.js
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -199,21 +199,24 @@ nsresult SetIconInfo(const RefPtr<Databa
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   nsCOMPtr<mozIStorageStatement> insertStmt = aDB->GetStatement(
       "INSERT INTO moz_icons "
       "(icon_url, fixed_icon_url_hash, width, root, expire_ms, data) "
       "VALUES (:url, hash(fixup_url(:url)), :width, :root, :expire, :data) ");
   NS_ENSURE_STATE(insertStmt);
+  // ReplaceFaviconData may replace data for an already existing icon, and in
+  // that case it won't have the page uri at hand, thus it can't tell if the
+  // icon is a root icon or not. For that reason, never overwrite a root = 1.
   nsCOMPtr<mozIStorageStatement> updateStmt = aDB->GetStatement(
       "UPDATE moz_icons SET width = :width, "
       "expire_ms = :expire, "
       "data = :data, "
-      "root = :root "
+      "root = (root  OR :root) "
       "WHERE id = :id ");
   NS_ENSURE_STATE(updateStmt);
 
   for (auto& payload : aIcon.payloads) {
     // Sanity checks.
     MOZ_ASSERT(payload.mimeType.EqualsLiteral(PNG_MIME_TYPE) ||
                    payload.mimeType.EqualsLiteral(SVG_MIME_TYPE),
                "Only png and svg payloads are supported");
@@ -809,17 +812,20 @@ AsyncAssociateIconToPage::Run() {
 
   RefPtr<Database> DB = Database::GetDatabase();
   NS_ENSURE_STATE(DB);
   mozStorageTransaction transaction(
       DB->MainConn(), false, mozIStorageConnection::TRANSACTION_IMMEDIATE);
   nsresult rv;
   if (shouldUpdateIcon) {
     rv = SetIconInfo(DB, mIcon);
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_FAILED(rv)) {
+      (void)transaction.Commit();
+      return rv;
+    }
 
     mIcon.status = (mIcon.status & ~(ICON_STATUS_CACHED)) | ICON_STATUS_SAVED;
   }
 
   // If the page does not have an id, don't try to insert a new one, cause we
   // don't know where the page comes from.  Not doing so we may end adding
   // a page that otherwise we'd explicitly ignore, like a POST or an error page.
   if (mPage.placeId == 0) {
@@ -1026,16 +1032,17 @@ AsyncReplaceFaviconData::Run() {
   RefPtr<Database> DB = Database::GetDatabase();
   NS_ENSURE_STATE(DB);
 
   mozStorageTransaction transaction(
       DB->MainConn(), false, mozIStorageConnection::TRANSACTION_IMMEDIATE);
   nsresult rv = SetIconInfo(DB, mIcon, true);
   if (rv == NS_ERROR_NOT_AVAILABLE) {
     // There's no previous icon to replace, we don't need to do anything.
+    (void)transaction.Commit();
     return NS_OK;
   }
   NS_ENSURE_SUCCESS(rv, rv);
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
   // We can invalidate the cache version since we now persist the icon.
   nsCOMPtr<nsIRunnable> event = NewRunnableMethod(
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -826,25 +826,17 @@ var clear = async function(db) {
     // Remove all non-bookmarked places entries first, this will speed up the
     // triggers work.
     await db.execute(`DELETE FROM moz_places WHERE foreign_count = 0`);
     await db.execute(`DELETE FROM moz_updateoriginsdelete_temp`);
 
     // Expire orphan icons.
     await db.executeCached(`DELETE FROM moz_pages_w_icons
                             WHERE page_url_hash NOT IN (SELECT url_hash FROM moz_places)`);
-    await db.executeCached(`DELETE FROM moz_icons WHERE id IN (
-                              SELECT id FROM moz_icons WHERE root = 0
-                              EXCEPT
-                              SELECT icon_id FROM moz_icons_to_pages
-                            )`);
-    await db.executeCached(`DELETE FROM moz_icons
-                            WHERE root = 1
-                              AND get_host_and_port(icon_url) NOT IN (SELECT host FROM moz_origins)
-                              AND fixup_url(get_host_and_port(icon_url)) NOT IN (SELECT host FROM moz_origins)`);
+    await removeOrphanIcons(db);
 
     // Expire annotations.
     await db.execute(`DELETE FROM moz_annos WHERE NOT EXISTS (
                         SELECT 1 FROM moz_places WHERE id = place_id
                       )`);
 
     // Expire inputhistory.
     await db.execute(`DELETE FROM moz_inputhistory WHERE place_id IN (
@@ -915,33 +907,44 @@ var cleanupPages = async function(db, pa
   // Hosts accumulated during the places delete are updated through a trigger
   // (see nsPlacesTriggers.h).
   await db.executeCached(`DELETE FROM moz_updateoriginsdelete_temp`);
 
   // Expire orphans.
   let hashesToRemove = pagesToRemove.map(p => p.hash);
   await db.executeCached(`DELETE FROM moz_pages_w_icons
                           WHERE page_url_hash IN (${sqlList(hashesToRemove)})`);
-  await db.executeCached(`DELETE FROM moz_icons WHERE id IN (
-                            SELECT id FROM moz_icons WHERE root = 0
-                            EXCEPT
-                            SELECT icon_id FROM moz_icons_to_pages
-                          )`);
-  await db.executeCached(`DELETE FROM moz_icons
-                          WHERE root = 1
-                            AND get_host_and_port(icon_url) NOT IN (SELECT host FROM moz_origins)
-                            AND fixup_url(get_host_and_port(icon_url)) NOT IN (SELECT host FROM moz_origins)`);
+  await removeOrphanIcons(db);
 
   await db.execute(`DELETE FROM moz_annos
                     WHERE place_id IN ( ${ idsList } )`);
   await db.execute(`DELETE FROM moz_inputhistory
                     WHERE place_id IN ( ${ idsList } )`);
 };
 
 /**
+ * Remove icons whose origin is not in moz_origins, unless referenced.
+ * @param db: (Sqlite connection)
+ *      The database.
+ */
+function removeOrphanIcons(db) {
+  return db.executeCached(`
+    DELETE FROM moz_icons WHERE id IN (
+      SELECT id FROM moz_icons WHERE root = 0
+      UNION ALL
+      SELECT id FROM moz_icons
+      WHERE root = 1
+        AND get_host_and_port(icon_url) NOT IN (SELECT host FROM moz_origins)
+        AND fixup_url(get_host_and_port(icon_url)) NOT IN (SELECT host FROM moz_origins)
+      EXCEPT
+      SELECT icon_id FROM moz_icons_to_pages
+    )`);
+}
+
+/**
  * Notify observers that pages have been removed/updated.
  *
  * @param db: (Sqlite connection)
  *      The database.
  * @param pages: (Array of objects)
  *      Pages that have been touched and that need cleaning up.
  *      Each object should have the following properties:
  *          - id: (number) The `moz_places` identifier for the place.
--- a/toolkit/components/places/PlacesDBUtils.jsm
+++ b/toolkit/components/places/PlacesDBUtils.jsm
@@ -613,31 +613,30 @@ var PlacesDBUtils = {
       // MOZ_ICONS
       // E.1 remove orphan icon entries.
       { query:
         `DELETE FROM moz_pages_w_icons WHERE page_url_hash NOT IN (
           SELECT url_hash FROM moz_places
         )`,
       },
 
+      // Remove icons whose origin is not in moz_origins, unless referenced.
       { query:
         `DELETE FROM moz_icons WHERE id IN (
           SELECT id FROM moz_icons WHERE root = 0
+          UNION ALL
+          SELECT id FROM moz_icons
+          WHERE root = 1
+            AND get_host_and_port(icon_url) NOT IN (SELECT host FROM moz_origins)
+            AND fixup_url(get_host_and_port(icon_url)) NOT IN (SELECT host FROM moz_origins)
           EXCEPT
           SELECT icon_id FROM moz_icons_to_pages
         )`,
       },
 
-      { query:
-        `DELETE FROM moz_icons
-         WHERE root = 1
-           AND get_host_and_port(icon_url) NOT IN (SELECT host FROM moz_origins)
-           AND fixup_url(get_host_and_port(icon_url)) NOT IN (SELECT host FROM moz_origins)`,
-      },
-
       // MOZ_HISTORYVISITS
       // F.1 remove orphan visits
       { query:
         `DELETE FROM moz_historyvisits WHERE id IN (
           SELECT id FROM moz_historyvisits v
           WHERE NOT EXISTS
             (SELECT id FROM moz_places WHERE id = v.place_id LIMIT 1)
         )`,
--- a/toolkit/components/places/nsFaviconService.cpp
+++ b/toolkit/components/places/nsFaviconService.cpp
@@ -425,17 +425,20 @@ nsFaviconService::ReplaceFaviconData(nsI
   NS_ENSURE_SUCCESS(rv, rv);
   // URIs can arguably lack a host.
   Unused << aFaviconURI->GetHost(iconData->host);
   if (StringBeginsWith(iconData->host, NS_LITERAL_CSTRING("www."))) {
     iconData->host.Cut(0, 4);
   }
 
   // Note we can't set rootIcon here, because don't know the page it will be
-  // associated with. We'll do that later in SetAndFetchFaviconForPage.
+  // associated with. We'll do that later in SetAndFetchFaviconForPage if the
+  // icon doesn't exist; otherwise, if AsyncReplaceFaviconData updates an
+  // existing icon, it will take care of not overwriting an existing
+  // root = 1 value.
 
   IconPayload payload;
   payload.mimeType = aMimeType;
   payload.data.Assign(TO_CHARBUFFER(aData), aDataLen);
   if (payload.mimeType.EqualsLiteral(SVG_MIME_TYPE)) {
     payload.width = UINT16_MAX;
   }
   // There may already be a previous payload, so ensure to only have one.
--- a/toolkit/components/places/nsPlacesTriggers.h
+++ b/toolkit/components/places/nsPlacesTriggers.h
@@ -163,21 +163,25 @@
     "DELETE FROM moz_origins " \
     "WHERE prefix = OLD.prefix AND host = OLD.host AND NOT EXISTS ( " \
       "SELECT id FROM moz_places " \
       "WHERE origin_id = moz_origins.id " \
       "LIMIT 1 " \
     "); " \
     /* Add the origin's new contribution to frecency stats */ \
     UPDATE_ORIGIN_FRECENCY_STATS("+") "; " \
-    "DELETE FROM moz_icons " \
-    "WHERE fixed_icon_url_hash = hash(fixup_url(OLD.host || '/favicon.ico')) " \
-      "AND fixup_url(icon_url) = fixup_url(OLD.host || '/favicon.ico') " \
-      "AND NOT EXISTS (SELECT 1 FROM moz_origins WHERE host = OLD.host " \
-                                                   "OR host = fixup_url(OLD.host)); " \
+    "DELETE FROM moz_icons WHERE id IN ( " \
+      "SELECT id FROM moz_icons " \
+      "WHERE fixed_icon_url_hash = hash(fixup_url(OLD.host || '/favicon.ico')) " \
+        "AND fixup_url(icon_url) = fixup_url(OLD.host || '/favicon.ico') " \
+        "AND NOT EXISTS (SELECT 1 FROM moz_origins WHERE host = OLD.host " \
+                                                     "OR host = fixup_url(OLD.host)) " \
+      "EXCEPT " \
+      "SELECT icon_id FROM moz_icons_to_pages " \
+    "); " \
   "END" \
 )
 
 // This trigger runs on updates to moz_places.frecency.
 //
 // However, we skip this when frecency changes are due to frecency decay since
 // (1) decay updates all frecencies at once, so this trigger would run for each
 // moz_place, which would be expensive; and (2) decay does not change the
--- a/toolkit/components/places/tests/favicons/test_replaceFaviconData.js
+++ b/toolkit/components/places/tests/favicons/test_replaceFaviconData.js
@@ -237,8 +237,41 @@ add_task(async function test_replaceFavi
             secondFavicon.file.remove(false);
             resolve();
           }, systemPrincipal);
       }, systemPrincipal);
   });
 
   await PlacesUtils.history.clear();
 });
+
+add_task(async function test_replaceFaviconData_rootOverwrite() {
+  info("test replaceFaviconData doesn't overwrite root = 1");
+
+  async function getRootValue(url) {
+    let db = await PlacesUtils.promiseDBConnection();
+    let rows = await db.execute("SELECT root FROM moz_icons WHERE icon_url = :url",
+                                {url});
+    return rows[0].getResultByName("root");
+  }
+
+  const PAGE_URL = "http://rootoverwrite.bar/";
+  let pageURI = Services.io.newURI(PAGE_URL);
+  const ICON_URL = "http://rootoverwrite.bar/favicon.ico";
+  let iconURI = Services.io.newURI(ICON_URL);
+
+  await PlacesTestUtils.addVisits(pageURI);
+
+  let icon = createFavicon("favicon9.png");
+  PlacesUtils.favicons.replaceFaviconData(iconURI, icon.data, icon.data.length,
+                                          icon.mimetype);
+  await PlacesTestUtils.addFavicons(new Map([[PAGE_URL, ICON_URL]]));
+
+  Assert.equal(await getRootValue(ICON_URL), 1, "Check root == 1");
+  let icon2 = createFavicon("favicon10.png");
+  PlacesUtils.favicons.replaceFaviconData(iconURI, icon2.data, icon2.data.length,
+                                          icon2.mimetype);
+  // replaceFaviconData doesn't have a callback, but we must wait its updated.
+  await PlacesTestUtils.promiseAsyncUpdates();
+  Assert.equal(await getRootValue(ICON_URL), 1, "Check root did not change");
+
+  await PlacesUtils.history.clear();
+});
--- a/toolkit/components/places/tests/favicons/test_root_icons.js
+++ b/toolkit/components/places/tests/favicons/test_root_icons.js
@@ -39,44 +39,56 @@ add_task(async function() {
   // Remove all the pages for the given domain.
   await PlacesUtils.history.remove("http://places.test/page2/");
   // The icon should be removed along with the domain.
   rows = await db.execute("SELECT * FROM moz_icons");
   Assert.equal(rows.length, 0, "The icon should have been removed");
 });
 
 add_task(async function test_removePagesByTimeframe() {
+  const BASE_URL = "http://www.places.test";
   // Add a visit in the past with no directly associated icon.
-  await PlacesTestUtils.addVisits({uri: "http://www.places.test/old/", visitDate: new Date(Date.now() - 86400000)});
-  let pageURI = NetUtil.newURI("http://www.places.test/page/");
-  await PlacesTestUtils.addVisits({uri: pageURI, visitDate: new Date(Date.now() - 7200000)});
-  let faviconURI = NetUtil.newURI("http://www.places.test/page/favicon.ico");
-  let rootIconURI = NetUtil.newURI("http://www.places.test/favicon.ico");
+  let oldPageURI = NetUtil.newURI(`${BASE_URL}/old/`);
+  await PlacesTestUtils.addVisits({
+    uri: oldPageURI,
+    visitDate: new Date(Date.now() - 86400000),
+  });
+  // And another more recent visit.
+  let pageURI = NetUtil.newURI(`${BASE_URL}/page/`);
+  await PlacesTestUtils.addVisits({
+    uri: pageURI,
+    visitDate: new Date(Date.now() - 7200000),
+  });
+
+  // Add a normal icon to the most recent page.
+  let faviconURI = NetUtil.newURI(`${BASE_URL}/page/favicon.ico`);
   PlacesUtils.favicons.replaceFaviconDataFromDataURL(
     faviconURI, SMALLSVG_DATA_URI.spec, 0, systemPrincipal);
   await setFaviconForPage(pageURI, faviconURI);
+  // Add a root icon to the most recent page.
+  let rootIconURI = NetUtil.newURI(`${BASE_URL}/favicon.ico`);
   PlacesUtils.favicons.replaceFaviconDataFromDataURL(
     rootIconURI, SMALLPNG_DATA_URI.spec, 0, systemPrincipal);
   await setFaviconForPage(pageURI, rootIconURI);
 
   // Sanity checks.
   Assert.equal(await getFaviconUrlForPage(pageURI),
                faviconURI.spec, "Should get the biggest icon");
   Assert.equal(await getFaviconUrlForPage(pageURI, 1),
                rootIconURI.spec, "Should get the smallest icon");
-  Assert.equal(await getFaviconUrlForPage("http://www.places.test/old/"),
+  Assert.equal(await getFaviconUrlForPage(oldPageURI),
                rootIconURI.spec, "Should get the root icon");
 
+  info("Removing the newer page, not the old one");
   await PlacesUtils.history.removeByFilter({
     beginDate: new Date(Date.now() - 14400000),
     endDate: new Date(),
   });
+  await PlacesTestUtils.promiseAsyncUpdates();
 
-  // Check database entries.
-  await PlacesTestUtils.promiseAsyncUpdates();
   let db = await PlacesUtils.promiseDBConnection();
   let rows = await db.execute("SELECT * FROM moz_icons");
   Assert.equal(rows.length, 1, "There should only be 1 icon entry");
   Assert.equal(rows[0].getResultByName("root"), 1, "It should be marked as a root icon");
   rows = await db.execute("SELECT * FROM moz_pages_w_icons");
   Assert.equal(rows.length, 0, "There should be no page entry");
   rows = await db.execute("SELECT * FROM moz_icons_to_pages");
   Assert.equal(rows.length, 0, "There should be no relation entry");
@@ -123,8 +135,50 @@ add_task(async function test_same_size()
   await setFaviconForPage(pageURI, faviconURI);
   faviconURI = NetUtil.newURI("http://new_places.test/another_icon.ico");
   PlacesUtils.favicons.replaceFaviconData(faviconURI, data, data.length, "image/png");
   await setFaviconForPage(pageURI, faviconURI);
 
   Assert.equal(await getFaviconUrlForPage(pageURI, 20),
                faviconURI.spec, "Should get the non-root icon");
 });
+
+add_task(async function test_root_on_different_host() {
+  async function getRootValue(url) {
+    let db = await PlacesUtils.promiseDBConnection();
+    let rows = await db.execute("SELECT root FROM moz_icons WHERE icon_url = :url",
+                                {url});
+    return rows[0].getResultByName("root");
+  }
+
+  // Check that a root icon associated to 2 domains is not removed when the
+  // root domain is removed.
+  const TEST_URL1 = "http://places1.test/page/";
+  let pageURI1 = NetUtil.newURI(TEST_URL1);
+  await PlacesTestUtils.addVisits(pageURI1);
+
+  const TEST_URL2 = "http://places2.test/page/";
+  let pageURI2 = NetUtil.newURI(TEST_URL2);
+  await PlacesTestUtils.addVisits(pageURI2);
+
+  // Root favicon for TEST_URL1.
+  const ICON_URL = "http://places1.test/favicon.ico";
+  let iconURI = NetUtil.newURI(ICON_URL);
+  PlacesUtils.favicons.replaceFaviconDataFromDataURL(
+    iconURI, SMALLPNG_DATA_URI.spec, 0, systemPrincipal);
+  await setFaviconForPage(pageURI1, iconURI);
+  Assert.equal(await getRootValue(ICON_URL), 1, "Check root == 1");
+  Assert.equal(await getFaviconUrlForPage(pageURI1, 16),
+               ICON_URL, "The icon should been found");
+
+  // Same favicon for TEST_URL2.
+  PlacesUtils.favicons.replaceFaviconDataFromDataURL(
+    iconURI, SMALLPNG_DATA_URI.spec, 0, systemPrincipal);
+  await setFaviconForPage(pageURI2, iconURI);
+  Assert.equal(await getRootValue(ICON_URL), 1, "Check root == 1");
+  Assert.equal(await getFaviconUrlForPage(pageURI2, 16),
+               ICON_URL, "The icon should be found");
+
+  await PlacesUtils.history.remove(pageURI1);
+
+  Assert.equal(await getFaviconUrlForPage(pageURI2, 16),
+               ICON_URL, "The icon should not have been removed");
+});