StackOverflowError when calling super and overriding a package protected java method

Here is what I found. In MetaClassImpl.MethodIndexAction#methodNameAction(),

1. If you use a protected modifier in the super class method, the mopMethods list contains the $super method. If the modifier is removed, the method disappears from mopMethods.

1. plus, the following test fails because of the modifier test (line 460) :

   (useThis ^ (method.getModifiers() & (Modifier.PUBLIC | Modifier.PROTECTED)) == 0)

So it seems there are two problems to solve : the fact that the super method is missing from the MOP methods and the test which forgets about package protected methods. I'm not sure this can be fixed without side effects.

I recommend changing the Priority to Blocker.

Hamlet,

Here is my test case for this bug. In the thread, you say that you have the error even with a non package protected method. Could you update the test class with an example of such a behaviour ? Thank you !

package groovy.bugs;

/**
 * Support class for GROOVY-4922 bug. Must be written in Java.
 */
class StackOverflowBugSupport {
    protected String support;

    void someMethod(String value) {
        support = value;
    }
}

package groovy.bugs


class GroovyStackOverflowBug extends GroovyTestCase {
    void testShouldNotThrowStackOverflow() {
        def child = new Child()
        child.someMethod("value")
        assert child.support == "value"
    }


    class Child extends StackOverflowBugSupport {
        protected void someMethod(String parameter) {
            super.someMethod(parameter)
        }
    }
}

I'll update this ticket if I find anything relevant meanwhile.

Cedric be careful and keep inner classes out of this, they may have differing semantics here and there. not that its necessarily intentional, but that would be a different bug then

Ok, I'll be testing both. I have a patch which solves the problem by adding package protected methods to the list of generated super$ methods. Doing so (and updating the modifiers test too), I can work around the stack overflow, but right now it seems to break the build (3 tests fail). I don't know if it's related to my changes or not, since I didn't do a clean build before patching, but this bug is the kind of ones which could have serious side effects.

I'll tell you as I advance, I'll have 2 hours of train where I'll be able to play around

most probably this is the right way to solve the issue. it would be good to (a) attach the not yet complete patch here and (b) to show what 3 tests fail. Normally I wouldn't be pressing, but we intend to release tomorrow and I would like this issue here to be solved till then

Here is the patch and the test case results. I won't be able to deliver a full fix until tomorrow because I won't be home, so feel free to complete.

Sorry I can't give you more information, I have to take my train now

The following patch (new) passes all tests on trunk. Basically, I moved my "Child" test class to its own file and made "shouldNotCompile" in GenericsTestBase protected. However, at first glance, it should have worked even without the latter modification. I would be great if Hamlet could test it.

I modified your test Cedric to test for private directly instead of excluding others. That did let the tests pass. So I am closing the issue for now

I still have this error when using 1.8.x branch and the trunk.

The fix requires that you recompile the child class. Did you do so ? If yes then it is probably a separate bug and it would help if you could update the unit test to highlight your use case.

here is a patch showing that this still fails. You probably fixed it for protected methods but not for package-private methods.

I think I am missing information here. The test I added uses a package private method in the java super class. In the test the child class uses a protected method to overwrite this. In the patch from Hamlet it is a public method. Oh I see the test case is not ending with Test, so maybe it is not picked up.

oops (closed wrong issue), so reopen

ok, seems not to be the issue.

the example from the list is causing an illegal access error in 1.7.11, since the class is package private. Same for trunk

and any version since 1.5.1 I tested has the same problem

Yes there are two separate problems in the latest patch from Hamlet :

1. the field "value" in the parent class is package protected and not recognized in the subclass
2. it throws a StackOverflowError while my example does not

While I'm still scratching my head wondering why his example throws the error and not mine, I think the 1st one should be considered as a separate bug.

I just tried it with the precompilation as suggested and this does indeed reproduce the issue... only 1.7.0 has exactly the same problem. And even 1.5.1 has this problem. So we are far away from a regression

When CodeNarc users run CodeNarc with 1.7, they never receive a stack overflow exception. When they run it with CodeNarc 1.8 they do. While this specific scenario in the use case may not be a regression, there is something in Groovy 1.8 that causes this. For instance, if Groovy provides a Java class called "ClassCodeVisitorSupport" and the visibility modifier of a method was changed from protected to package-visibile (which should be a non-breaking change), then there is a chance that this is introduced, correct? There is definitely a regression somewhere. If we don't fix this ticket, then I will start looking for changed visibility modifiers that need to be reverted... because from a user perspective there are stackoverflow errors in 1.8 that are not in 1.7.

changing from protected to package-private is a breaking change in Java. A subclass in a different package can see protected methods, but not package-private methods. The problem is that I don't have a test case. The one from here is not exhibiting the special problem, the zip from the list also not. Without test case I cannot reproduce the issue. Since it seems that you can reproduce the issue, I guess bisecting is the one way to identify the commit that introduced that. You can of course also look for changed modifiers. In any case we will then have to discuss why this is a problem.

It is too bad that the stackoverflow example you gave is not so easy to debug here

what is wrong with the failing test case I attached? that reproduces the error every time for me?

Hamlet,

Yes, I can reproduce the error with your test case. I think what Jochen says is that this test does not correspond to the error you initially reported which is related to the ClassCodeVisitorSupport. There are two things mixed up here (well, I could identify more) : the regression which was introduced somewhere in ClassCodeVisitorSupport or any other class, and the bug which reproduces the StackOverflowError here, but which is not a regression as it could be identified in every version of Groovy tested so far.

Here's the result of my late investigations :

