Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException. r=sebastian, a=gchang
authorJing-wei Wu <topwu.tw@gmail.com>
Mon, 20 Feb 2017 09:45:16 +0800
changeset 378835 316111b53777bdb0300954306fc4c0e866c5a0e9
parent 378834 3f81626a32601f8f4eccf452d3b83ed11cc4ce87
child 378836 7c9e6963c68e0df88b347ca9d171da5cea58faed
push id1419
push userjlund@mozilla.com
push dateMon, 10 Apr 2017 20:44:07 +0000
treeherdermozilla-release@5e6801b73ef6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssebastian, gchang
bugs1323366
milestone53.0a2
Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException. r=sebastian, a=gchang
mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequestBuilder.java
--- a/mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
+++ b/mobile/android/base/java/org/mozilla/gecko/icons/IconRequestBuilder.java
@@ -5,138 +5,150 @@
 
 package org.mozilla.gecko.icons;
 
 import android.content.Context;
 import android.support.annotation.CheckResult;
 
 import org.mozilla.gecko.GeckoAppShell;
 
+import java.util.TreeSet;
+
 import ch.boye.httpclientandroidlib.util.TextUtils;
 
 /**
  * Builder for creating a request to load an icon.
  */
 public class IconRequestBuilder {
-    private final IconRequest request;
+    private final IconRequest internal;
 
     /* package-private */ IconRequestBuilder(Context context) {
-        this(new IconRequest(context));
+        internal = new IconRequest(context);
     }
 
     /* package-private */ IconRequestBuilder(IconRequest request) {
-        this.request = request;
+        internal = request;
     }
 
     /**
      * Set the URL of the page for which the icon should be loaded.
      */
     @CheckResult
     public IconRequestBuilder pageUrl(String pageUrl) {
-        request.pageUrl = pageUrl;
+        internal.pageUrl = pageUrl;
         return this;
     }
 
     /**
      * Set whether this request is allowed to load icons from non http(s) URLs (e.g. the omni.ja).
      *
      * For example web content referencing internal URLs should not lead to us loading icons from
      * internal data structures like the omni.ja.
      */
     @CheckResult
     public IconRequestBuilder privileged(boolean privileged) {
-        request.privileged = privileged;
+        internal.privileged = privileged;
         return this;
     }
 
     /**
      * Add an icon descriptor describing the location and properties of an icon. All descriptors
      * will be ranked and tried in order of their rank. Executing the request will modify the list
      * of icons (filter or add additional descriptors).
      */
     @CheckResult
     public IconRequestBuilder icon(IconDescriptor descriptor) {
-        request.icons.add(descriptor);
+        internal.icons.add(descriptor);
         return this;
     }
 
     /**
      * Skip the network and do not load an icon from a network connection.
      */
     @CheckResult
     public IconRequestBuilder skipNetwork() {
-        request.skipNetwork = true;
+        internal.skipNetwork = true;
         return this;
     }
 
     /**
      * If shouldSkipNetwork is true then do not load icon from a network connection.
      */
     @CheckResult
     public IconRequestBuilder skipNetworkIf(boolean shouldSkipNetwork) {
-        request.skipNetwork = shouldSkipNetwork;
+        internal.skipNetwork = shouldSkipNetwork;
         return this;
     }
 
     /**
      * Skip the disk cache and do not load an icon from disk.
      */
     @CheckResult
     public IconRequestBuilder skipDisk() {
-        request.skipDisk = true;
+        internal.skipDisk = true;
         return this;
     }
 
     /**
      * Skip the memory cache and do not return a previously loaded icon.
      */
     @CheckResult
     public IconRequestBuilder skipMemory() {
-        request.skipMemory = true;
+        internal.skipMemory = true;
         return this;
     }
 
     /**
      * The icon will be used as (Android) launcher icon. The loaded icon will be scaled to the
      * preferred Android launcher icon size.
      */
     public IconRequestBuilder forLauncherIcon() {
-        request.targetSize = GeckoAppShell.getPreferredIconSize();
+        internal.targetSize = GeckoAppShell.getPreferredIconSize();
         return this;
     }
 
     /**
      * Execute the callback on the background thread. By default the callback is always executed on
      * the UI thread in order to add the loaded icon to a view easily.
      */
     @CheckResult
     public IconRequestBuilder executeCallbackOnBackgroundThread() {
-        request.backgroundThread = true;
+        internal.backgroundThread = true;
         return this;
     }
 
     /**
      * When executing the request then only prepare executing it but do not actually load an icon.
      * This mode is only used for some legacy code that uses the icon URL and therefore needs to
      * perform a lookup of the URL but doesn't want to load the icon yet.
      */
     public IconRequestBuilder prepareOnly() {
-        request.prepareOnly = true;
+        internal.prepareOnly = true;
         return this;
     }
 
     /**
      * Return the request built with this builder.
      */
     @CheckResult
     public IconRequest build() {
-        if (TextUtils.isEmpty(request.pageUrl)) {
+        if (TextUtils.isEmpty(internal.pageUrl)) {
             throw new IllegalStateException("Page URL is required");
         }
 
+        IconRequest request = new IconRequest(internal.getContext());
+        request.pageUrl = internal.pageUrl;
+        request.privileged = internal.privileged;
+        request.icons = new TreeSet<>(internal.icons);
+        request.skipNetwork = internal.skipNetwork;
+        request.backgroundThread = internal.backgroundThread;
+        request.skipDisk = internal.skipDisk;
+        request.skipMemory = internal.skipMemory;
+        request.targetSize = internal.targetSize;
+        request.prepareOnly = internal.prepareOnly;
         return request;
     }
 
     /**
      * This is a no-op method.
      *
      * All builder methods are annotated with @CheckResult to denote that the
      * methods return the builder object and that it is typically an error to not call another method
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequestBuilder.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/icons/TestIconRequestBuilder.java
@@ -6,16 +6,21 @@ package org.mozilla.gecko.icons;
 import org.junit.Assert;
 
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mozilla.gecko.GeckoAppShell;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 import org.robolectric.RuntimeEnvironment;
 
+import java.util.ConcurrentModificationException;
+import java.util.Iterator;
+
+import static org.junit.Assert.fail;
+
 @RunWith(TestRunner.class)
 public class TestIconRequestBuilder {
     private static final String TEST_PAGE_URL_1 = "http://www.mozilla.org";
     private static final String TEST_PAGE_URL_2 = "http://www.example.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
@@ -172,9 +177,43 @@ public class TestIconRequestBuilder {
         Assert.assertEquals(32, request.getTargetSize());
 
         request.modify()
                 .forLauncherIcon()
                 .deferBuild();
 
         Assert.assertEquals(48, request.getTargetSize());
     }
+
+    @Test
+    public void testConcurrentAccess() {
+        IconRequestBuilder builder = Icons.with(RuntimeEnvironment.application)
+                .pageUrl(TEST_PAGE_URL_1)
+                .icon(IconDescriptor.createGenericIcon(TEST_ICON_URL_1))
+                .icon(IconDescriptor.createGenericIcon(TEST_ICON_URL_2));
+
+        // Call build() twice on a builder and verify that the two objects are not the same
+        IconRequest request = builder.build();
+        IconRequest compare = builder.build();
+        Assert.assertNotSame(request, compare);
+        Assert.assertNotSame(request.icons, compare.icons);
+
+        // After building call methods on the builder and verify that the previously build object is not changed
+        int iconCount = request.getIconCount();
+        builder.icon(IconDescriptor.createGenericIcon(TEST_PAGE_URL_2))
+                .deferBuild();
+        int iconCountAfterBuild = request.getIconCount();
+        Assert.assertEquals(iconCount, iconCountAfterBuild);
+
+        // Iterate the TreeSet and call methods on the builder
+        try {
+            final Iterator<IconDescriptor> iterator = request.icons.iterator();
+            while (iterator.hasNext()) {
+                iterator.next();
+                builder.icon(IconDescriptor.createGenericIcon(TEST_PAGE_URL_2))
+                        .deferBuild();
+            }
+        } catch (Exception e) {
+            e.printStackTrace();
+            fail("Got exception.");
+        }
+    }
 }