Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'this' is not available - when enabling mock-maker-inline #2082

Closed
leonroy opened this issue Oct 28, 2020 · 33 comments
Closed

'this' is not available - when enabling mock-maker-inline #2082

leonroy opened this issue Oct 28, 2020 · 33 comments
Assignees

Comments

@leonroy
Copy link

leonroy commented Oct 28, 2020

When we enable mock-maker-inline in our project and debug a class with @Spy annotated on it IntelliJ reports that 'this' is not available:
image

When we unset mock-maker-inline then things work as expected and we can inspect the class variables:
image

It appears to be the same as this report: https://stackoverflow.com/questions/62661996/unable-to-debug-junit-test-anywhere

Is this expected Mockito behaviour?

@leonroy leonroy changed the title 'this' is not available 'this' is not available - when enabling *mock-maker-inline* Oct 28, 2020
@leonroy leonroy changed the title 'this' is not available - when enabling *mock-maker-inline* 'this' is not available - when enabling mock-maker-inline Oct 28, 2020
@TimvdLippe
Copy link
Contributor

Thanks for filing the issue. Could you please provide us a minimal reproduction case that we can clone/download to debug the issue? Thanks in advance!

@AlarkaSanyal
Copy link

Facing a similar issue. It can be reproduced by following these steps: https://www.baeldung.com/mockito-final
and then debugging through the code. For any variable, in the Variables window, we can see the following
image

@jwb441
Copy link

jwb441 commented Jan 21, 2021

I am able to recreate this in Eclipse with the following example
Eclipse: 2020-09 (4.17.0)
Java: jdk1.8.0_201
JUnit: 5.4.2
mockito: 3.6.0 (core buildtime inline testruntime)

When I try to step into the call at
assertEquals(1, myClass.doAThing());

I get: com.sun.jdi.InternalException: Got error code in reply:35 occurred retrieving 'this' from stack frame.

MockitoInlineTest.txt
MyClass.txt

@jwb441
Copy link

jwb441 commented Feb 10, 2021

Let me know if there is any additional data I can add to help get to the bottom of this.

Thanks!
John

@afillatre
Copy link

Same issue with Mockito-inline 3.8.0 and java 15. This makes tests very difficult to debug, because it makes impossible to debug the code step by step

@MojoQ
Copy link

MojoQ commented May 5, 2021

Our team is having this issue as well with 3.9.0. It's a major show-stopper. Would be great if it'd be fixed <3

@TimvdLippe
Copy link
Contributor

Rafael, does this mean we need to generate some more bytecode to make sure that this references are working as intended with the inline mockmaker?

@raphw
Copy link
Member

raphw commented May 5, 2021

I am afraid we are the wrong place to fix this. Byte Buddy needs to shift the local variable array to add Mockito's logic. For reasons (tm), this requires to move the this variable further to the left. It is however still there:

  LocalVariableTable:
    Start  Length  Slot  Name   Signature
       76       4     2  this   Lfoo/Sample;

IntelliJ could certainly pick this up and make the variable available in the debugger. Normally this is put at slot 0 but the JVM does not require this at all. I think IntelliJ falsely assumes this. If I rename the variable to this2, it is even displayed but that would be inconsistent for other parsing processors. This needs to be fixed in IntelliJ.

@TimvdLippe
Copy link
Contributor

Thank you for the explanation! Does anybody want to help us out filing this bug for IntelliJ?

@jwb441
Copy link

jwb441 commented May 5, 2021

Would something similar need to be done to get this fixed in eclipse?

thank you for looking at it!

@raphw
Copy link
Member

raphw commented May 6, 2021

Possibly, I have not used Eclipse in a while. This might even relate to the debugger API.

@alblue
Copy link

alblue commented May 6, 2021

As per twitter conversation:

The JVM spec says that for instance methods, the "this" reference is sorted in slot 0 on a stack frame:

https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.6.1

"On instance method invocation, local variable 0 is always used to pass a reference to the object on which the instance method is being invoked (this in the Java programming language)."

This is hard coded into the Java debug protocol which is why IDEs like IntelliJ and Eclipse report specific issues with "this" but not other variables; there's a specific "thisObject" call in the wire protocol (as opposed to a local variable which happens to have the name "this"

https://github.com/openjdk/jdk/blob/master/src/jdk.jdwp.agent/share/native/libjdwp/StackFrameImpl.c#L378

https://docs.oracle.com/en/java/javase/11/docs/api/jdk.jdi/com/sun/jdi/StackFrame.html#thisObject

@raphw
Copy link
Member

raphw commented May 6, 2021

Why do IDEs not process local variables named 'this' though? It's the very same value, would not make a difference for the user and the verifier certainly considers the byte code legal.

It is certainly true that variables are passed on their respective index but they are commonly reassigned within the method what is what happens here.

@alblue
Copy link

alblue commented May 6, 2021

Because in Java, at least, you can't create a variable called "this" and in fact it's a synthetic construct generated by the java compiler as a shorthand for local[0].

Secondly the local variable table is optional which can be skipped when generating code using -g:none. In this circumstance it is necessary for the debugger to be able to resolve what the object instance is instead of looking up by name.

Thirdly it's what is defined in the debug api which is what IDEs use and which is baked into various assumptions and places within the JVM, including various JIT related operations. This may not be an issue in this case (particularly if it's not called frequently) but for performance reasons the calls to other instance methods will passthrough local[0] implicitly, regardless of what the local variable table has in it.

