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

Wrong super constructor is called #582

Closed
Vampire opened this issue Jan 28, 2019 · 8 comments
Closed

Wrong super constructor is called #582

Vampire opened this issue Jan 28, 2019 · 8 comments

Comments

@Vampire
Copy link

@Vampire Vampire commented Jan 28, 2019

  • Version of JMockit that was used: 1.44 & 1.45

We had very strange behavior:

  • In a test we have a field @Mocked javax.faces.context.PartialResponseWriter writer;
  • In the constructor of another class used in that test we initialize a private field like responseWriter = new TestResponseWriter();
  • javax.faces.context.ResponseWriter which both classes extend from (one directly, one indirectly) has no explicit constructor, just the default implied no-arg constuctor
  • java.io.Writer on the other hand has two protected constructors, one no-arg and one with an Object parameter that must not be null

grafik

  • Now when we run our tests, we get
    java.lang.NullPointerException
    	at java.base/java.io.Writer.<init>(Writer.java:174)
    	at javax.faces.context.ResponseWriter.<init>(ResponseWriter.java)
    	at de.empic.web.testutils.TestResponseWriter.<init>(TestResponseWriter.java:14)
    
  • This on first look makes absolutely no sense, as there is no constructor in ResponseWriter that calls the one-arg constructor of Writer.
  • So I naturally kicked off the debugger to look what is going on, but suddenly it didn't happen anymore then. And this constantly stays like hat, if you run without debugger it fails with NPE, if you run with debug setting (even if you don't even attach a debugger), it runs fine

So after a couple of hours of adding stdout-debugging to JMockit and digging my way through subtle differences in the output, I got to the root cause.
In mockit.internal.expectations.mocking.MockedClassModifier#visitMethod you call generateCallToSuperConstructor().
I'm not sure why this is done, I guess so that you are able to then add the call to the MockingBridge, as the super-constructor always has to be called first.

The problem is, that your code calls a random super constructor with default values according to types. I'm not sure this is the best decision, as different constructors can ensure different invariants and so calling a random one might not be the best solution. Maybe it would better to look into the actual implementation of the constructor and calling the super-constructor the real method also is calling? But even then using default values for the arguments might not be the best solution.

In my case here, after adding

System.out.println("vvvvvvvvvvvvvvvvvvvvvvvv");
for (MethodInfo methodOrConstructor : cmr.getMethods()) {
   if (methodOrConstructor.isConstructor()) {
      System.out.println("methodOrConstructor.desc = " + methodOrConstructor.desc);
      System.out.println("methodOrConstructor.accessFlags = " + methodOrConstructor.accessFlags);
   }
}
System.out.println("^^^^^^^^^^^^^^^^^^^^^^^^");

to mockit.internal.SuperConstructorCollector#findConstructor I have seen that without debug I get

vvvvvvvvvvvvvvvvvvvvvvvv
methodOrConstructor.desc = (Ljava/lang/Object;)V
methodOrConstructor.accessFlags = 4
methodOrConstructor.desc = ()V
methodOrConstructor.accessFlags = 4
^^^^^^^^^^^^^^^^^^^^^^^^

whereas with debug I get

vvvvvvvvvvvvvvvvvvvvvvvv
methodOrConstructor.desc = ()V
methodOrConstructor.accessFlags = 4
methodOrConstructor.desc = (Ljava/lang/Object;)V
methodOrConstructor.accessFlags = 4
^^^^^^^^^^^^^^^^^^^^^^^^

and the code just takes the first one it finds.
In my case without debug this was the one-arg constructor that was given null as parameter and thus produced that NPE.

So one problem is, that it sometimes uses one constructor, sometimes the other, another problem probably is that actually it uses some random constructor that might have requirements or produces certain invariants.

@rliesenfeld

This comment has been minimized.

Copy link
Member

@rliesenfeld rliesenfeld commented Jan 30, 2019

Ok... So, the call to generateCallToSuperConstructor() is done to avoid a VerifyError (as the JVM requires any constructor body - except those in java.lang.Object - to begin by calling another).

