Bug 1323366 - Create new IconRequest to prevent ConcurrentModificationException, r?sebastian
--- 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.");
+ }
+ }
}