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

--HG--
extra : rebase_source : f8918c022188401e21a03ac666628cff3e87f317

mobile/android/base/java/org/mozilla/gecko/icons/IconDescriptorComparator.java

@@ -16,7 +16,7 @@ import java.util.Comparator;
  */
 /* 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.
@@ -43,8 +43,10 @@ import java.util.Comparator;
             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) {

mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconDescriptorComparator.java

@@ -9,6 +9,8 @@ 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";
@@ -115,4 +117,36 @@ public class TestIconDescriptorComparator {
         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());
+        }
+    }
 }

mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequest.java

@@ -39,14 +39,14 @@ public class TestIconRequest {
         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();