The super constructor that gets called is not chosen at random, but it simply is the first one to be found in the classfile. The JVM spec doesn't require the order of methods/constructors to be preserved in the classfile (when a ".java" file is compiled to its ".class" file), but normally the order is preserved (and since <init>() comes first in Writer.java, that's the one that should always get chosen). I don't know how it happens, in your "without debug" case, that the <init>(Object) constructor comes first - it's surprising.

That said, it should not matter which super constructor is called (and with which arguments), since all the classes (with all methods and constructors), starting from the one in the @Mocked declaration and up to the one before Object, are getting mocked. Apparently, java.io.Writer is getting unexpectedly unmocked in your environment, since the real Writer(Object) constructor gets executed (I can't reproduce the problem myself).

Ideally, yes, the same constructor that gets called from the unmocked constructor body should be the one to be called. Or at least, the accessible super constructor with the fewer parameters. So, an enhancement could be made to avoid situations like this. It would help to a have a reproducible test case, though.

@andi5

This comment has been minimized.

Copy link

@andi5 andi5 commented Jan 30, 2019

You can find a (hopefully) reproducible test case at https://github.com/andi5/jmockit-wrong-super

@Vampire

This comment has been minimized.

Copy link
Author

@Vampire Vampire commented Jan 30, 2019

I don't think there is a problem when the constructor is actually mocked.
As you can see in my description and in andis reproducer (thanks andi), the instantiation of the mocked subclass PartialResponseWriter works without a problem.

But when instantiating the non-mocked subclass TestResponseWriter in the same context, it fails.
Because then the rewritten constructor call also is used and is actually executed.
So I guess any constructor found through guessing is wrong and even if the correct constructor would be found, the default parameters would break it eventually. The only rock-solid case probably is, if there is only one constructor and it has no parameters, as then not much can go wrong. For all other cases it is like russian roulette.

I guess what might work is, to search in the original constructor the original super constructor call and copy it as first instruction, including the arguements given and then continue.
If the call is mocked, it doesn't matter, if it is not mocked, hopefully the correct things happen with the correct arguments.

@Vampire

This comment has been minimized.

Copy link
Author

@Vampire Vampire commented Jan 30, 2019

And btw. I just now also tested with different Java version, with Java 10.0.2 it does not happen, with 11.0.1 it is happening.

@Vampire

This comment has been minimized.

Copy link
Author

@Vampire Vampire commented Jan 30, 2019

While of course even when the exception does not happen, potentially the wrong constructor is used generally. If I e. g. the ResponseWriter class would call the one-arg super-constructor of Writer, after instrumentation it would call the no-arg constructor, even for non-mocked calls and thus could lead to faulty and unexpected behavior.

@Vampire

This comment has been minimized.

Copy link
Author

@Vampire Vampire commented Feb 18, 2019

Unfortunately the fix you did is not enough.
While it fixes the one case we described, without the logic I suggested you still change the logic of non-mocked classes as described.
I extended the test from Andreas a bit: https://github.com/Vampire/jmockit-wrong-super
You can see that with your latest changes, the original NPE (as would have happened in test1_2) does not happen anymore as you search for the constructor with the fewest arguments and in case there is a no-arg constructor, it works now for that specific case.

But as you can see in test2_2 and test3_2 you still modify how non-mocked classes work, in case of test3_2 you again get the NPE, in case of test2_2 you violate the checked invariant.

With your fix-commit for this, it should also consistently fail with and without debug and on all Java versions as now the selected constructor does not depend on the order of constructors anymore.

@rliesenfeld

This comment has been minimized.

Copy link
Member

@rliesenfeld rliesenfeld commented Feb 19, 2019

Those cases will be handled when #492 gets resolved.

@Vampire

This comment has been minimized.

Copy link
Author

@Vampire Vampire commented Feb 19, 2019

I see, thanks for the info.

@jmockit jmockit locked and limited conversation to collaborators Apr 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants
You can’t perform that action at this time.