@Advice.Origin will not always provide the right method when an advice is applied to a bridged method

[yrodiere]

Context

While the Java language forbids two methods to have the same signature with a different return type, the JVM allows this.

Thus, the bytecode of a given class can actually contain two methods with the same name and arguments, just with a different return type.

Some libraries take advantage of this using various bytecode generators in order to change the return type of a method in a new version of the library, while still preserving backwards compatibility by keeping the old version of the same method, with the old return type.

Description of the problem

Unfortunately, when applying an Advice, byte-buddy doesn't seem to take into account the fact that the same method can appear multiple times with a different return type:

• It only generates one method override, apparently the original one. Maybe it cannot see the bridge methods for some reason? Anyway, that's not really a problem in my case, except that...
• When the advice uses an argument such as @Advice.Origin Method origin to inject a Method representing the method being "advised", byte-buddy injects a method with the same name and arguments, but not necessarily the right return type.

Reproducer

See https://github.com/yrodiere/bytebuddy-playground/tree/bridges-wrong-method-passed-to-advice . Just run with ./mvnw clean test.

We use this class as the base to apply an advice:

public class MyClass {

	@WithBridgeMethods(value = { String.class, int.class }, adapterMethod = "longToStringOrInt")
	public long myMethod() {
		return 0L;
	}

	private Object longToStringOrInt(long value, Class type) {
		if (type == String.class)
			return String.valueOf(value);
		if (type == int.class)
			return (int) value;
		throw new AssertionError("Unexpected type: " + type);
	}

}

With a processor that generates two bridge methods. The bytecode ends up looking like this:

Details

// class version 52.0 (52)
// access flags 0x21
public class org/hibernate/bytebuddy/playground/MyClass {

  // compiled from: MyClass.java

  @Lcom/infradna/tool/bridge_method_injector/BridgeMethodsAdded;() // invisible

  // access flags 0x1
  public <init>()V
   L0
    LINENUMBER 5 L0
    ALOAD 0
    INVOKESPECIAL java/lang/Object.<init> ()V
    RETURN
   L1
    LOCALVARIABLE this Lorg/hibernate/bytebuddy/playground/MyClass; L0 L1 0
    MAXSTACK = 1
    MAXLOCALS = 1

  // access flags 0x1
  public myMethod()J
  @Lcom/infradna/tool/bridge_method_injector/WithBridgeMethods;(value={java.lang.String.class, int.class}, adapterMethod="longToStringOrInt") // invisible
   L0
    LINENUMBER 9 L0
    LCONST_0
    LRETURN
   L1
    LOCALVARIABLE this Lorg/hibernate/bytebuddy/playground/MyClass; L0 L1 0
    MAXSTACK = 2
    MAXLOCALS = 1

