Bug 1331808 - IconDescriptorComparator: Return consistent order for compared items. r=Grisha
authorSebastian Kaspari <s.kaspari@gmail.com>
Wed, 18 Jan 2017 14:50:35 +0100
changeset 375021 5c43a389e735d16940d2f36b6f167249285c51dc
parent 375020 b69accee20b4a38ba050768729f86f8d21ee0d3f
child 375022 bc9d8f30b7f0a765835efc764afc1fccbb638373
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGrisha
bugs1331808
milestone53.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 1331808 - IconDescriptorComparator: Return consistent order for compared items. r=Grisha Previously if we could not compare two icon descriptions we'd always return the "right" one. This does not create a consistent order if the parameters are flipped. As a result some operations on a TreeSet can fail (like remove()). MozReview-Commit-ID: EYPlhzGUEnD
mobile/android/base/java/org/mozilla/gecko/icons/IconDescriptorComparator.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconDescriptorComparator.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequest.java
--- a/mobile/android/base/java/org/mozilla/gecko/icons/IconDescriptorComparator.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/IconDescriptorComparator.java
@@ -11,17 +11,17 @@ import java.util.Comparator;
  * This comparator implementation compares IconDescriptor objects in order to determine which icon
  * to load first.
  *
  * In general this comparator will try touch icons before favicons (they usually have a higher resolution)
  * and prefers larger icons over smaller ones.
  */
 /* package-private */ class IconDescriptorComparator implements Comparator<IconDescriptor> {
     @Override
-    public int compare(IconDescriptor lhs, IconDescriptor rhs) {
+    public int compare(final IconDescriptor lhs, final IconDescriptor rhs) {
         if (lhs.getUrl().equals(rhs.getUrl())) {
             // Two descriptors pointing to the same URL are always referencing the same icon. So treat
             // them as equal.
             return 0;
         }
 
         // First compare the types. We prefer touch icons because they tend to have a higher resolution
         // than ordinary favicons.
@@ -38,18 +38,20 @@ import java.util.Comparator;
         // an image larger than the size given in the <link> tag.
         final boolean lhsContainer = IconsHelper.isContainerType(lhs.getMimeType());
         final boolean rhsContainer = IconsHelper.isContainerType(rhs.getMimeType());
 
         if (lhsContainer != rhsContainer) {
             return lhsContainer ? -1 : 1;
         }
 
-        // There's no way to know which icon might be better. Just pick rhs.
-        return 1;
+        // There's no way to know which icon might be better. However we need to pick a consistent
+        // one to avoid breaking the TreeSet implementation (See Bug 1331808). Therefore we are
+        // picking one by just comparing the URLs.
+        return lhs.getUrl().compareTo(rhs.getUrl());
     }
 
     private int compareType(IconDescriptor lhs, IconDescriptor rhs) {
         if (lhs.getType() > rhs.getType()) {
             return -1;
         } else {
             return 1;
         }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconDescriptorComparator.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconDescriptorComparator.java
@@ -4,16 +4,18 @@
 package org.mozilla.gecko.icons;
 
 import org.junit.Assert;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 
+import java.util.TreeSet;
+
 @RunWith(TestRunner.class)
 public class TestIconDescriptorComparator {
     private static final String TEST_ICON_URL_1 = "http://www.mozilla.org/favicon.ico";
     private static final String TEST_ICON_URL_2 = "http://www.example.org/favicon.ico";
     private static final String TEST_ICON_URL_3 = "http://www.example.com/favicon.ico";
 
     private static final String TEST_MIME_TYPE = "image/png";
     private static final int TEST_SIZE = 32;
@@ -110,9 +112,41 @@ public class TestIconDescriptorComparato
         final IconDescriptor descriptor1 = IconDescriptor.createFavicon(TEST_ICON_URL_1, TEST_SIZE, TEST_MIME_TYPE);
         final IconDescriptor descriptor2 = IconDescriptor.createFavicon(TEST_ICON_URL_2, TEST_SIZE, TEST_MIME_TYPE);
 
         final IconDescriptorComparator comparator = new IconDescriptorComparator();
 
         Assert.assertNotEquals(0, comparator.compare(descriptor1, descriptor2));
         Assert.assertNotEquals(0, comparator.compare(descriptor2, descriptor1));
     }
+
+    @Test
+    public void testWithSameObject() {
+        final IconDescriptor descriptor = IconDescriptor.createTouchicon(TEST_ICON_URL_1, TEST_SIZE, TEST_MIME_TYPE);
+
+        final IconDescriptorComparator comparator = new IconDescriptorComparator();
+        Assert.assertEquals(0, comparator.compare(descriptor, descriptor));
+    }
+
+    /**
+     * This test reconstructs the scenario from bug 1331808. A comparator implementation that does
+     * not return a consistent order can break the implementation of remove() of the TreeSet class.
+     */
+    @Test
+    public void testBug1331808() {
+        TreeSet<IconDescriptor> set = new TreeSet<>(new IconDescriptorComparator());
+
+        set.add(IconDescriptor.createFavicon("http://example.org/new-logo32.jpg", 0, ""));
+        set.add(IconDescriptor.createTouchicon("http://example.org/new-logo57.jpg", 0, ""));
+        set.add(IconDescriptor.createTouchicon("http://example.org/new-logo76.jpg", 76, ""));
+        set.add(IconDescriptor.createTouchicon("http://example.org/new-logo120.jpg", 120, ""));
+        set.add(IconDescriptor.createTouchicon("http://example.org/new-logo152.jpg", 114, ""));
+        set.add(IconDescriptor.createFavicon("http://example.org/02.png", 32, ""));
+        set.add(IconDescriptor.createFavicon("http://example.org/01.png", 192, ""));
+        set.add(IconDescriptor.createTouchicon("http://example.org/03.png", 0, ""));
+
+        for (int i = 8; i > 0; i--) {
+            Assert.assertEquals("items in set before deleting: " + i, i, set.size());
+            Assert.assertTrue("item removed successfully: " + i, set.remove(set.first()));
+            Assert.assertEquals("items in set after deleting: " + i, i - 1, set.size());
+        }
+    }
 }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequest.java
@@ -34,23 +34,23 @@ public class TestIconRequest {
 
         request.modify()
                 .icon(IconDescriptor.createGenericIcon(TEST_ICON_URL_2))
                 .deferBuild();
 
         Assert.assertEquals(2, request.getIconCount());
         Assert.assertTrue(request.hasIconDescriptors());
 
-        Assert.assertEquals(TEST_ICON_URL_1, request.getBestIcon().getUrl());
+        Assert.assertEquals(TEST_ICON_URL_2, request.getBestIcon().getUrl());
 
         request.moveToNextIcon();
 
         Assert.assertEquals(1, request.getIconCount());
         Assert.assertTrue(request.hasIconDescriptors());
 
-        Assert.assertEquals(TEST_ICON_URL_2, request.getBestIcon().getUrl());
+        Assert.assertEquals(TEST_ICON_URL_1, request.getBestIcon().getUrl());
 
         request.moveToNextIcon();
 
         Assert.assertEquals(0, request.getIconCount());
         Assert.assertFalse(request.hasIconDescriptors());
     }
 }