Repository: mockito/mockito
Issue: Incomplete stack trace returned from spy inside another spy
Labels: please contribute

Hi,

due to the fix of issue #2238, the returned stack trace is not complete when an exception is thrown from a spy inside another spy.
The problem appeared in released version 3.9.0 (specifically 3.8.14) and still exists in 4.1.0.

I wrote a unit test simulating this behavior:
https://github.com/holubec-petr/mockito/blob/stack_trace_clean_up/src/test/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMakerTest.java#L272

Problem is that the method `MockMethodAdvice.hideRecursiveCall()` is too aggressive when removing stack trace frames.

Check that
 - [x] The mockito message in the stacktrace have useful information, but it didn't help
 - [x] The problematic code (if that's possible) is copied here;
       Note that some configuration are impossible to mock via Mockito
 - [x] Provide versions (mockito / jdk / os / any other relevant information)
 - [x] Provide a [Short, Self Contained, Correct (Compilable), Example](http://sscce.org) of the issue
       (same as any question on stackoverflow.com)
 - [x] Read the [contributing guide](https://github.com/mockito/mockito/blob/main/.github/CONTRIBUTING.md)




Issue comments:
Do you mind sending us a PR with the regression test and a corresponding fix? That would be awesome 🎉 
I'm thinking about some fix the last few days and I don't know if I'll be able to provide any generic-enough fix.

Maybe that's just my lack of knowledge of why is this stack trace cleaning implemented and what frames exactly we want to remove.

For example, this unit test:
https://github.com/mockito/mockito/blob/main/src/test/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMakerTest.java#L270
Why exactly those 3 elements should remain in stack trace after cleaning?

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java
index 1490501eae..580f22b309 100644
--- a/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java
+++ b/src/main/java/org/mockito/internal/creation/bytebuddy/MockMethodAdvice.java
@@ -19,6 +19,9 @@
 import java.lang.reflect.Method;
 import java.util.List;
 import java.util.Map;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Comparator;
 import java.util.concurrent.Callable;
 import java.util.function.Predicate;
 
@@ -114,27 +117,6 @@ private static void exit(
         }
     }
 
-    static Throwable hideRecursiveCall(Throwable throwable, int current, Class<?> targetType) {
-        try {
-            StackTraceElement[] stack = throwable.getStackTrace();
-            int skip = 0;
-            StackTraceElement next;
-            do {
-                next = stack[stack.length - current - ++skip];
-            } while (!next.getClassName().equals(targetType.getName()));
-            int top = stack.length - current - skip;
-            StackTraceElement[] cleared = new StackTraceElement[stack.length - skip];
-            System.arraycopy(stack, 0, cleared, 0, top);
-            System.arraycopy(stack, top + skip, cleared, top, current);
-            throwable.setStackTrace(cleared);
-            return throwable;
-        } catch (RuntimeException ignored) {
-            // This should not happen unless someone instrumented or manipulated exception stack
-            // traces.
-            return throwable;
-        }
-    }
-
     @Override
     public Callable<?> handle(Object instance, Method origin, Object[] arguments) throws Throwable {
         MockMethodInterceptor interceptor = interceptors.get(instance);
@@ -333,25 +315,34 @@ private static Object tryInvoke(Method origin, Object instance, Object[] argumen
             return accessor.invoke(origin, instance, arguments);
         } catch (InvocationTargetException exception) {
             Throwable cause = exception.getCause();
-            StackTraceElement[] tmpStack = new Throwable().getStackTrace();
-
-            int skip = tmpStack.length;
-            // if there is a suitable instance, do not skip the root-cause for the exception
-            if (instance != null) {
-                skip = 0;
-                String causingClassName = instance.getClass().getName();
-                StackTraceElement stackFrame;
-                do {
-                    stackFrame = tmpStack[skip++];
-                } while (stackFrame.getClassName().startsWith(causingClassName));
-            }
-
             new ConditionalStackTraceFilter()
-                    .filter(hideRecursiveCall(cause, skip, origin.getDeclaringClass()));
+                    .filter(removeRecursiveCalls(cause, origin.getDeclaringClass()));
             throw cause;
         }
     }
 
+    static Throwable removeRecursiveCalls(final Throwable cause, final Class<?> declaringClass) {
+        final List<String> uniqueStackTraceItems = new ArrayList<>();
+        final List<Integer> indexesToBeRemoved = new ArrayList<>();
+        for (StackTraceElement element : cause.getStackTrace()) {
+            final String key = element.getClassName() + element.getLineNumber();
+            final int elementIndex = uniqueStackTraceItems.lastIndexOf(key);
+            uniqueStackTraceItems.add(key);
+
+            if (elementIndex > -1 && declaringClass.getName().equals(element.getClassName())) {
+                indexesToBeRemoved.add(elementIndex);
+            }
+        }
+        final List<StackTraceElement> adjustedList =
+                new ArrayList<>(Arrays.asList(cause.getStackTrace()));
+        indexesToBeRemoved.stream()
+                .sorted(Comparator.reverseOrder())
+                .mapToInt(Integer::intValue)
+                .forEach(adjustedList::remove);
+        cause.setStackTrace(adjustedList.toArray(new StackTraceElement[] {}));
+        return cause;
+    }
+
     private static class ReturnValueWrapper implements Callable<Object> {
 
         private final Object returned;


Test patch:
diff --git a/src/test/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMakerTest.java b/src/test/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMakerTest.java
index e27605640b..3d42f3a377 100644
--- a/src/test/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMakerTest.java
+++ b/src/test/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMakerTest.java
@@ -9,19 +9,14 @@
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.fail;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.*;
 import static org.junit.Assume.assumeTrue;
 
 import java.io.IOException;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Observable;
-import java.util.Observer;
-import java.util.Optional;
-import java.util.Set;
+import java.util.*;
 import java.util.concurrent.Callable;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import net.bytebuddy.ByteBuddy;
 import net.bytebuddy.ClassFileVersion;
@@ -253,17 +248,56 @@ public void should_leave_causing_stack() throws Exception {
                 mockMaker.createSpy(
                         settings, new MockHandlerImpl<>(settings), new ExceptionThrowingClass());
 
-        StackTraceElement[] returnedStack = null;
-        try {
-            proxy.get().throwException();
-        } catch (IOException ex) {
-            returnedStack = ex.getStackTrace();
-        }
+        StackTraceElement[] returnedStack =
+                assertThrows(IOException.class, () -> proxy.get().throwException()).getStackTrace();
 
         assertNotNull("Stack trace from mockito expected", returnedStack);
 
-        assertEquals(ExceptionThrowingClass.class.getName(), returnedStack[0].getClassName());
-        assertEquals("internalThrowException", returnedStack[0].getMethodName());
+        List<StackTraceElement> exceptionClassElements =
+                Arrays.stream(returnedStack)
+                        .filter(
+                                element ->
+                                        element.getClassName()
+                                                .equals(ExceptionThrowingClass.class.getName()))
+                        .collect(Collectors.toList());
+        assertEquals(3, exceptionClassElements.size());
+        assertEquals("internalThrowException", exceptionClassElements.get(0).getMethodName());
+        assertEquals("internalThrowException", exceptionClassElements.get(1).getMethodName());
+        assertEquals("throwException", exceptionClassElements.get(2).getMethodName());
+    }
+
+    @Test
+    public void should_leave_causing_stack_with_two_spies() throws Exception {
+        // given
+        MockSettingsImpl<ExceptionThrowingClass> settingsEx = new MockSettingsImpl<>();
+        settingsEx.setTypeToMock(ExceptionThrowingClass.class);
+        settingsEx.defaultAnswer(Answers.CALLS_REAL_METHODS);
+        Optional<ExceptionThrowingClass> proxyEx =
+                mockMaker.createSpy(
+                        settingsEx,
+                        new MockHandlerImpl<>(settingsEx),
+                        new ExceptionThrowingClass());
+
+        MockSettingsImpl<WrapperClass> settingsWr = new MockSettingsImpl<>();
+        settingsWr.setTypeToMock(WrapperClass.class);
+        settingsWr.defaultAnswer(Answers.CALLS_REAL_METHODS);
+        Optional<WrapperClass> proxyWr =
+                mockMaker.createSpy(
+                        settingsWr, new MockHandlerImpl<>(settingsWr), new WrapperClass());
+
+        // when
+        IOException ex =
+                assertThrows(IOException.class, () -> proxyWr.get().callWrapped(proxyEx.get()));
+        List<StackTraceElement> wrapperClassElements =
+                Arrays.stream(ex.getStackTrace())
+                        .filter(
+                                element ->
+                                        element.getClassName().equals(WrapperClass.class.getName()))
+                        .collect(Collectors.toList());
+
+        // then
+        assertEquals(1, wrapperClassElements.size());
+        assertEquals("callWrapped", wrapperClassElements.get(0).getMethodName());
     }
 
     @Test
@@ -271,30 +305,33 @@ public void should_remove_recursive_self_call_from_stack_trace() throws Exceptio
         StackTraceElement[] stack =
                 new StackTraceElement[] {
                     new StackTraceElement("foo", "", "", -1),
-                    new StackTraceElement(SampleInterface.class.getName(), "", "", -1),
+                    new StackTraceElement(SampleInterface.class.getName(), "", "", 15),
                     new StackTraceElement("qux", "", "", -1),
                     new StackTraceElement("bar", "", "", -1),
+                    new StackTraceElement(SampleInterface.class.getName(), "", "", 15),
                     new StackTraceElement("baz", "", "", -1)
                 };
 
         Throwable throwable = new Throwable();
         throwable.setStackTrace(stack);
-        throwable = MockMethodAdvice.hideRecursiveCall(throwable, 2, SampleInterface.class);
+        throwable = MockMethodAdvice.removeRecursiveCalls(throwable, SampleInterface.class);
 
         assertThat(throwable.getStackTrace())
                 .isEqualTo(
                         new StackTraceElement[] {
                             new StackTraceElement("foo", "", "", -1),
+                            new StackTraceElement("qux", "", "", -1),
                             new StackTraceElement("bar", "", "", -1),
+                            new StackTraceElement(SampleInterface.class.getName(), "", "", 15),
                             new StackTraceElement("baz", "", "", -1)
                         });
     }
 
     @Test
-    public void should_handle_missing_or_inconsistent_stack_trace() throws Exception {
+    public void should_handle_missing_or_inconsistent_stack_trace() {
         Throwable throwable = new Throwable();
         throwable.setStackTrace(new StackTraceElement[0]);
-        assertThat(MockMethodAdvice.hideRecursiveCall(throwable, 0, SampleInterface.class))
+        assertThat(MockMethodAdvice.removeRecursiveCalls(throwable, SampleInterface.class))
                 .isSameAs(throwable);
     }
 
@@ -579,6 +616,12 @@ public T value() {
         }
     }
 
+    public static class WrapperClass {
+        public void callWrapped(ExceptionThrowingClass exceptionThrowingClass) throws IOException {
+            exceptionThrowingClass.throwException();
+        }
+    }
+
     public static class GenericSubClass extends GenericClass<String> {}
 
     public static class ExceptionThrowingClass {