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 = {};