Fix CGLIB memory leak for method injection

This commit continues the work for fixing memory leaks resulting from
CGLIB subclass generation for beans relying on method injection.

- Set proxy callbacks on the CGLIB Factory (i.e., the instance) instead
  of in the generated subclass (i.e., via the Enhancer).

- Convert private inner classes in CglibSubclassingInstantiationStrategy
  to private static classes in order to avoid unnecessary coupling to
  classes generated using CGLIB.

- Tidy up XmlBeanFactoryTests.

- Update logic in serializableMethodReplacerAndSuperclass() so that it
  finally aligns with the decision made for SPR-356.

Issue: SPR-10785, SPR-356

spring-beans/src/main/java/org/springframework/beans/factory/support/CglibSubclassingInstantiationStrategy.java

@@ -22,22 +22,27 @@ import java.lang.reflect.Method;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import org.springframework.beans.BeanInstantiationException;
+import org.springframework.beans.BeanUtils;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.cglib.core.SpringNamingPolicy;
 import org.springframework.cglib.proxy.Callback;
 import org.springframework.cglib.proxy.CallbackFilter;
 import org.springframework.cglib.proxy.Enhancer;
+import org.springframework.cglib.proxy.Factory;
 import org.springframework.cglib.proxy.MethodInterceptor;
 import org.springframework.cglib.proxy.MethodProxy;
 import org.springframework.cglib.proxy.NoOp;
 
 /**
  * Default object instantiation strategy for use in BeanFactories.
- * Uses CGLIB to generate subclasses dynamically if methods need to be
- * overridden by the container, to implement Method Injection.
+ *
+ * <p>Uses CGLIB to generate subclasses dynamically if methods need to be
+ * overridden by the container to implement <em>Method Injection</em>.
  *
  * @author Rod Johnson
  * @author Juergen Hoeller
+ * @author Sam Brannen
  * @since 1.1
  */
 public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationStrategy {
@@ -50,7 +55,7 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt
 
 	/**
 	 * Index in the CGLIB callback array for a method that should
-	 * be overridden to provide method lookup.
+	 * be overridden to provide <em>method lookup</em>.
 	 */
 	private static final int LOOKUP_OVERRIDE = 1;
 
@@ -62,18 +67,17 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt
 
 
 	@Override
-	protected Object instantiateWithMethodInjection(
-			RootBeanDefinition beanDefinition, String beanName, BeanFactory owner) {
+	protected Object instantiateWithMethodInjection(RootBeanDefinition beanDefinition, String beanName,
+			BeanFactory owner) {
 
-		// Must generate CGLIB subclass.
-		return new CglibSubclassCreator(beanDefinition, owner).instantiate(null, null);
+		return instantiateWithMethodInjection(beanDefinition, beanName, owner, null, null);
 	}
 
 	@Override
-	protected Object instantiateWithMethodInjection(
-			RootBeanDefinition beanDefinition, String beanName, BeanFactory owner,
-			Constructor<?> ctor, Object[] args) {
+	protected Object instantiateWithMethodInjection(RootBeanDefinition beanDefinition, String beanName,
+			BeanFactory owner, Constructor<?> ctor, Object[] args) {
 
+		// Must generate CGLIB subclass.
 		return new CglibSubclassCreator(beanDefinition, owner).instantiate(ctor, args);
 	}
 
@@ -84,13 +88,15 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt
 	 */
 	private static class CglibSubclassCreator {
 
-		private static final Log logger = LogFactory.getLog(CglibSubclassCreator.class);
+		private static final Class<?>[] CALLBACK_TYPES = new Class<?>[] { NoOp.class,
+			LookupOverrideMethodInterceptor.class, ReplaceOverrideMethodInterceptor.class };
 
 		private final RootBeanDefinition beanDefinition;
 
 		private final BeanFactory owner;
 
-		public CglibSubclassCreator(RootBeanDefinition beanDefinition, BeanFactory owner) {
+
+		CglibSubclassCreator(RootBeanDefinition beanDefinition, BeanFactory owner) {
 			this.beanDefinition = beanDefinition;
 			this.owner = owner;
 		}
@@ -101,105 +107,158 @@ public class CglibSubclassingInstantiationStrategy extends SimpleInstantiationSt
 		 * @param ctor constructor to use. If this is {@code null}, use the
 		 * no-arg constructor (no parameterization, or Setter Injection)
 		 * @param args arguments to use for the constructor.
-		 * Ignored if the ctor parameter is {@code null}.
+		 * Ignored if the {@code ctor} parameter is {@code null}.
 		 * @return new instance of the dynamically generated subclass
 		 */
-		public Object instantiate(Constructor<?> ctor, Object[] args) {
+		Object instantiate(Constructor<?> ctor, Object[] args) {
+			Class<?> subclass = createEnhancedSubclass(this.beanDefinition);
+
+			Object instance;
+			if (ctor == null) {
+				instance = BeanUtils.instantiate(subclass);
+			}
+			else {
+				try {
+					Constructor<?> enhancedSubclassConstructor = subclass.getConstructor(ctor.getParameterTypes());
+					instance = enhancedSubclassConstructor.newInstance(args);
+				}
+				catch (Exception e) {
+					throw new BeanInstantiationException(this.beanDefinition.getBeanClass(), String.format(
+						"Failed to invoke construcor for CGLIB enhanced subclass [%s]", subclass.getName()), e);
+				}
+			}
+
+			// SPR-10785: set callbacks directly on the instance instead of in the
+			// enhanced class (via the Enhancer) in order to avoid memory leaks.
+			Factory factory = (Factory) instance;
+			factory.setCallbacks(new Callback[] { NoOp.INSTANCE,//
+				new LookupOverrideMethodInterceptor(beanDefinition, owner),//
+				new ReplaceOverrideMethodInterceptor(beanDefinition, owner) });
+
+			return instance;
+		}
+
+		/**
+		 * Create an enhanced subclass of the bean class for the provided bean
+		 * definition, using CGLIB.
+		 */
+		private Class<?> createEnhancedSubclass(RootBeanDefinition beanDefinition) {
 			Enhancer enhancer = new Enhancer();
-			enhancer.setSuperclass(this.beanDefinition.getBeanClass());
+			enhancer.setSuperclass(beanDefinition.getBeanClass());
 			enhancer.setNamingPolicy(SpringNamingPolicy.INSTANCE);
-			enhancer.setCallbackFilter(new CallbackFilterImpl());
-			enhancer.setCallbacks(new Callback[] {
-					NoOp.INSTANCE,
-					new LookupOverrideMethodInterceptor(),
-					new ReplaceOverrideMethodInterceptor()
-			});
-
-			return (ctor != null ? enhancer.create(ctor.getParameterTypes(), args) : enhancer.create());
+			enhancer.setCallbackFilter(new CallbackFilterImpl(beanDefinition));
+			enhancer.setCallbackTypes(CALLBACK_TYPES);
+			return enhancer.createClass();
 		}
+	}
+
+	/**
+	 * Class providing hashCode and equals methods required by CGLIB to
+	 * ensure that CGLIB doesn't generate a distinct class per bean.
+	 * Identity is based on class and bean definition.
+	 */
+	private static class CglibIdentitySupport {
 
+		private final RootBeanDefinition beanDefinition;
+
+
+		CglibIdentitySupport(RootBeanDefinition beanDefinition) {
+			this.beanDefinition = beanDefinition;
+		}
 
 		/**
-		 * Class providing hashCode and equals methods required by CGLIB to
-		 * ensure that CGLIB doesn't generate a distinct class per bean.
-		 * Identity is based on class and bean definition.
+		 * Exposed for equals method to allow access to enclosing class field
 		 */
-		private class CglibIdentitySupport {
-
-			/**
-			 * Exposed for equals method to allow access to enclosing class field
-			 */
-			protected RootBeanDefinition getBeanDefinition() {
-				return beanDefinition;
-			}
+		protected RootBeanDefinition getBeanDefinition() {
+			return beanDefinition;
+		}
 
-			@Override
-			public boolean equals(Object other) {
-				return (other.getClass().equals(getClass()) &&
-						((CglibIdentitySupport) other).getBeanDefinition().equals(beanDefinition));
-			}
+		@Override
+		public boolean equals(Object other) {
+			return (other.getClass().equals(getClass()) && ((CglibIdentitySupport) other).getBeanDefinition().equals(
+				beanDefinition));
+		}
 
-			@Override
-			public int hashCode() {
-				return beanDefinition.hashCode();
-			}
+		@Override
+		public int hashCode() {
+			return beanDefinition.hashCode();
 		}
+	}
 
+	/**
+	 * CGLIB object to filter method interception behavior.
+	 */
+	private static class CallbackFilterImpl extends CglibIdentitySupport implements CallbackFilter {
+
+		private static final Log logger = LogFactory.getLog(CallbackFilterImpl.class);
 
-		/**
-		 * CGLIB MethodInterceptor to override methods, replacing them with an
-		 * implementation that returns a bean looked up in the container.
-		 */
-		private class LookupOverrideMethodInterceptor extends CglibIdentitySupport implements MethodInterceptor {
 
-			@Override
-			public Object intercept(Object obj, Method method, Object[] args, MethodProxy mp) throws Throwable {
-				// Cast is safe, as CallbackFilter filters are used selectively.
-				LookupOverride lo = (LookupOverride) beanDefinition.getMethodOverrides().getOverride(method);
-				return owner.getBean(lo.getBeanName());
+		CallbackFilterImpl(RootBeanDefinition beanDefinition) {
+			super(beanDefinition);
+		}
+
+		@Override
+		public int accept(Method method) {
+			MethodOverride methodOverride = getBeanDefinition().getMethodOverrides().getOverride(method);
+			if (logger.isTraceEnabled()) {
+				logger.trace("Override for '" + method.getName() + "' is [" + methodOverride + "]");
+			}
+			if (methodOverride == null) {
+				return PASSTHROUGH;
 			}
+			else if (methodOverride instanceof LookupOverride) {
+				return LOOKUP_OVERRIDE;
+			}
+			else if (methodOverride instanceof ReplaceOverride) {
+				return METHOD_REPLACER;
+			}
+			throw new UnsupportedOperationException("Unexpected MethodOverride subclass: "
+					+ methodOverride.getClass().getName());
 		}
+	}
 
+	/**
+	 * CGLIB MethodInterceptor to override methods, replacing them with an
+	 * implementation that returns a bean looked up in the container.
+	 */
+	private static class LookupOverrideMethodInterceptor extends CglibIdentitySupport implements MethodInterceptor {
 
-		/**
-		 * CGLIB MethodInterceptor to override methods, replacing them with a call
-		 * to a generic MethodReplacer.
-		 */
-		private class ReplaceOverrideMethodInterceptor extends CglibIdentitySupport implements MethodInterceptor {
-
-			@Override
-			public Object intercept(Object obj, Method method, Object[] args, MethodProxy mp) throws Throwable {
-				ReplaceOverride ro = (ReplaceOverride) beanDefinition.getMethodOverrides().getOverride(method);
-				// TODO could cache if a singleton for minor performance optimization
-				MethodReplacer mr = (MethodReplacer) owner.getBean(ro.getMethodReplacerBeanName());
-				return mr.reimplement(obj, method, args);
-			}
+		private final BeanFactory owner;
+
+
+		LookupOverrideMethodInterceptor(RootBeanDefinition beanDefinition, BeanFactory owner) {
+			super(beanDefinition);
+			this.owner = owner;
 		}
 
+		@Override
+		public Object intercept(Object obj, Method method, Object[] args, MethodProxy mp) throws Throwable {
+			// Cast is safe, as CallbackFilter filters are used selectively.
+			LookupOverride lo = (LookupOverride) getBeanDefinition().getMethodOverrides().getOverride(method);
+			return this.owner.getBean(lo.getBeanName());
+		}
+	}
 
-		/**
-		 * CGLIB object to filter method interception behavior.
-		 */
-		private class CallbackFilterImpl extends CglibIdentitySupport implements CallbackFilter {
+	/**
+	 * CGLIB MethodInterceptor to override methods, replacing them with a call
+	 * to a generic MethodReplacer.
+	 */
+	private static class ReplaceOverrideMethodInterceptor extends CglibIdentitySupport implements MethodInterceptor {
 
-			@Override
-			public int accept(Method method) {
-				MethodOverride methodOverride = beanDefinition.getMethodOverrides().getOverride(method);
-				if (logger.isTraceEnabled()) {
-					logger.trace("Override for '" + method.getName() + "' is [" + methodOverride + "]");
-				}
-				if (methodOverride == null) {
-					return PASSTHROUGH;
-				}
-				else if (methodOverride instanceof LookupOverride) {
-					return LOOKUP_OVERRIDE;
-				}
-				else if (methodOverride instanceof ReplaceOverride) {
-					return METHOD_REPLACER;
-				}
-				throw new UnsupportedOperationException(
-						"Unexpected MethodOverride subclass: " + methodOverride.getClass().getName());
-			}
+		private final BeanFactory owner;
+
+
+		ReplaceOverrideMethodInterceptor(RootBeanDefinition beanDefinition, BeanFactory owner) {
+			super(beanDefinition);
+			this.owner = owner;
+		}
+
+		@Override
+		public Object intercept(Object obj, Method method, Object[] args, MethodProxy mp) throws Throwable {
+			ReplaceOverride ro = (ReplaceOverride) getBeanDefinition().getMethodOverrides().getOverride(method);
+			// TODO could cache if a singleton for minor performance optimization
+			MethodReplacer mr = owner.getBean(ro.getMethodReplacerBeanName(), MethodReplacer.class);
+			return mr.reimplement(obj, method, args);
 		}
 	}
 

spring-context/src/test/java/org/springframework/beans/factory/xml/XmlBeanFactoryTests.java

@@ -65,7 +65,6 @@ import org.springframework.util.ClassUtils;
 import org.springframework.util.FileCopyUtils;
 import org.springframework.util.SerializationTestUtils;
 import org.springframework.util.StopWatch;
-
 import org.xml.sax.InputSource;
 
 import static org.hamcrest.CoreMatchers.*;
@@ -390,11 +389,11 @@ public final class XmlBeanFactoryTests {
 		XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(xbf);
 		reader.loadBeanDefinitions(OVERRIDES_CONTEXT);
 
-		TestBean david = (TestBean)xbf.getBean("magicDavid");
+		TestBean david = (TestBean) xbf.getBean("magicDavid");
 		// the parent bean is autowiring
 		assertNotNull(david.getSpouse());
 
-		TestBean derivedDavid = (TestBean)xbf.getBean("magicDavidDerived");
+		TestBean derivedDavid = (TestBean) xbf.getBean("magicDavidDerived");
 		// this fails while it inherits from the child bean
 		assertNull("autowiring not propagated along child relationships", derivedDavid.getSpouse());
 	}
@@ -492,7 +491,7 @@ public final class XmlBeanFactoryTests {
 		DefaultListableBeanFactory child = new DefaultListableBeanFactory(parent);
 		new XmlBeanDefinitionReader(child).loadBeanDefinitions(CHILD_CONTEXT);
 		TestBean inherits = (TestBean) child.getBean("singletonInheritsFromParentFactoryPrototype");
-		// Name property value is overriden
+		// Name property value is overridden
 		assertTrue(inherits.getName().equals("prototype-override"));
 		// Age property is inherited from bean in parent factory
 		assertTrue(inherits.getAge() == 2);
@@ -643,17 +642,11 @@ public final class XmlBeanFactoryTests {
 		assertEquals(5, xbf.getSingletonCount());
 	}
 
-	@Test
-	public void testNoSuchFactoryBeanMethod() {
-		try {
-			DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(NO_SUCH_FACTORY_METHOD_CONTEXT);
-			assertNotNull(xbf.getBean("defaultTestBean"));
-			fail("Should not get invalid bean");
-		}
-		catch (BeanCreationException ex) {
-			// Ok
-		}
+	@Test(expected = BeanCreationException.class)
+	public void noSuchFactoryBeanMethod() {
+		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(NO_SUCH_FACTORY_METHOD_CONTEXT);
+		assertNotNull(xbf.getBean("defaultTestBean"));
 	}
 
 	@Test
@@ -757,50 +750,30 @@ public final class XmlBeanFactoryTests {
 		}
 	}
 
-	@Test
-	public void testNoSuchXmlFile() throws Exception {
+	@Test(expected = BeanDefinitionStoreException.class)
+	public void noSuchXmlFile() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-		try {
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(MISSING_CONTEXT);
-			fail("Must not create factory from missing XML");
-		}
-		catch (BeanDefinitionStoreException expected) {
-		}
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(MISSING_CONTEXT);
 	}
 
-	@Test
-	public void testInvalidXmlFile() throws Exception {
+	@Test(expected = BeanDefinitionStoreException.class)
+	public void invalidXmlFile() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-		try {
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(INVALID_CONTEXT);
-			fail("Must not create factory from invalid XML");
-		}
-		catch (BeanDefinitionStoreException expected) {
-		}
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(INVALID_CONTEXT);
 	}
 
-	@Test
-	public void testUnsatisfiedObjectDependencyCheck() throws Exception {
+	@Test(expected = UnsatisfiedDependencyException.class)
+	public void unsatisfiedObjectDependencyCheck() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-		try {
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(UNSATISFIED_OBJECT_DEP_CONTEXT);
-			xbf.getBean("a", DependenciesBean.class);
-			fail("Must have thrown an UnsatisfiedDependencyException");
-		}
-		catch (UnsatisfiedDependencyException ex) {
-		}
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(UNSATISFIED_OBJECT_DEP_CONTEXT);
+		xbf.getBean("a", DependenciesBean.class);
 	}
 
-	@Test
-	public void testUnsatisfiedSimpleDependencyCheck() throws Exception {
+	@Test(expected = UnsatisfiedDependencyException.class)
+	public void unsatisfiedSimpleDependencyCheck() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-		try {
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(UNSATISFIED_SIMPLE_DEP_CONTEXT);
-			xbf.getBean("a", DependenciesBean.class);
-			fail("Must have thrown an UnsatisfiedDependencyException");
-		}
-		catch (UnsatisfiedDependencyException expected) {
-		}
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(UNSATISFIED_SIMPLE_DEP_CONTEXT);
+		xbf.getBean("a", DependenciesBean.class);
 	}
 
 	@Test
@@ -820,16 +793,11 @@ public final class XmlBeanFactoryTests {
 		assertEquals(a.getAge(), 33);
 	}
 
-	@Test
-	public void testUnsatisfiedAllDependencyCheck() throws Exception {
+	@Test(expected = UnsatisfiedDependencyException.class)
+	public void unsatisfiedAllDependencyCheck() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-		try {
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(UNSATISFIED_ALL_DEP_CONTEXT);
-			xbf.getBean("a", DependenciesBean.class);
-			fail("Must have thrown an UnsatisfiedDependencyException");
-		}
-		catch (UnsatisfiedDependencyException expected) {
-		}
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(UNSATISFIED_ALL_DEP_CONTEXT);
+		xbf.getBean("a", DependenciesBean.class);
 	}
 
 	@Test
@@ -1092,28 +1060,18 @@ public final class XmlBeanFactoryTests {
 		assertEquals(File.separator + "test", file.getPath());
 	}
 
-	@Test
-	public void testThrowsExceptionOnTooManyArguments() throws Exception {
+	@Test(expected = BeanCreationException.class)
+	public void throwsExceptionOnTooManyArguments() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
 		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(CONSTRUCTOR_ARG_CONTEXT);
-		try {
-			xbf.getBean("rod7", ConstructorDependenciesBean.class);
-			fail("Should have thrown BeanCreationException");
-		}
-		catch (BeanCreationException expected) {
-		}
+		xbf.getBean("rod7", ConstructorDependenciesBean.class);
 	}
 
-	@Test
-	public void testThrowsExceptionOnAmbiguousResolution() throws Exception {
+	@Test(expected = UnsatisfiedDependencyException.class)
+	public void throwsExceptionOnAmbiguousResolution() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
 		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(CONSTRUCTOR_ARG_CONTEXT);
-		try {
-			xbf.getBean("rod8", ConstructorDependenciesBean.class);
-			fail("Must have thrown UnsatisfiedDependencyException");
-		}
-		catch (UnsatisfiedDependencyException expected) {
-		}
+		xbf.getBean("rod8", ConstructorDependenciesBean.class);
 	}
 
 	@Test
@@ -1274,17 +1232,10 @@ public final class XmlBeanFactoryTests {
 		xbf.getBean("resource2", ResourceTestBean.class);
 	}
 
-	@Test
-	public void testRecursiveImport() {
+	@Test(expected = BeanDefinitionStoreException.class)
+	public void recursiveImport() {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
-		try {
-			new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(RECURSIVE_IMPORT_CONTEXT);
-			fail("Should have thrown BeanDefinitionStoreException");
-		}
-		catch (BeanDefinitionStoreException ex) {
-			// expected
-			ex.printStackTrace();
-		}
+		new XmlBeanDefinitionReader(xbf).loadBeanDefinitions(RECURSIVE_IMPORT_CONTEXT);
 	}
 
 	/**
@@ -1296,7 +1247,7 @@ public final class XmlBeanFactoryTests {
 	public void methodInjectedBeanMustBeOfSameEnhancedCglibSubclassTypeAcrossBeanFactories() {
 		Class<?> firstClass = null;
 
-		for (int i = 1; i <= 10; i++) {
+		for (int i = 0; i < 10; i++) {
 			DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
 			new XmlBeanDefinitionReader(bf).loadBeanDefinitions(OVERRIDES_CONTEXT);
 
@@ -1314,15 +1265,15 @@ public final class XmlBeanFactoryTests {
 	}
 
 	@Test
-	public void testLookupOverrideMethodsWithSetterInjection() {
+	public void lookupOverrideMethodsWithSetterInjection() {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
 		XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(xbf);
 		reader.loadBeanDefinitions(OVERRIDES_CONTEXT);
 
-		testLookupOverrideMethodsWithSetterInjection(xbf, "overrideOneMethod", true);
+		lookupOverrideMethodsWithSetterInjection(xbf, "overrideOneMethod", true);
 		// Should work identically on subclass definition, in which lookup
 		// methods are inherited
-		testLookupOverrideMethodsWithSetterInjection(xbf, "overrideInheritedMethod", true);
+		lookupOverrideMethodsWithSetterInjection(xbf, "overrideInheritedMethod", true);
 
 		// Check cost of repeated construction of beans with method overrides
 		// Will pick up misuse of CGLIB
@@ -1330,10 +1281,10 @@ public final class XmlBeanFactoryTests {
 		StopWatch sw = new StopWatch();
 		sw.start("Look up " + howMany + " prototype bean instances with method overrides");
 		for (int i = 0; i < howMany; i++) {
-			testLookupOverrideMethodsWithSetterInjection(xbf, "overrideOnPrototype", false);
+			lookupOverrideMethodsWithSetterInjection(xbf, "overrideOnPrototype", false);
 		}
 		sw.stop();
-		System.out.println(sw);
+		// System.out.println(sw);
 		if (!LogFactory.getLog(DefaultListableBeanFactory.class).isDebugEnabled()) {
 			assertTrue(sw.getTotalTimeMillis() < 2000);
 		}
@@ -1347,7 +1298,8 @@ public final class XmlBeanFactoryTests {
 		assertEquals("Jenny", tb.getName());
 	}
 
-	private void testLookupOverrideMethodsWithSetterInjection(BeanFactory xbf, String beanName, boolean singleton) {
+	private void lookupOverrideMethodsWithSetterInjection(BeanFactory xbf,
+			String beanName, boolean singleton) {
 		OverrideOneMethod oom = (OverrideOneMethod) xbf.getBean(beanName);
 
 		if (singleton) {
@@ -1428,7 +1380,7 @@ public final class XmlBeanFactoryTests {
 	}
 
 	@Test
-	public void testLookupOverrideOneMethodWithConstructorInjection() {
+	public void lookupOverrideOneMethodWithConstructorInjection() {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
 		XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(xbf);
 		reader.loadBeanDefinitions(CONSTRUCTOR_OVERRIDES_CONTEXT);
@@ -1466,21 +1418,8 @@ public final class XmlBeanFactoryTests {
 		}
 	}
 
-	/**
-	 * Assert the presence of this bug until we resolve it.
-	 */
 	@Test
-	public void testSerializabilityOfMethodReplacer() throws Exception {
-		try {
-			BUGtestSerializableMethodReplacerAndSuperclass();
-			fail();
-		}
-		catch (AssertionError ex) {
-			System.err.println("****** SPR-356: Objects with MethodReplace overrides are not serializable");
-		}
-	}
-
-	public void BUGtestSerializableMethodReplacerAndSuperclass() throws IOException, ClassNotFoundException {
+	public void serializableMethodReplacerAndSuperclass() throws Exception {
 		DefaultListableBeanFactory xbf = new DefaultListableBeanFactory();
 		XmlBeanDefinitionReader reader = new XmlBeanDefinitionReader(xbf);
 		reader.loadBeanDefinitions(DELEGATION_OVERRIDES_CONTEXT);
@@ -1488,9 +1427,10 @@ public final class XmlBeanFactoryTests {
 		String forwards = "this is forwards";
 		String backwards = new StringBuffer(forwards).reverse().toString();
 		assertEquals(backwards, s.replaceMe(forwards));
-		assertTrue(SerializationTestUtils.isSerializable(s));
-		s = (SerializableMethodReplacerCandidate) SerializationTestUtils.serializeAndDeserialize(s);
-		assertEquals("Method replace still works after serialization and deserialization", backwards, s.replaceMe(forwards));
+		// SPR-356: lookup methods & method replacers are not serializable.
+		assertFalse(
+				"Lookup methods and method replacers are not meant to be serializable.",
+				SerializationTestUtils.isSerializable(s));
 	}
 
 	@Test