Bug 1331808 - IconRequest.moveToNextIcon(): Throw exception if removing current item failed. r=Grisha
authorSebastian Kaspari <s.kaspari@gmail.com>
Wed, 18 Jan 2017 15:06:44 +0100
changeset 375022 bc9d8f30b7f0a765835efc764afc1fccbb638373
parent 375021 5c43a389e735d16940d2f36b6f167249285c51dc
child 375023 f5fdf8bb9729298c05cea82e9cebd90ed9d231ec
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 - IconRequest.moveToNextIcon(): Throw exception if removing current item failed. r=Grisha MozReview-Commit-ID: 2dfgG7N9bX6
mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequest.java
--- a/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/IconRequest.java
@@ -159,17 +159,22 @@ public class IconRequest {
     }
 
     /**
      * Move to the next icon. This method is called after all loaders for the current best icon
      * have failed. After calling this method getBestIcon() will return the next icon to try.
      * hasIconDescriptors() should be called before requesting the next icon.
      */
     /* package-private */ void moveToNextIcon() {
-        icons.remove(getBestIcon());
+        if (!icons.remove(getBestIcon())) {
+            // Calling this method when there's no next icon is an error (use hasIconDescriptors()).
+            // Theoretically this method can fail even if there's a next icon (like it did in bug 1331808).
+            // In this case crashing to see and fix the issue is desired.
+            throw new IllegalStateException("Moving to next icon failed. Could not remove first icon from set.");
+        }
     }
 
     /**
      * Should this request be prepared but not actually load an icon?
      */
     /* package-private */ boolean shouldPrepareOnly() {
         return prepareOnly;
     }
--- 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
@@ -5,16 +5,23 @@ package org.mozilla.gecko.icons;
 
 import junit.framework.Assert;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 import org.robolectric.RuntimeEnvironment;
 
+import java.util.TreeSet;
+
+import static org.hamcrest.Matchers.any;
+import static org.mockito.Matchers.anyObject;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+
 @RunWith(TestRunner.class)
 public class TestIconRequest {
     private static final String TEST_PAGE_URL = "http://www.mozilla.org";
     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";
 
     @Test
     public void testIconHandling() {
@@ -48,9 +55,27 @@ public class TestIconRequest {
 
         Assert.assertEquals(TEST_ICON_URL_1, request.getBestIcon().getUrl());
 
         request.moveToNextIcon();
 
         Assert.assertEquals(0, request.getIconCount());
         Assert.assertFalse(request.hasIconDescriptors());
     }
+
+    /**
+     * If removing an icon from the internal set failed then we want to throw an exception.
+     */
+    @Test(expected = IllegalStateException.class)
+    public void testMoveToNextIconThrowsException() {
+        final IconRequest request = Icons.with(RuntimeEnvironment.application)
+                .pageUrl(TEST_PAGE_URL)
+                .build();
+
+        //noinspection unchecked - Creating a mock of a generic type
+        request.icons = (TreeSet<IconDescriptor>) mock(TreeSet.class);
+
+        //noinspection SuspiciousMethodCalls
+        doReturn(false).when(request.icons).remove(anyObject());
+
+        request.moveToNextIcon();
+    }
 }