• Bug
  • Status: Closed
  • 3 Minor
  • Resolution: Fixed
  • DSO:L1
  • nadeem
  • Reporter: hhuynh
  • August 08, 2007
  • 0
  • Watchers: 0
  • February 02, 2009
  • January 16, 2009

Description

For a class that doesn’t implement Cloneable, the expected CloneNotSupportedException got lost if you wrap it in RuntimeException. The caller will see a IllegalMonitorStateException instead.

Test for this bug: CloneExceptionTest, currently disabled.

private static class MyStuff { protected Object clone() { try { return super.clone(); } catch (CloneNotSupportedException e) { throw new RuntimeException(e); } } }

Another case to be considered is when clone() is called on an object in a way other than super.clone(), for instance when an object’s clone clones the fields of itself.

Additionally, there seems to be another wrinkle when calling clone() on an Object[] such that the same sort of resolving needs to occur that happens with a typical call to Object.clone(). As far as I can see, that’s not handled by any of the current code.

Comments

Alex Miller 2007-08-23

The modification currently made by TransparencyCodeAdapter.handleJavaLangObjectCloneCall() is to replace calls to refToBeCloned.clone() with:

   Object refToBeCloned;
   Object rv;
   
   TCObject tco = (refToBeCloned instanceof Manageable) ? ((Manageable) refToBeCloned).__tc_managed() : null;
   if (tco != null) {
     synchronized (tco.getResolveLock()) {
       tco.resolveAllReferences();
       rv = Util.fixTCObjectReferenceOfClonedObject(refToBeCloned, refToBeCloned.clone());
     }
   } else {
     rv = refToBeCloned.clone();
   }

This modification of the caller of clone() is done using ASM and is not really exactly this, but something like this with some modifications. The subtle bug is introduced because clone() can throw CloneNotSupportedException, which is not handled here and the ASM modification will not (cannot?) properly update the exception handlers at the call site. I took a few stabs at tweaking the ASM code but it’s pretty hairy (and will make this code even harder to maintain going forward).

Tim and I have been discussing some alternatives. One possibility is to make the modification at the callee intead of in the caller. We were trying to do it without modifying Manageable but I’m not sure it’s possible to both do this in Manageable and avoid reflection (for performance).

Option 1: Given a clone() method on a Manageable object, replace with:

	public Object clone() {		// copy throws from original
		return Util.cloneHelper(this);
	}
	
	public Object __tc_clone() {	// copy throws from original clone
		return new Bar(a);
	}

and add a new static Util method (which would only be called from code in Manageable clone methods):

public static Object cloneHelper(Manabeable refToBeCloned) throws CloneNotSupportedException {
	Object rv;    
	TCObject tco = refToBeCloned.__tc_managed();
	if (tco != null) {
  		synchronized (tco.getResolveLock()) {
    		    tco.resolveAllReferences();
    		    rv = Util.fixTCObjectReferenceOfClonedObject(refToBeCloned, refToBeCloned.__tc_clone());
                }
	} else {
          rv = refToBeCloned.__tc_clone();      
	}

The issue here is that __tc_clone() would need to be added to Manageable. I could work around that by using reflection in the call above to __tc_clone(), but I presume the performance implications would be too much to bear.

Another option would be to modify at both caller and callee site so that the caller would receive an extra method to do this logic. I think the exact same Manageable/reflection problem arises though (which is why callee would need a new method). Maybe this is the only option for logical?

Thoughts?

Geert Bevin 2007-08-24

I didn’t look at it myself, but just read through your description. Imho changing Manageable is not really an option unless it’s done in a major version upgrade. I was try to tweak the ASM modification to properly catch the exception and generate the expected one. Maybe it’s not possible, as you say, but it seems to me that it should be.

Alex Miller 2007-08-24

The solution with the smallest change is probably tweaking the existing approach to handle exceptions thrown from the call to clone() better (basically need to make sure monitorexit is called). I think that is possible, having looked at it some more, just merely tricky and hard to maintain. :) So, I thought it was worth throwing out a couple alternatives.

Instead of changing Manageable, another option might be to add a new separate interface like ManageableWithClone that managed objects with clone() would also implement (in addition to Manageable). Or to extend the Manageable interface with a new interface like this.

I’d love some insight on how logical managed objects play into the choices here too - I don’t understand enough of the difference to know whether it affects the choices.

Alex Miller 2007-11-09

Retarget per Steve

Chris Dennis 2008-12-16

I would go so far as to label this an ASM bug. Looking at ClassReader and ClassWriter it seems like ASM is forcing all the original exception handlers to the top of the table regardless of any nesting/scope relationships between the original handlers and any new handlers that have been added.

Chris Dennis 2008-12-19

I modified my local ASM copy (com.tc.asm) by implementing an exception handler comparator and then modding MethodWriter to sort the handlers using the comparator before writing out. This fix made the CloneExceptionTest pass…

Chris Dennis 2008-12-19

It appears ASM javadoc is incorrect… I managed to do this with just a method adapter. Adapter now checked into trunk at “dso-l1/com/tc/object/bytecode/ExceptionTableOrderingMethodAdapter”.

Alex Miller 2008-12-19

If fixable now, go for it in 2.7.3.

Chris Dennis 2009-01-16

Exception table ordering (via a new adapter in the main chain) now ensures that the finally block that unlocks the monitor catches this exception (or any other exception) before any enclosing handler.

nadeem ghani 2009-01-28

test is enabled and passing