TL;DR "this" isn't a variable in the normal sense and changing the LVT isn't sufficient to make IDEa believe that it is.

@raphw
Copy link
Member

raphw commented May 6, 2021

'this' only exists as a Java keyword and you are feee to call it whatever in the language, you like, also in the LVT. The zero slot is in no way privileged. The verifier already tracks if the slot is reassigned to avoid super calls on other identities then the currently dispatched instance. At the same time, it allows such calls if 'this' is reassigned to another slot later from that targeted slot.

If I am reading the spec correctly, this is also perfectly legal. It only defines on what slot 'this' is passed to a method. I do neither think such reassignment is affecting the JIT where 'this' is no longer a special variable once the verifier is satisfied.

Furthermore, Byte Buddy retains the 'this' variable at slot 0. As a matter of fact, that's the point of it. It makes a defensive copy of the original array to function when obfuscators or alternative JVM languages are used which sometimes reassign slots that are no longer used in the code. Therefore, I am wondering what both IDEs are doing if this is both available at slot 0 and on the copied slot that is referrenced in the LVT. It would be available from both locations.

@raphw
Copy link
Member

raphw commented May 7, 2021

I added an exemption for remapping the this variable's index. As it is assumed immutable, that exemption works for the this variable only but it seems to yield the correct result in the debugger. All other variables are still remapped what is necessary since they might be mutated but the debugger seems to accept this.

In this context I noticed another unfortunate break point confusion. Since we prepend the line number to the very beginning of any method in order to add that line number to exceptions we throw from mocked methods, the debugger will stop if a break point is placed in the very first line of a method, even if the mocked path of the inline-mocked method is dispatched. In this case, no variables (including this) are displayed since the real code is never invoked but the call gets redirected into Mockito's own infrastructure. @TimvdLippe I think we rather have the Mockito exceptions show the right line number rather then not break in the first line of a method in case that a break point is set there, but maybe we should document this somewhere?

@TimvdLippe
Copy link
Contributor

@raphw Yes I think that makes the most sense. We should probably add that caveat to https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#39

@iamkarlson
Copy link

Is there any way to hot fix this issue locally?

@TimvdLippe
Copy link
Contributor

@iamkarlson We first need a new release of ByteBuddy. Then we can roll that into Mockito and release Mockito as well. We can do that relatively quickly, so it's mostly blocked on the ByteBuddy release.

@jwb441
Copy link

jwb441 commented May 31, 2021

@TimvdLippe would that fix the eclipse as well as IntelliJ ?

@raphw
Copy link
Member

raphw commented Jun 1, 2021

It should fix both. A new release is underway in this minute.

@TimvdLippe
Copy link
Contributor

We will need to wait until 1.11.1 is on Maven Central before we can update our dependency in Mockito.

@TimvdLippe
Copy link
Contributor

It seems like the ByteBuddy release is experiencing some issues: https://github.com/raphw/byte-buddy/runs/2713960314

@raphw
Copy link
Member

raphw commented Jun 1, 2021

On it, javadoc does not like something, not reproducable locally but I'll find out what it is.

@raphw
Copy link
Member

raphw commented Jun 1, 2021

Syncing to Central now and should be available throughout the night. We'll update Mockito soon but in the mean time it should be sufficient to simply bump the Byte Buddy dependency to 1.11.1.

@TimvdLippe
Copy link
Contributor

I am waiting for #2312 to get merged (after review) and then we can upload the new version of Mockito to Maven Central.

@jwb441
Copy link

jwb441 commented Jun 2, 2021

Syncing to Central now and should be available throughout the night. We'll update Mockito soon but in the mean time it should be sufficient to simply bump the Byte Buddy dependency to 1.11.1.

I tried upping Byte Buddy to 1.11.1 and still recreated the issue. Will try when the never version of Mokito is uploaded.

thanks again!

@TimvdLippe
Copy link
Contributor

Mockito 3.11.0 is being pushed to Maven Central atm: https://github.com/mockito/mockito/actions/runs/903060692

@jwb441
Copy link

jwb441 commented Jun 3, 2021

I just took 3.11.0 and reran the test and unfortunately I am still seeing the same issue. Let me know if there is anything else that I can add.

@TimvdLippe
Copy link
Contributor

@jwb441 Could you double-check that you aren't accidentally using an older version of ByteBuddy via a different transitive dependency? I can retest this issue tomorrow.

@raphw
Copy link
Member

raphw commented Jun 3, 2021

I just tested it myself in IntelliJ, and it does now work as expected. There is one caveat, however. If you set the breakpoint in the first line of the method, it will:

a) not contain the this reference if stopped in the first line.
b) trigger the breakpoint for mocked method calls.

This is something we can hardly avoid, unfortunately. But we should certainly document it in the Mockito class.

@jwb441
Copy link

jwb441 commented Jun 3, 2021

@TimvdLippe and @raphw - I missed that part in the discussion so went back and added a few more lines to the method in the spied class and I noticed a few things. If I put the break point in the method that I'm calling it never gets tripped. When I put it in the test and step into it I'm getting the error on line one (expected) and then my next step always jumps to the end of the method - I do get 'this' at that point though.

@ciancheng
Copy link

still got the same error. I am using mockito-core 4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants