Bug 1357555 - Properly handle an expiration value of 0 when fetching an icon. r=adw
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 18 Apr 2017 23:51:05 +0200
changeset 354039 a6bbb991c0d2d13c34d675df39eec6db9155fbbf
parent 354038 ecc42c31cc283151f6a452af171045687023cf75
child 354040 ad7d2ae91ce67d5cdc71e36293d8978235269c10
push id31685
push userkwierso@gmail.com
push dateThu, 20 Apr 2017 21:45:29 +0000
treeherdermozilla-central@5e3dc7e1288a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1357555
milestone55.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 1357555 - Properly handle an expiration value of 0 when fetching an icon. r=adw Favicons migrated from the old store have expiration set to 0. The code is not expecting that, since expiration has always been positive, thus those icons are always considered valid and never replaced. They should instead be considered expired. MozReview-Commit-ID: Cz0JB4IbURA
toolkit/components/places/FaviconHelpers.cpp
toolkit/components/places/tests/favicons/test_expire_migrated_icons.js
toolkit/components/places/tests/favicons/xpcshell.ini
toolkit/components/places/tests/head_common.js
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -154,16 +154,17 @@ FetchPageInfo(const RefPtr<Database>& aD
 nsresult
 SetIconInfo(const RefPtr<Database>& aDB,
             IconData& aIcon,
             bool aMustReplace = false)
 {
   MOZ_ASSERT(!NS_IsMainThread());
   MOZ_ASSERT(aIcon.payloads.Length() > 0);
   MOZ_ASSERT(!aIcon.spec.IsEmpty());
+  MOZ_ASSERT(aIcon.expiration > 0);
 
   // There are multiple cases possible at this point:
   //   1. We must insert some payloads and no payloads exist in the table. This
   //      would be a straight INSERT.
   //   2. The table contains the same number of payloads we are inserting. This
   //      would be a straight UPDATE.
   //   3. The table contains more payloads than we are inserting. This would be
   //      an UPDATE and a DELETE.
@@ -514,18 +515,17 @@ AsyncFetchAndSetIconForPage::Run()
   MOZ_ASSERT(!NS_IsMainThread());
 
   // Try to fetch the icon from the database.
   RefPtr<Database> DB = Database::GetDatabase();
   NS_ENSURE_STATE(DB);
   nsresult rv = FetchIconInfo(DB, 0, mIcon);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  bool isInvalidIcon = !mIcon.payloads.Length() ||
-                       (mIcon.expiration && PR_Now() > mIcon.expiration);
+  bool isInvalidIcon = !mIcon.payloads.Length() || PR_Now() > mIcon.expiration;
   bool fetchIconFromNetwork = mIcon.fetchMode == FETCH_ALWAYS ||
                               (mIcon.fetchMode == FETCH_IF_MISSING && isInvalidIcon);
 
   if (!fetchIconFromNetwork) {
     // There is already a valid icon or we don't want to fetch a new one,
     // directly proceed with association.
     RefPtr<AsyncAssociateIconToPage> event =
         new AsyncAssociateIconToPage(mIcon, mPage, mCallback);
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/favicons/test_expire_migrated_icons.js
@@ -0,0 +1,27 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * This file tests that favicons migrated from a previous profile, having a 0
+ * expiration, will be properly expired when fetching new ones.
+ */
+
+add_task(function* test_storing_a_normal_16x16_icon() {
+  const PAGE_URL = "http://places.test";
+  yield PlacesTestUtils.addVisits(PAGE_URL);
+  yield setFaviconForPage(PAGE_URL, SMALLPNG_DATA_URI);
+
+  // Now set expiration to 0 and change the payload.
+  do_print("Set expiration to 0 and replace favicon data");
+  yield PlacesUtils.withConnectionWrapper("Change favicons payload", db => {
+    return db.execute(`UPDATE moz_icons SET expire_ms = 0, data = "test"`);
+  });
+
+  let {data, mimeType} = yield getFaviconDataForPage(PAGE_URL);
+  Assert.equal(mimeType, "image/png");
+  Assert.deepEqual(data, "test".split("").map(c => c.charCodeAt(0)));
+
+  do_print("Refresh favicon");
+  yield setFaviconForPage(PAGE_URL, SMALLPNG_DATA_URI, false);
+  yield compareFavicons("page-icon:" + PAGE_URL, SMALLPNG_DATA_URI);
+});
--- a/toolkit/components/places/tests/favicons/xpcshell.ini
+++ b/toolkit/components/places/tests/favicons/xpcshell.ini
@@ -19,16 +19,17 @@ support-files =
   favicon-multi-frame32.png
   favicon-multi-frame64.png
   favicon-normal16.png
   favicon-normal32.png
   favicon-scale160x3.jpg
   favicon-scale3x160.jpg
 
 [test_expireAllFavicons.js]
+[test_expire_migrated_icons.js]
 [test_expire_on_new_icons.js]
 [test_favicons_conversions.js]
 [test_favicons_protocols_ref.js]
 [test_getFaviconDataForPage.js]
 [test_getFaviconURLForPage.js]
 [test_moz-anno_favicon_mime_type.js]
 [test_multiple_frames.js]
 [test_page-icon_protocol.js]
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -858,25 +858,27 @@ function sortBy(array, prop) {
 }
 
 /**
  * Asynchronously set the favicon associated with a page.
  * @param page
  *        The page's URL
  * @param icon
  *        The URL of the favicon to be set.
+ * @param [optional] forceReload
+ *        Whether to enforce reloading the icon.
  */
-function setFaviconForPage(page, icon) {
+function setFaviconForPage(page, icon, forceReload = true) {
   let pageURI = page instanceof Ci.nsIURI ? page
                                           : NetUtil.newURI(new URL(page).href);
   let iconURI = icon instanceof Ci.nsIURI ? icon
                                           : NetUtil.newURI(new URL(icon).href);
   return new Promise(resolve => {
     PlacesUtils.favicons.setAndFetchFaviconForPage(
-      pageURI, iconURI, true,
+      pageURI, iconURI, forceReload,
       PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,
       resolve,
       Services.scriptSecurityManager.getSystemPrincipal()
     );
   });
 }
 
 function getFaviconUrlForPage(page, width = 0) {