Bug 1519058 - Some bookmark favicons disappear after clearing history. r=Standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 11 Feb 2019 17:27:29 +0000
changeset 458544 c2784634bafabee48fcd70e22570568391c7187a
parent 458543 6d8e6f960446fe8145ccb028d6644db879f2caa6
child 458545 6b8f94d83037cd30bef3f4f2ab7716baf30e58df
push id77901
push usermak77@bonardo.net
push dateMon, 11 Feb 2019 18:41:44 +0000
treeherderautoland@c2784634bafa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8
bugs1519058
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1519058 - Some bookmark favicons disappear after clearing history. r=Standard8 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");
+});