Ensure all ProxyAutoBeans not directly referenced from root object or via a shim can be garbage-collected.
Issue 6193.
Patch by: bobv
Suggested by: t.broyer
Review by: rjrjr
Review at http://gwt-code-reviews.appspot.com/1451819
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@10344 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/core/client/impl/WeakMapping.java b/user/src/com/google/gwt/core/client/impl/WeakMapping.java
index 435816c..5a7f345 100644
--- a/user/src/com/google/gwt/core/client/impl/WeakMapping.java
+++ b/user/src/com/google/gwt/core/client/impl/WeakMapping.java
@@ -18,8 +18,10 @@
import java.lang.ref.Reference;
import java.lang.ref.ReferenceQueue;
import java.lang.ref.WeakReference;
-import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
/**
* A class associating a (String, Object) map with arbitrary source objects
@@ -88,12 +90,25 @@
}
}
+ private static class ManagedWeakReference<T> extends WeakReference<T> {
+ public ManagedWeakReference(T object) {
+ super(object);
+ }
+ }
+
+ /**
+ * Provides synchronization around {@link #queue} so at most one thread is
+ * cleaning at any given time.
+ */
+ private static final Lock cleanupLock = new ReentrantLock();
+
/**
* A Map from Objects to <String,Object> maps. Hashing is based on object
* identity. Weak references are used to allow otherwise unreferenced Objects
* to be garbage collected.
*/
- private static Map<IdentityWeakReference, Map<String, Object>> map = new HashMap<IdentityWeakReference, Map<String, Object>>();
+ private static final ConcurrentHashMap<IdentityWeakReference, Map<String, Object>> map =
+ new ConcurrentHashMap<IdentityWeakReference, Map<String, Object>>();
/**
* A ReferenceQueue used to clean up the map as its keys are
@@ -109,15 +124,18 @@
* @param key a String key.
* @return an Object associated with that key on the given instance, or null.
*/
- public static synchronized Object get(Object instance, String key) {
+ public static Object get(Object instance, String key) {
cleanup();
-
Object ref = new IdentityWeakReference(instance, queue);
Map<String, Object> m = map.get(ref);
if (m == null) {
return null;
}
- return m.get(key);
+ Object toReturn = m.get(key);
+ if (toReturn instanceof ManagedWeakReference) {
+ toReturn = ((ManagedWeakReference<?>) toReturn).get();
+ }
+ return toReturn;
}
/**
@@ -128,16 +146,15 @@
* <p>
* Due to restrictions of the Production Mode implementation, the instance
* argument must not be a String.
- *
+ *
* @param instance the source Object, which must not be a String.
* @param key a String key.
* @param value the Object to associate with the key on the given source
* Object.
* @throws IllegalArgumentException if instance is a String.
*/
- public static synchronized void set(Object instance, String key, Object value) {
+ public static void set(Object instance, String key, Object value) {
cleanup();
-
if (instance instanceof String) {
throw new IllegalArgumentException("Cannot use Strings with WeakMapping");
}
@@ -145,10 +162,23 @@
IdentityWeakReference ref = new IdentityWeakReference(instance, queue);
Map<String, Object> m = map.get(ref);
if (m == null) {
- m = new HashMap<String, Object>();
- map.put(ref, m);
+ m = new ConcurrentHashMap<String, Object>();
+ map.putIfAbsent(ref, m);
+ m = map.get(ref);
}
- m.put(key, value);
+ if (value == null) {
+ m.remove(key);
+ } else {
+ m.put(key, value);
+ }
+ }
+
+ /**
+ * Like {@link #set(Object, String, Object)}, but doesn't guarantee that
+ * {@code value} can be retrieved.
+ */
+ public static void setWeak(Object instance, String key, Object value) {
+ set(instance, key, new ManagedWeakReference<Object>(value));
}
/**
@@ -157,13 +187,16 @@
* will be eligible for future garbage collection.
*/
private static void cleanup() {
- Reference<? extends Object> ref;
- while ((ref = queue.poll()) != null) {
- /**
- * Note that we can still remove ref from the map even though its referent
- * has been nulled out since we only need == equality to do so.
- */
- map.remove(ref);
+ if (cleanupLock.tryLock()) {
+ Reference<? extends Object> ref;
+ while ((ref = queue.poll()) != null) {
+ /**
+ * Note that we can still remove ref from the map even though its
+ * referent has been nulled out since we only need == equality to do so.
+ */
+ map.remove(ref);
+ }
+ cleanupLock.unlock();
}
}
}
diff --git a/user/src/com/google/web/bindery/autobean/shared/impl/AbstractAutoBean.java b/user/src/com/google/web/bindery/autobean/shared/impl/AbstractAutoBean.java
index 4e801d9..bc7a599 100644
--- a/user/src/com/google/web/bindery/autobean/shared/impl/AbstractAutoBean.java
+++ b/user/src/com/google/web/bindery/autobean/shared/impl/AbstractAutoBean.java
@@ -15,6 +15,7 @@
*/
package com.google.web.bindery.autobean.shared.impl;
+import com.google.gwt.core.client.impl.WeakMapping;
import com.google.web.bindery.autobean.shared.AutoBean;
import com.google.web.bindery.autobean.shared.AutoBeanFactory;
import com.google.web.bindery.autobean.shared.AutoBeanUtils;
@@ -23,7 +24,6 @@
import com.google.web.bindery.autobean.shared.Splittable;
import com.google.web.bindery.autobean.shared.impl.AutoBeanCodexImpl.Coder;
import com.google.web.bindery.autobean.shared.impl.AutoBeanCodexImpl.EncodeState;
-import com.google.gwt.core.client.impl.WeakMapping;
import java.util.HashMap;
import java.util.HashSet;
@@ -94,7 +94,7 @@
this.wrapped = wrapped;
// Used by AutoBeanUtils
- WeakMapping.set(wrapped, AutoBean.class.getName(), this);
+ WeakMapping.setWeak(wrapped, AutoBean.class.getName(), this);
}
public void accept(AutoBeanVisitor visitor) {
diff --git a/user/src/com/google/web/bindery/autobean/vm/impl/JsonSplittable.java b/user/src/com/google/web/bindery/autobean/vm/impl/JsonSplittable.java
index 4ff8d07..913b50d 100644
--- a/user/src/com/google/web/bindery/autobean/vm/impl/JsonSplittable.java
+++ b/user/src/com/google/web/bindery/autobean/vm/impl/JsonSplittable.java
@@ -306,10 +306,10 @@
if (seen == null) {
if (object instanceof JSONObject) {
seen = new JsonSplittable((JSONObject) object);
- WeakMapping.set(object, JsonSplittable.class.getName(), seen);
+ WeakMapping.setWeak(object, JsonSplittable.class.getName(), seen);
} else if (object instanceof JSONArray) {
seen = new JsonSplittable((JSONArray) object);
- WeakMapping.set(object, JsonSplittable.class.getName(), seen);
+ WeakMapping.setWeak(object, JsonSplittable.class.getName(), seen);
} else if (object instanceof String) {
seen = new JsonSplittable(object.toString());
} else if (object instanceof Number) {
diff --git a/user/src/com/google/web/bindery/autobean/vm/impl/ProxyAutoBean.java b/user/src/com/google/web/bindery/autobean/vm/impl/ProxyAutoBean.java
index d52579d..4c37d1f 100644
--- a/user/src/com/google/web/bindery/autobean/vm/impl/ProxyAutoBean.java
+++ b/user/src/com/google/web/bindery/autobean/vm/impl/ProxyAutoBean.java
@@ -124,16 +124,23 @@
* ok if it's deallocated, since it has no interesting state.
*
* <pre>
- *
- * ----------------- --------------
- * | ProxyAutoBean | | Shim |
- * | | <------+-bean |
- * | shim-+---X--->|____________|
- * |_______________| ^
- * ^ |
- * | X
- * |______ WeakMapping ___|
+ * _________________ ______________
+ * | ProxyAutoBean | | Shim |
+ * | | <----------+-bean |
+ * | shim-+---X------> | |
+ * |_______________| |____________|
+ * ^ ^ ^
+ * X X |
+ * |__value__WeakMapping__key__| |
+ * ^ |
+ * | |
+ * GC Roots -> Owner____|
* </pre>
+ * <p>
+ * In the case of a wrapped object (for example, an ArrayList), the weak
+ * reference from WeakMapping to the ProxyAutoBean may cause the AutoBean to
+ * be prematurely collected if neither the bean nor the shim are referenced
+ * elsewhere. The alternative is a massive memory leak.
*/
private WeakReference<T> shim;
@@ -332,7 +339,7 @@
private T createShim() {
T toReturn = ProxyAutoBean.makeProxy(beanType, new ShimHandler<T>(this, getWrapped()));
- WeakMapping.set(toReturn, AutoBean.class.getName(), this);
+ WeakMapping.setWeak(toReturn, AutoBean.class.getName(), this);
return toReturn;
}
}
diff --git a/user/super/com/google/gwt/core/translatable/com/google/gwt/core/client/impl/WeakMapping.java b/user/super/com/google/gwt/core/translatable/com/google/gwt/core/client/impl/WeakMapping.java
index 35500be..ee701d0 100644
--- a/user/super/com/google/gwt/core/translatable/com/google/gwt/core/client/impl/WeakMapping.java
+++ b/user/super/com/google/gwt/core/translatable/com/google/gwt/core/client/impl/WeakMapping.java
@@ -59,6 +59,10 @@
setNative(instance, key, value);
}
+ public static void setWeak(Object instance, String key, Object value) {
+ set(instance, key, value);
+ }
+
private static native void setNative(Object instance, String key, Object value) /*-{
if (!instance.@java.lang.Object::expando) {
instance.@java.lang.Object::expando = {};