  // access flags 0x2
  private longToStringOrInt(JLjava/lang/Class;)Ljava/lang/Object;
   L0
    LINENUMBER 13 L0
    ALOAD 3
    LDC Ljava/lang/String;.class
    IF_ACMPNE L1
   L2
    LINENUMBER 14 L2
    LLOAD 1
    INVOKESTATIC java/lang/String.valueOf (J)Ljava/lang/String;
    ARETURN
   L1
    LINENUMBER 15 L1
   FRAME SAME
    ALOAD 3
    GETSTATIC java/lang/Integer.TYPE : Ljava/lang/Class;
    IF_ACMPNE L3
   L4
    LINENUMBER 16 L4
    LLOAD 1
    L2I
    INVOKESTATIC java/lang/Integer.valueOf (I)Ljava/lang/Integer;
    ARETURN
   L3
    LINENUMBER 17 L3
   FRAME SAME
    NEW java/lang/AssertionError
    DUP
    NEW java/lang/StringBuilder
    DUP
    INVOKESPECIAL java/lang/StringBuilder.<init> ()V
    LDC "Unexpected type: "
    INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/String;)Ljava/lang/StringBuilder;
    ALOAD 3
    INVOKEVIRTUAL java/lang/StringBuilder.append (Ljava/lang/Object;)Ljava/lang/StringBuilder;
    INVOKEVIRTUAL java/lang/StringBuilder.toString ()Ljava/lang/String;
    INVOKESPECIAL java/lang/AssertionError.<init> (Ljava/lang/Object;)V
    ATHROW
   L5
    LOCALVARIABLE this Lorg/hibernate/bytebuddy/playground/MyClass; L0 L5 0
    LOCALVARIABLE value J L0 L5 1
    LOCALVARIABLE type Ljava/lang/Class; L0 L5 3
    MAXSTACK = 4
    MAXLOCALS = 4

  // access flags 0x1041
  public synthetic bridge myMethod()Ljava/lang/String;
    ALOAD 0
    ALOAD 0
    INVOKEVIRTUAL org/hibernate/bytebuddy/playground/MyClass.myMethod ()J
    LDC Ljava/lang/String;.class
    INVOKEVIRTUAL org/hibernate/bytebuddy/playground/MyClass.longToStringOrInt (JLjava/lang/Class;)Ljava/lang/Object;
    CHECKCAST java/lang/String
    ARETURN
    MAXSTACK = 4
    MAXLOCALS = 1

  // access flags 0x1041
  public synthetic bridge myMethod()I
    ALOAD 0
    ALOAD 0
    INVOKEVIRTUAL org/hibernate/bytebuddy/playground/MyClass.myMethod ()J
    GETSTATIC java/lang/Integer.TYPE : Ljava/lang/Class;
    INVOKEVIRTUAL org/hibernate/bytebuddy/playground/MyClass.longToStringOrInt (JLjava/lang/Class;)Ljava/lang/Object;
    CHECKCAST java/lang/Number
    INVOKEVIRTUAL java/lang/Number.intValue ()I
    IRETURN
    MAXSTACK = 4
    MAXLOCALS = 1
}

We apply this advice:

public class MyAdvice {
	@Advice.OnMethodEnter(inline = false)
	public static Callable<?> enter(@Advice.Origin Method origin) {
		return new Callable<Object>() {
			@Override
			public Object call() throws Exception {
				Class<?> returnType = origin.getReturnType();
				if ( String.class.equals( returnType ) ) {
					return "42";
				}
				else if ( long.class.equals( returnType ) ) {
					return 42L;
				}
				else if ( int.class.equals( returnType ) ) {
					return 42;
				}
				throw new IllegalStateException( "Unsupported return type: " + returnType );
			}
		};
	}

	@Advice.OnMethodExit
	public static void exit(
			@Advice.Return(readOnly = false, typing = Assigner.Typing.DYNAMIC) Object returned,
			@Advice.Enter Callable<?> mocked)
			throws Throwable {
		returned = mocked.call();
	}
}

Like this:

		DynamicType.Unloaded<MyClass> unloadedTypeWithAdvice = new ByteBuddy()
				// Apply suggestion from https://github.com/raphw/byte-buddy/issues/999#issuecomment-759773044
				// This does not solve the problem, unfortunately.
				.with( MethodGraph.Compiler.Default.forJVMHierarchy() )
				.subclass( MyClass.class )
				.name( MyClass.class.getName() + "_withAdvice" )
				.method( named( "myMethod" ) )
				.intercept( Advice.to( MyAdvice.class ) )
				.make();

And byte-buddy generates the following bytecode, where:

• only the original method is overridden (not the bridge methods)
• the retrieval of the Method instance doesn't take care of getting the right one with the right return type

Details

// class version 55.0 (55)
// access flags 0x21
public class org/hibernate/bytebuddy/playground/MyClass_withAdvice extends org/hibernate/bytebuddy/playground/MyClass {