• the initial patch works if the superclass is public and a method is package private and the child class is defined in its own file (bug #1, fixed)
• if you remove the "public" modifier from the superclass definition, it fails with "class groovy.bugs.Groovy4922BugChild cannot access its superclass groovy.bugs.Groovy4922BugSupport" (bug #2, not fixed)
• if you remove the "protected" modifier on the "value" property of the superclass, it fails with "groovy.lang.MissingPropertyException: No such property: support for class: groovy.bugs.Groovy4922BugChild" (bug #3, not fixed but less critical)
• if you define the child class directly in the test case, like you did Hamlet, instead of in the "assertScript" section, it fails with a StackOverflowError (bug #4)

So, I would say your initial error is bug #5, which could be a modifier changed somewhere in the ClassCodeVisitorSupport class or its hierarchy between 1.7 and 1.8.

There's still some work to fully understand what's going on there

Bug #4 - This is the most direct representation of the CodeNarc code that is seeing the regression. i suggest focusing on this use case.

Bug #5 - "CodeNarc use case" - CodeNarc subclasses ClassCodeVisitorSupport, and the overflow comes out of the overridden "void visitDeclarationExpression(DeclarationExpression expression)". This method is overridden in CCVS in 1.7 but not in 1.8. The method isn't on the class in 1.7, it is higher up in the hierarchy, but it is on the class in 1.8. Perhaps this is causing the issue, in which case the method should be added back on.

Regarding whether this is a regression or not... No CodeNarc users see a StackOverflowError using 1.7 and almost all users see it with 1.8. If we're finding defects that are present in 1.7 and earlier then those are nice to fix, but not the root cause of the CodeNarc exceptions.

to clarify things... the method is not overriden in ClassCodeVisitorSupport in 1.7, but is in 1.8 and 1.9. The method is also overridden in CodeVisitorSupport, the first place in 1.7 and the second case in 1.8/1.9, the class is higher up in the hierarchy.

The CodeNarc problem is then fully unrelated to modifiers, since the method is public only.

The next step would be to reproduce the issue using overridden public methods only. And that is a setup that does not exist as of yet.

since the CodeNarc problem seems to be a different one than the one here I created GROOVY-4936. If GROOVY-4936 is fixed we can test if this issue is resolved as well - by chance.

GROOVY-4936 is now fixed, it would be good to know if that fixed the original issue here as well.

I retested the scenario in the .zip file that I uploaded, and that is fixed.
However, I retested against CodeNarc and that is not fixed.
I will try to make a new small, self-executing example that shows the problem.

I was afraid that may be the case, that's why I made the other issue

I attached a new self executing stack overflow example.
This shows it broken when using the ast visitors.

Isn't example the same as before?

It is slightly different and the example extends ClassCodeVisitorSupport the way our production code does.

still, when I execute your "run.sh" it works here with trunk and 1.8

Does your command line environment call the Groovy 1.7 groovyc?
If you have Groovy 1.8 in your path then the sample runs fine.
You need to make sure that groovyc dispatches to the 1.7 version.

compiled with 1.7.11 and run with 1.8.1 works fine

compiled with 1.7.8 and run with 1.8.1 does not work, but run with 1.7.8 does also not

comparing the bytecode generated by 1.8.1 and 1.7.8 I can see that it simply cannot work. there is a method

  public super$2$someMethod(Ljava/lang/String;)V
    ALOAD 0
    ALOAD 1
    INVOKESPECIAL Parent.someMethod (Ljava/lang/String;)V
    RETURN
    MAXSTACK = 2
    MAXLOCALS = 2

needed to call the super method, which in 1.7.8 does simply not exist. That is also the reason why compiled with 1.7.8 and run with 1.7.8 cannot work as well. 1.7.11 fixes the problem as does 1.8.1. But there is no chance that code compiled with 1.7.8 will ever run correctly like this. And there is nothing we can fix, since the support method needed for this is missing.

for CodeNarc you could look at the bytecode of the class containing the super method call. Does it have a method like above?

Here is the "new" stack overflow example. You and I are clearly working from different examples.

ok, the important difference between 1.7 and 1.8/1.9 is in 1.7 we have

  public super$2$visitDeclarationExpression(Lorg/codehaus/groovy/ast/expr/DeclarationExpression;)V
    ALOAD 0
    ALOAD 1
    INVOKESPECIAL org/codehaus/groovy/ast/CodeVisitorSupport.visitDeclarationExpression (Lorg/codehaus/groovy/ast/expr/DeclarationExpression;)V
    RETURN
    MAXSTACK = 2
    MAXLOCALS = 2

and in 1.8 we have

  public super$3$visitDeclarationExpression(Lorg/codehaus/groovy/ast/expr/DeclarationExpression;)V
    ALOAD 0
    ALOAD 1
    INVOKESPECIAL org/codehaus/groovy/ast/ClassCodeVisitorSupport.visitDeclarationExpression (Lorg/codehaus/groovy/ast/expr/DeclarationExpression;)V
    RETURN
    MAXSTACK = 2
    MAXLOCALS = 2

As can be clearly see there is a difference in what class is called as well as what number is used

From the trace I can tell the 1.7 method is not called. I assume the runtime looks only for the exact match, which for 1.8 is super$3 and not super$2, thus not finding the method and then placing the normal method for the call to super, when it should have caused an exception instead

If my assumption with the fall back is right, then GROOVY-5285 is caused by the exact same logic

GROOVY-5285 is unrelated. Another stack overflow

Fixed by testing if other MOP methods exist with the same name but with a lower index.