Bug 1359456 - Page-icon protocol is wrongly removing any ref from the page url. r=adw draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 28 Apr 2017 17:47:10 +0200
changeset 570256 960db557a697674762d45624fb8e651c9476dbc3
parent 569138 0b77ed3f26c5335503bc16e85b8c067382e7bb1e
child 626450 a4fe06b4f07baa5872c82af90e44a5acf2e1491a
push id56443
push usermak77@bonardo.net
push dateFri, 28 Apr 2017 16:52:25 +0000
reviewersadw
bugs1359456
milestone55.0a1
Bug 1359456 - Page-icon protocol is wrongly removing any ref from the page url. r=adw MozReview-Commit-ID: FejFQj1Px7n
toolkit/components/places/FaviconHelpers.cpp
toolkit/components/places/PageIconProtocolHandler.js
toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js
toolkit/components/places/tests/favicons/test_page-icon_protocol.js
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -402,16 +402,18 @@ FetchIconPerSpec(const RefPtr<Database>&
   // for a specific size when an associated icon for that size doesn't exist.
   nsCOMPtr<mozIStorageStatement> stmt = aDB->GetStatement(
     "/* do not warn (bug no: not worth having a compound index) */ "
     "SELECT width, icon_url, root "
     "FROM moz_icons i "
     "JOIN moz_icons_to_pages ON i.id = icon_id "
     "JOIN moz_pages_w_icons p ON p.id = page_id "
     "WHERE page_url_hash = hash(:url) AND page_url = :url "
+       "OR (:hash_idx AND page_url_hash = hash(substr(:url, 0, :hash_idx)) "
+                     "AND page_url = substr(:url, 0, :hash_idx)) "
     "UNION ALL "
     "SELECT width, icon_url, root "
     "FROM moz_icons i "
     "WHERE fixed_icon_url_hash = hash(fixup_url(:root_icon_url)) "
     "ORDER BY width DESC, root ASC "
   );
   NS_ENSURE_STATE(stmt);
   mozStorageStatementScoper scoper(stmt);
@@ -420,16 +422,19 @@ FetchIconPerSpec(const RefPtr<Database>&
   NS_ENSURE_SUCCESS(rv, rv);
   nsAutoCString rootIconFixedUrl(aPageHost);
   if (!rootIconFixedUrl.IsEmpty()) {
     rootIconFixedUrl.AppendLiteral("/favicon.ico");
   }
   rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("root_icon_url"),
                                   rootIconFixedUrl);
   NS_ENSURE_SUCCESS(rv, rv);
+  int32_t hashIdx = PromiseFlatCString(aPageSpec).RFind("#");
+  rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("hash_idx"), hashIdx + 1);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   // Return the biggest icon close to the preferred width. It may be bigger
   // or smaller if the preferred width isn't found.
   bool hasResult;
   while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
     int32_t width;
     rv = stmt->GetInt32(0, &width);
     if (!aIconData.spec.IsEmpty() && width < aPreferredWidth) {
--- a/toolkit/components/places/PageIconProtocolHandler.js
+++ b/toolkit/components/places/PageIconProtocolHandler.js
@@ -87,17 +87,17 @@ PageIconProtocolHandler.prototype = {
       // Create our channel.
       let channel = Cc["@mozilla.org/network/input-stream-channel;1"]
                       .createInstance(Ci.nsIInputStreamChannel);
       channel.QueryInterface(Ci.nsIChannel);
       channel.setURI(uri);
       channel.contentStream = pipe.inputStream;
       channel.loadInfo = loadInfo;
 
-      let pageURI = NetUtil.newURI(uri.cloneIgnoringRef().path);
+      let pageURI = NetUtil.newURI(uri.path.replace(/[&#]size=[^&]+$/, ""));
       let preferredSize = PlacesUtils.favicons.preferredSizeFromURI(uri);
       PlacesUtils.favicons.getFaviconDataForPage(pageURI, (iconURI, len, data, mimeType) => {
         channel.contentType = mimeType;
         if (len == 0) {
           streamDefaultFavicon(uri, loadInfo, pipe.outputStream);
         } else {
           try {
             serveIcon(pipe, data, len);
--- a/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js
+++ b/toolkit/components/places/tests/favicons/test_favicons_protocols_ref.js
@@ -42,19 +42,33 @@ add_task(function* () {
                         PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON16_URL)),
                         "Size=1 should return the 16px icon");
   yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 0),
                         PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
                         "Size=0 should return the bigger icon");
   yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, -1),
                         PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
                         "Invalid size should return the bigger icon");
+
+  // Ass the icon also for the page with ref.
+  yield PlacesTestUtils.addVisits(PAGE_URL + "#other§=12");
+  yield setFaviconForPage(PAGE_URL + "#other§=12", ICON16_URL, false);
+  yield setFaviconForPage(PAGE_URL + "#other§=12", ICON32_URL, false);
+  yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#other§=12", 16),
+                        PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON16_URL)),
+                        "Pre-existing refs should be retained");
   yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#other§=12", 32),
                         PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
-                        "Pre-existing refs should be ignored");
+                        "Pre-existing refs should be retained");
+
+  // If the ref-ed url is unknown, should still try to fetch icon for the unref-ed url.
+  yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL + "#randomstuff", 32),
+                        PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
+                        "Non-existing refs should be ignored");
+
   win = { devicePixelRatio: 1.1 };
   yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 16),
                         PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
                         "Size=16 with HIDPI should return the 32px icon");
   yield compareFavicons(PlacesUtils.urlWithSizeRef(win, PAGE_ICON_URL, 32),
                         PlacesUtils.favicons.getFaviconLinkForIcon(NetUtil.newURI(ICON32_URL)),
                         "Size=32 with HIDPI should return the 32px icon");
 });
--- a/toolkit/components/places/tests/favicons/test_page-icon_protocol.js
+++ b/toolkit/components/places/tests/favicons/test_page-icon_protocol.js
@@ -77,8 +77,21 @@ add_task(function* svg_icon() {
     faviconURI, SMALLSVG_DATA_URI.spec, 0, Services.scriptSecurityManager.getSystemPrincipal());
   yield setFaviconForPage(TEST_URI, faviconURI);
   let svgIcon = yield fetchIconForSpec(SMALLSVG_DATA_URI.spec);
   do_print(svgIcon.contentType)
   let pageIcon = yield fetchIconForSpec("page-icon:" + TEST_URI.spec);
   Assert.equal(svgIcon.contentType, pageIcon.contentType);
   Assert.deepEqual(svgIcon.data, pageIcon.data, "Got the root favicon data");
 });
+
+add_task(function* page_with_ref() {
+  for (let url of ["http://places.test.ref/#myref",
+                   "http://places.test.ref/#!&b=16",
+                   "http://places.test.ref/#"]) {
+    yield PlacesTestUtils.addVisits(url);
+    yield setFaviconForPage(url, ICON_URI, false);
+    let {data, contentType} = yield fetchIconForSpec("page-icon:" + url);
+    Assert.equal(contentType, gFavicon.contentType);
+    Assert.deepEqual(data, gFavicon.data, "Got the favicon data");
+    yield PlacesUtils.history.remove(url);
+  }
+});