  // access flags 0x1
  public myMethod()J
    LDC Lorg/hibernate/bytebuddy/playground/MyClass;.class
    LDC "myMethod"
    ICONST_0
    ANEWARRAY java/lang/Class
    INVOKEVIRTUAL java/lang/Class.getMethod (Ljava/lang/String;[Ljava/lang/Class;)Ljava/lang/reflect/Method;
    INVOKESTATIC org/hibernate/bytebuddy/playground/MyAdvice.enter (Ljava/lang/reflect/Method;)Ljava/util/concurrent/Callable;
    ASTORE 1
   L0
   FRAME APPEND [java/util/concurrent/Callable]
    ALOAD 0
    ASTORE 2
   L1
   FRAME APPEND [org/hibernate/bytebuddy/playground/MyClass_withAdvice]
    NOP
    ALOAD 2
    INVOKESPECIAL org/hibernate/bytebuddy/playground/MyClass.myMethod ()J
    GOTO L2
   L2
   FRAME FULL [org/hibernate/bytebuddy/playground/MyClass_withAdvice java/util/concurrent/Callable] [J]
    LSTORE 2
   L3
   FRAME APPEND [J]
    ALOAD 1
    INVOKEINTERFACE java/util/concurrent/Callable.call ()Ljava/lang/Object; (itf)
    CHECKCAST java/lang/Long
    INVOKEVIRTUAL java/lang/Long.longValue ()J
    LSTORE 2
    GOTO L4
   L4
   FRAME SAME
    LLOAD 2
    LRETURN
    MAXSTACK = 4
    MAXLOCALS = 5

  // access flags 0x1
  public <init>()V
    ALOAD 0
    INVOKESPECIAL org/hibernate/bytebuddy/playground/MyClass.<init> ()V
    RETURN
    MAXSTACK = 1
    MAXLOCALS = 1
}

Affected projects

Most problematic is the fact that this affects Mockito when trying to mock methods that happen to also have bridges declared.

In my specific case the library declaring bridge methods is https://github.com/hub4j/github-api/, and it's using https://github.com/infradna/bridge-method-injector to generate the bridge methods. When we try to mock such a method (e.g. GHObject#getId) with Mockito, byte-buddy passes the wrong Method instance to Mockito, which selects the wrong return value for the mock, ultimately leading to an NPE when trying to convert null to a primitive int.

[raphw]

Byte Buddy uses a resolver to find the "non-bridge" method for every stack, as methods and their bridges might be declared on multiple levels of a type hierarchy. Byte Buddy cannot really override bridges the same way, as it would cause endless loops: when a bridge method override invokes its actual super method, it would virtually dispatch the next bridge value; therefore this is not possible.

Also, the reflection API via getMethod normally resolves the first method with a given name and parameter types. javac makes sure that this is the non-bridge. As loops require frame emission, this is the pragmatic approach.

I generally recommend against using such bridge emission tools. The reflection API does not understand it and it confuses other frameworks, too. The problem also seems to be rather rare. Instead, if changing a return type, add a super interface with the old signature as parameterized bound type and implement it such that javac generates a valid hierarchy. If the type is non-replaceable, I recommend adding a method with a different name.

I can look into Byte Buddy's behavior in such cases and see if I can improve the default, but bridge method resolution is already rather complicated, so I would not want to implode it to cover a corner case.

[yrodiere]

Byte Buddy cannot really override bridges the same way

Understood. That's fine by me; I mentioned this because that seemed strange, but that doesn't affect me as I don't call the bridge methods anyway. Thanks for the explanation.

I generally recommend against using such bridge emission tools

I couldn't agree more; these hacks have proven a nightmare. Unfortunately, this is code I don't have control over...

the reflection API via getMethod normally resolves the first method with a given name and parameter types. javac makes sure that this is the non-bridge

I would expect this as well, but as you can see in my reproducer, this is not what is happening. In the reproducer, getMethod definitely returns a bridge method.

As loops require frame emission, this is the pragmatic approach

Ok, so no loops in the general case; understood.

Maybe it would be possible, during bytecode generation, to detect that there are bridge methods, and in that case only we would generate a loop that selects the Method with the right return type? That way most people not dealing with bridges would be unaffected. People dealing with bridges would take a performance hit, but at least Advice would work for them.

[raphw]

I will look into it once I have some free time. I will need to investigate how javac behaves in different scenarios and how the reflection API aligns.

[gsmet]

Thanks @raphw , it's very much appreciated as it makes testing our quarkus-github-app based bots close to impossible given the Kohsuke's GitHub API uses this pattern a lot.