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

Avoid StackOverflowError in TypeUtils#resolveGenerics #9858

Merged

Conversation

dodgex
Copy link
Contributor

@dodgex dodgex commented Oct 21, 2023

While the issue (#9855) is related to JDK21 and its new getFirst and getLast methods in the List/SequencedCollection interfaces, the current behaviour can in theory cause problems on JDK versions before 21 too.

With this fix the TypeUtils#resolveGenerics method falls back to TypeUtils#ensureBaseType (already used method-level type variables) when a type variable is used in the same class where it has been defined.

This class for example, currently causes a StackOverflowError. With this PR the error is avoided, and the T is resolved as java.lang.Object

class BrokenClass<T> {
    public T getT() {
        return null;
    }
}

Fixes #9855

Fall back to the result of `ensureBaseType` when the generic variable is declared in the same class as it is used.
@niloc132
Copy link
Contributor

Thanks! Are you aware of any other issues preventing Java 21 from building and passing tests, or can we also add Java 21 to the nightly CI runs?

@dodgex
Copy link
Contributor Author

dodgex commented Oct 21, 2023

Until now I did not build GWT on JDK21. But in a first build I just did, it fails due to errorprone beeing too old. Version v2.20.0 added support for JDK21 (ea builds), latest is v2.23.0

@dodgex
Copy link
Contributor Author

dodgex commented Oct 21, 2023

Okay, updating errorprone might not be possible. At least not easy?

It requires JDK11 starting with v2.11.0. And in v2.22.0 they mention that they consider to require JDK 17 in a future release.

Either CI runs need to drop running on Java 8, or there has to be a way to use diffrent versions of errorprone per JDK version.

But I think this discussion deservers its own issue at this point. :)

@niloc132
Copy link
Contributor

Thanks for the investigation. I'll see about manually running more tests on Java 21 to validate this fix, and after this release we'll be dropping support for building GWT or running the compiler on Java 8. We may keep Java 8 support for the server, it will depend on what kind of pushback we get on other changes.

@niloc132
Copy link
Contributor

Running AutoBeanSuite on Java 21 reproduces this issue:

Testcase: testSplittable took 0.055 sec
	Caused an ERROR
java.lang.StackOverflowError
java.lang.RuntimeException: java.lang.StackOverflowError
	at com.google.web.bindery.autobean.vm.impl.ProxyAutoBean.traverseProperties(ProxyAutoBean.java:328)
	at com.google.web.bindery.autobean.shared.impl.AbstractAutoBean.traverse(AbstractAutoBean.java:166)
	at com.google.web.bindery.autobean.shared.impl.AbstractAutoBean.accept(AbstractAutoBean.java:101)
	at com.google.web.bindery.autobean.shared.impl.AutoBeanCodexImpl.doEncode(AutoBeanCodexImpl.java:560)
	at com.google.web.bindery.autobean.shared.AutoBeanCodex.encode(AutoBeanCodex.java:83)
	at com.google.web.bindery.autobean.shared.AutoBeanCodexTest.checkEncode(AutoBeanCodexTest.java:453)
	at com.google.web.bindery.autobean.shared.AutoBeanCodexTest.testSplittable(AutoBeanCodexTest.java:433)

While we can't build GWT with 21 yet, you can first do an ant clean dist-dev with Java 11/17, then cd into user/ and run something like

ant test '-Dgwt.junit.testcase.includes=**/AutoBeanSuite.class' '-Dgwt.nongwt.testcase.includes=**/AutoBeanSuite.class'

I confirmed that this patch fixed those tests, then ran ant test.web.htmlunit '-Dgwt.junit.testcase.includes=com/google/web/bindery/**/*Suite.class' in user/ with Java 21 to make sure we had no other surprises (from this patch or from Java 21 in general). That too passed - I'm now waiting locally for a full test.web.htmlunit run on all suites, but expect a pass from that too.

Copy link
Contributor

@niloc132 niloc132 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of test.web.htmlunit passed in Java 21, marking as approved, will merge in a day or so as usual to give time for any more discussion.

@niloc132 niloc132 merged commit 105e63d into gwtproject:main Nov 13, 2023
3 checks passed
@dodgex dodgex deleted the 9855-fix-stackoverflow-in-resolveGenerics branch November 13, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants