@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:

``` // 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 ()V L0 LINENUMBER 5 L0 ALOAD 0 INVOKESPECIAL java/lang/Object. ()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. ()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. (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

``` // 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 ()V ALOAD 0 INVOKESPECIAL org/hibernate/bytebuddy/playground/MyClass. ()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.