Some work on issue 910 -- ensure that anonymous keys & typeliterals don't

retain references to their parent classes.  Still some more work to do in
WeakKeySet to let it clean up more frequently, but this should help for now.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=89328452

10 files changed, 137 insertions, 62 deletions

diff --git a/core/src/com/google/inject/internal/InjectorImpl.java b/core/src/com/google/inject/internal/InjectorImpl.java
index d1d028f1..3d868eba 100644
--- a/core/src/com/google/inject/internal/InjectorImpl.java
+++ b/core/src/com/google/inject/internal/InjectorImpl.java
@@ -801,6 +801,7 @@ final class InjectorImpl implements Injector, Lookups {
       throw errors.childBindingAlreadySet(key, sources).toException();
     }
 
+    key = MoreTypes.canonicalizeKey(key); // before storing the key long-term, canonicalize it.
     BindingImpl<T> binding = createJustInTimeBinding(key, errors, jitDisabled, jitType);
     state.parent().blacklist(key, state, binding.getSource());
     jitBindings.put(key, binding);
diff --git a/core/src/com/google/inject/internal/MoreTypes.java b/core/src/com/google/inject/internal/MoreTypes.java
index 12a76250..bdf6029a 100644
--- a/core/src/com/google/inject/internal/MoreTypes.java
+++ b/core/src/com/google/inject/internal/MoreTypes.java
@@ -23,6 +23,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableMap;
 import com.google.inject.ConfigurationException;
+import com.google.inject.Key;
 import com.google.inject.TypeLiteral;
 import com.google.inject.util.Types;
 
@@ -64,6 +65,25 @@ public class MoreTypes {
           .build();
 
   /**
+   * Returns a key that doesn't hold any references to parent classes.
+   * This is necessary for anonymous keys, so ensure we don't hold a ref
+   * to the containing module (or class) forever.
+   */
+  public static <T> Key<T> canonicalizeKey(Key<T> key) {
+    // If we know this isn't a subclass, return as-is.
+    // Otherwise, recreate the key to avoid the subclass 
+    if (key.getClass() == Key.class) {
+      return key;
+    } else if (key.getAnnotation() != null) {
+      return Key.get(key.getTypeLiteral(), key.getAnnotation());
+    } else if (key.getAnnotationType() != null) {
+      return Key.get(key.getTypeLiteral(), key.getAnnotationType());
+    } else {
+      return Key.get(key.getTypeLiteral());
+    }
+  }
+
+  /**
    * Returns an type that's appropriate for use in a key.
    *
    * <p>If the raw type of {@code typeLiteral} is a {@code javax.inject.Provider}, this returns a
@@ -93,9 +113,20 @@ public class MoreTypes {
 
     @SuppressWarnings("unchecked")
     TypeLiteral<T> wrappedPrimitives = (TypeLiteral<T>) PRIMITIVE_TO_WRAPPER.get(typeLiteral);
-    return wrappedPrimitives != null
-        ? wrappedPrimitives
-        : typeLiteral;
+    if (wrappedPrimitives != null) {
+      return wrappedPrimitives;
+    }
+
+    // If we know this isn't a subclass, return as-is.
+    if (typeLiteral.getClass() == TypeLiteral.class) {
+      return typeLiteral;
+    }
+
+    // recreate the TypeLiteral to avoid anonymous TypeLiterals from holding refs to their
+    // surrounding classes.
+    @SuppressWarnings("unchecked")
+    TypeLiteral<T> recreated = (TypeLiteral<T>) TypeLiteral.get(typeLiteral.getType());
+    return recreated;
   }
 
   /**
diff --git a/core/src/com/google/inject/spi/Dependency.java b/core/src/com/google/inject/spi/Dependency.java
index c51d87c2..f86e2559 100644
--- a/core/src/com/google/inject/spi/Dependency.java
+++ b/core/src/com/google/inject/spi/Dependency.java
@@ -22,6 +22,7 @@ import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
 import com.google.inject.Key;
+import com.google.inject.internal.MoreTypes;
 
 import java.util.List;
 import java.util.Set;
@@ -54,7 +55,7 @@ public final class Dependency<T> {
    * nullable.
    */
   public static <T> Dependency<T> get(Key<T> key) {
-    return new Dependency<T>(null, key, true, -1);
+    return new Dependency<T>(null, MoreTypes.canonicalizeKey(key), true, -1);
   }
 
   /**
diff --git a/core/src/com/google/inject/spi/Elements.java b/core/src/com/google/inject/spi/Elements.java
index f5bbe894..9348276b 100644
--- a/core/src/com/google/inject/spi/Elements.java
+++ b/core/src/com/google/inject/spi/Elements.java
@@ -44,6 +44,7 @@ import com.google.inject.internal.ConstantBindingBuilderImpl;
 import com.google.inject.internal.Errors;
 import com.google.inject.internal.ExposureBuilder;
 import com.google.inject.internal.InternalFlags.IncludeStackTraceOption;
+import com.google.inject.internal.MoreTypes;
 import com.google.inject.internal.PrivateElementsImpl;
 import com.google.inject.internal.ProviderMethodsModule;
 import com.google.inject.internal.util.SourceProvider;
@@ -242,13 +243,14 @@ public final class Elements {
 
     @Override
     public <T> void requestInjection(TypeLiteral<T> type, T instance) {
-      elements.add(new InjectionRequest<T>(getElementSource(), type, instance));
+      elements.add(new InjectionRequest<T>(getElementSource(), MoreTypes.canonicalizeForKey(type),
+          instance));
     }
 
     @Override
     public <T> MembersInjector<T> getMembersInjector(final TypeLiteral<T> typeLiteral) {
-      final MembersInjectorLookup<T> element
-          = new MembersInjectorLookup<T>(getElementSource(), typeLiteral);
+      final MembersInjectorLookup<T> element = new MembersInjectorLookup<T>(getElementSource(),
+          MoreTypes.canonicalizeForKey(typeLiteral));
       elements.add(element);
       return element.getMembersInjector();
     }
@@ -370,7 +372,8 @@ public final class Elements {
     }
 
     public <T> AnnotatedBindingBuilder<T> bind(Key<T> key) {
-      BindingBuilder<T> builder = new BindingBuilder<T>(this, elements, getElementSource(), key);
+      BindingBuilder<T> builder =
+          new BindingBuilder<T>(this, elements, getElementSource(), MoreTypes.canonicalizeKey(key));
       return builder;
     }
 
@@ -480,7 +483,8 @@ public final class Elements {
         };
       }
 
-      ExposureBuilder<T> builder = new ExposureBuilder<T>(this, getElementSource(), key);
+      ExposureBuilder<T> builder =
+          new ExposureBuilder<T>(this, getElementSource(), MoreTypes.canonicalizeKey(key));
       privateElements.addExposureBuilder(builder);
       return builder;
     }
diff --git a/core/test/com/google/inject/Asserts.java b/core/test/com/google/inject/Asserts.java
index 363e1610..6c63158a 100644
--- a/core/test/com/google/inject/Asserts.java
+++ b/core/test/com/google/inject/Asserts.java
@@ -21,12 +21,14 @@ import static com.google.inject.internal.InternalFlags.IncludeStackTraceOption;
 import static com.google.inject.internal.InternalFlags.getIncludeStackTraceOption;
 import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertNotNull;
+import static junit.framework.Assert.assertSame;
 import static junit.framework.Assert.assertTrue;
 
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
+import com.google.common.testing.GcFinalization;
 
 import junit.framework.Assert;
 
@@ -36,6 +38,8 @@ import java.io.IOException;
 import java.io.NotSerializableException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
 
 /**
  * @author jessewilson@google.com (Jesse Wilson)
@@ -159,4 +163,42 @@ public class Asserts {
     } catch (NotSerializableException expected) {
     }
   }
+
+  public static void awaitFullGc() {
+    // GcFinalization *should* do it, but doesn't work well in practice...
+    // so we put a second latch and wait for a ReferenceQueue to tell us.
+    ReferenceQueue<Object> queue = new ReferenceQueue<Object>();
+    WeakReference ref = new WeakReference<Object>(new Object(), queue);
+    GcFinalization.awaitFullGc();
+    try {
+      assertSame("queue didn't return ref in time", ref, queue.remove(5000));
+    } catch (IllegalArgumentException e) {
+      throw new RuntimeException(e);
+    } catch (InterruptedException e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  public static void awaitClear(WeakReference<?> ref) {
+    // GcFinalization *should* do it, but doesn't work well in practice...
+    // so we put a second latch and wait for a ReferenceQueue to tell us.
+    Object data = ref.get();
+    ReferenceQueue<Object> queue = null;
+    WeakReference extraRef = null;
+    if (data != null) {
+      queue = new ReferenceQueue<Object>();
+      extraRef = new WeakReference<Object>(data, queue);
+      data = null;
+    }
+    GcFinalization.awaitClear(ref);
+    if (queue != null) {
+      try {
+        assertSame("queue didn't return ref in time", extraRef, queue.remove(5000));
+      } catch (IllegalArgumentException e) {
+        throw new RuntimeException(e);
+      } catch (InterruptedException e) {
+        throw new RuntimeException(e);
+      }
+    }
+  }
 }
diff --git a/core/test/com/google/inject/KeyTest.java b/core/test/com/google/inject/KeyTest.java
index c0401e06..d9dd943f 100644
--- a/core/test/com/google/inject/KeyTest.java
+++ b/core/test/com/google/inject/KeyTest.java
@@ -19,10 +19,12 @@ package com.google.inject;
 import static com.google.inject.Asserts.assertContains;
 import static com.google.inject.Asserts.assertEqualsBothWays;
 import static com.google.inject.Asserts.assertNotSerializable;
+import static com.google.inject.Asserts.awaitClear;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 
 import com.google.inject.name.Named;
 import com.google.inject.name.Names;
+import com.google.inject.spi.Dependency;
 import com.google.inject.util.Types;
 
 import junit.framework.TestCase;
@@ -31,12 +33,15 @@ import java.io.IOException;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
+import java.lang.ref.WeakReference;
 import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.lang.reflect.TypeVariable;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * @author crazybob@google.com (Bob Lee)
@@ -275,4 +280,42 @@ public class KeyTest extends TestCase {
   @Marker
   class HasAnnotations {}
 
+  public void testAnonymousClassesDontHoldRefs() {
+    final AtomicReference<Provider<List<String>>> stringProvider =
+        new AtomicReference<Provider<List<String>>>();
+    final AtomicReference<Provider<List<Integer>>> intProvider =
+        new AtomicReference<Provider<List<Integer>>>();
+    final Object foo = new Object() {
+      @SuppressWarnings("unused") @Inject List<String> list;
+    };
+    Module module = new AbstractModule() {
+      @Override protected void configure() {
+        bind(new Key<List<String>>() {}).toInstance(new ArrayList<String>());
+        bind(new TypeLiteral<List<Integer>>() {}).toInstance(new ArrayList<Integer>());
+
+        stringProvider.set(getProvider(new Key<List<String>>() {}));
+        intProvider.set(binder().getProvider(Dependency.get(new Key<List<Integer>>() {})));
+
+        binder().requestInjection(new TypeLiteral<Object>() {}, foo);
+      }
+    };
+    WeakReference<Module> moduleRef = new WeakReference<Module>(module);
+    final Injector injector = Guice.createInjector(module);
+    module = null;
+    awaitClear(moduleRef); // Make sure anonymous keys & typeliterals don't hold the module.
+
+    Runnable runner = new Runnable() {
+      @Override public void run() {
+        injector.getInstance(new Key<Typed<String>>() {});
+        injector.getInstance(Key.get(new TypeLiteral<Typed<Integer>>() {}));
+      }
+    };
+    WeakReference<Runnable> runnerRef = new WeakReference<Runnable>(runner);
+    runner.run();
+    runner = null;
+    awaitClear(runnerRef); // also make sure anonymous keys & typeliterals don't hold for JITs
+  }
+
+  static class Typed<T> {}
+
 }
diff --git a/core/test/com/google/inject/internal/WeakKeySetTest.java b/core/test/com/google/inject/internal/WeakKeySetTest.java
index 3797d889..4a81ebbe 100644
--- a/core/test/com/google/inject/internal/WeakKeySetTest.java
+++ b/core/test/com/google/inject/internal/WeakKeySetTest.java
@@ -16,13 +16,13 @@
 
 package com.google.inject.internal;
 
+import static com.google.inject.Asserts.awaitClear;
+import static com.google.inject.Asserts.awaitFullGc;
 import static com.google.inject.internal.WeakKeySetUtils.assertBlacklisted;
 import static com.google.inject.internal.WeakKeySetUtils.assertInSet;
 import static com.google.inject.internal.WeakKeySetUtils.assertNotBlacklisted;
 import static com.google.inject.internal.WeakKeySetUtils.assertNotInSet;
 import static com.google.inject.internal.WeakKeySetUtils.assertSourceNotInSet;
-import static com.google.inject.internal.WeakKeySetUtils.awaitClear;
-import static com.google.inject.internal.WeakKeySetUtils.awaitFullGc;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -34,13 +34,6 @@ import com.google.inject.Injector;
 import com.google.inject.Key;
 import com.google.inject.Scope;
 import com.google.inject.TypeLiteral;
-import com.google.inject.internal.BindingImpl;
-import com.google.inject.internal.Errors;
-/*if[AOP]*/
-import com.google.inject.internal.MethodAspect;
-/*end[AOP]*/
-import com.google.inject.internal.State;
-import com.google.inject.internal.WeakKeySet;
 import com.google.inject.spi.ModuleAnnotatedMethodScannerBinding;
 import com.google.inject.spi.ProvisionListenerBinding;
 import com.google.inject.spi.ScopeBinding;
diff --git a/core/test/com/google/inject/internal/WeakKeySetUtils.java b/core/test/com/google/inject/internal/WeakKeySetUtils.java
index bab9e925..b023aa1d 100644
--- a/core/test/com/google/inject/internal/WeakKeySetUtils.java
+++ b/core/test/com/google/inject/internal/WeakKeySetUtils.java
@@ -18,15 +18,11 @@ import static junit.framework.Assert.assertEquals;
 import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertNotNull;
 import static junit.framework.Assert.assertNull;
-import static junit.framework.Assert.assertSame;
 import static junit.framework.Assert.assertTrue;
 
-import com.google.common.testing.GcFinalization;
 import com.google.inject.Injector;
 import com.google.inject.Key;
 
-import java.lang.ref.ReferenceQueue;
-import java.lang.ref.WeakReference;
 import java.util.Set;
 
 /**
@@ -38,44 +34,6 @@ public final class WeakKeySetUtils {
 
   private WeakKeySetUtils() {}
 
-  public static void awaitFullGc() {
-    // GcFinalization *should* do it, but doesn't work well in practice...
-    // so we put a second latch and wait for a ReferenceQueue to tell us.
-    ReferenceQueue<Object> queue = new ReferenceQueue<Object>();
-    WeakReference ref = new WeakReference<Object>(new Object(), queue);
-    GcFinalization.awaitFullGc();
-    try {
-      assertSame("queue didn't return ref in time", ref, queue.remove(5000));
-    } catch (IllegalArgumentException e) {
-      throw new RuntimeException(e);
-    } catch (InterruptedException e) {
-      throw new RuntimeException(e);
-    }
-  }
-
-  public static void awaitClear(WeakReference<?> ref) {
-    // GcFinalization *should* do it, but doesn't work well in practice...
-    // so we put a second latch and wait for a ReferenceQueue to tell us.
-    Object data = ref.get();
-    ReferenceQueue<Object> queue = null;
-    WeakReference extraRef = null;
-    if (data != null) {
-      queue = new ReferenceQueue<Object>();
-      extraRef = new WeakReference<Object>(data, queue);
-      data = null;
-    }
-    GcFinalization.awaitClear(ref);
-    if (queue != null) {
-      try {
-        assertSame("queue didn't return ref in time", extraRef, queue.remove(5000));
-      } catch (IllegalArgumentException e) {
-        throw new RuntimeException(e);
-      } catch (InterruptedException e) {
-        throw new RuntimeException(e);
-      }
-    }
-  }
-
   public static void assertBlacklisted(Injector injector, Key<?> key) {
     assertBlacklistState(injector, key, true);
   }
diff --git a/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java b/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java
index 849993f5..4206521d 100644
--- a/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java
+++ b/extensions/multibindings/test/com/google/inject/multibindings/MapBinderTest.java
@@ -32,6 +32,7 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
 import com.google.inject.AbstractModule;
+import com.google.inject.Asserts;
 import com.google.inject.Binding;
 import com.google.inject.BindingAnnotation;
 import com.google.inject.ConfigurationException;
@@ -1025,7 +1026,7 @@ public class MapBinderTest extends TestCase {
     // Clear the ref, GC, and ensure that we are no longer blacklisting.
     childInjector = null;
     
-    WeakKeySetUtils.awaitClear(weakRef);
+    Asserts.awaitClear(weakRef);
     WeakKeySetUtils.assertNotBlacklisted(parentInjector, mapKey);
   }
 }
diff --git a/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java b/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java
index d0a239ac..f3c9f63f 100644
--- a/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java
+++ b/extensions/multibindings/test/com/google/inject/multibindings/OptionalBinderTest.java
@@ -30,6 +30,7 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.inject.AbstractModule;
+import com.google.inject.Asserts;
 import com.google.inject.Binding;
 import com.google.inject.BindingAnnotation;
 import com.google.inject.CreationException;
@@ -1203,7 +1204,7 @@ public class OptionalBinderTest extends TestCase {
    // Clear the ref, GC, and ensure that we are no longer blacklisting.
    childInjector = null;
    
-   WeakKeySetUtils.awaitClear(weakRef);
+   Asserts.awaitClear(weakRef);
    WeakKeySetUtils.assertNotBlacklisted(parentInjector, Key.get(Integer.class));
  }