8280377: MethodHandleProxies does not correctly invoke default methods with varags #2235

Delawen · 2024-02-26T12:52:28Z

This is a backport of https://bugs.openjdk.org/browse/JDK-8280377 MethodHandleProxies does not correctly invoke default methods with varags

I applied the same fix that was applied to version 19 in openjdk/jdk@a183bfb

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
JDK-8280377 needs maintainer approval

Issue

JDK-8280377: MethodHandleProxies does not correctly invoke default methods with varags (Bug - P4 - Approved)

Reviewers

Andrew Haley (@theRealAph - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk17u-dev.git pull/2235/head:pull/2235
$ git checkout pull/2235

Update a local copy of the PR:
$ git checkout pull/2235
$ git pull https://git.openjdk.org/jdk17u-dev.git pull/2235/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2235

View PR using the GUI difftool:
$ git pr show -t 2235

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk17u-dev/pull/2235.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-26T12:52:46Z

👋 Welcome back Delawen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-26T12:54:12Z

This backport pull request has now been updated with issue from the original commit.

openjdk · 2024-02-26T12:54:42Z

⚠️ @Delawen This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

mlbridge · 2024-02-26T12:57:13Z

Webrevs

00: Full (12f832cc)

Delawen · 2024-02-26T14:09:25Z

/approval JDK-8280377 request Fix Request
This fix is a backport. I am applying the same fix that was done for version 19
It includes tests to make sure that when we call MethodHandleProxies, it does correctly invoke default methods both with and without varags

openjdk · 2024-02-26T14:09:56Z

@Delawen
JDK-8280377: The approval request has been created successfully.

GoeLin · 2024-02-27T19:50:48Z

Hi @Delawen, I think this is a larger change that should get some more testing than just checking that is fixes the issue.

Delawen · 2024-03-01T09:18:02Z

I tested with the Main.java example that is provided in the bug:

import java.lang.invoke.MethodHandleProxies;
import java.lang.invoke.MethodHandles;

import static java.lang.invoke.MethodType.methodType;

public class Main {

    public interface Interface
    {
        void method(Object... args);
        default void defaultMethod(Object... args) {
            method(args);
        }
        default void defaultMethod2(Object[] args) {
            method(args);
        }
    }

    private static void callMeViaMethodHandles(String foo, int bar) {
        System.out.println("foo=" + foo + ", bar=" + bar);
    }

    public static void main(String[] args) throws NoSuchMethodException, IllegalAccessException
    {
        var mh = MethodHandles.lookup().findStatic(
                Main.class, "callMeViaMethodHandles", methodType(void.class, String.class, int.class)
        );
        mh = mh.asSpreader(Object[].class, mh.type().parameterCount());
        var proxy = MethodHandleProxies.asInterfaceInstance(Interface.class, mh);

        // throws incorrect exception
        proxy.defaultMethod("should be foo", 3);
        
        // works fine
        proxy.defaultMethod2(new Object[] { "should be foo", 3 });
    }
}

With the regular Java 17 installed on my Debian, I get the expected error pre-fix (I also tested with a local build of latest master at the time, but as I built with the fix, I cannot provide right now the output for that):

$ javac Main.java ; java --version; java Main
openjdk 17.0.10 2024-01-16
OpenJDK Runtime Environment (build 17.0.10+7-Debian-1deb12u1)
OpenJDK 64-Bit Server VM (build 17.0.10+7-Debian-1deb12u1, mixed mode, sharing)
Exception in thread "main" java.lang.IllegalArgumentException: array is not of length 2
	at java.base/java.lang.invoke.MethodHandleStatics.newIllegalArgumentException(MethodHandleStatics.java:167)
	at java.base/java.lang.invoke.MethodHandleImpl.checkSpreadArgument(MethodHandleImpl.java:584)
	at java.base/java.lang.invoke.MethodHandleProxies$1.invoke(MethodHandleProxies.java:198)
	at jdk.proxy1/com.sun.proxy.jdk.proxy1.$Proxy0.method(Unknown Source)
	at Main$Interface.defaultMethod(Main.java:12)
	at java.base/java.lang.invoke.MethodHandleProxies.callDefaultMethod(MethodHandleProxies.java:354)
	at java.base/java.lang.invoke.MethodHandleProxies$1.invoke(MethodHandleProxies.java:205)
	at jdk.proxy1/com.sun.proxy.jdk.proxy1.$Proxy0.defaultMethod(Unknown Source)
	at Main.main(Main.java:32)

After fixing it, I get the correct answer:

$ ~/git/jdk17u-dev/build/linux-x86_64-server-release/jdk/bin/javac Main.java && ~/git/jdk17u-dev/build/linux-x86_64-server-release/jdk/bin/java --version && ~/git/jdk17u-dev/build/linux-x86_64-server-release/jdk/bin/java Main
openjdk 17.0.11-internal 2024-04-16
OpenJDK Runtime Environment (build 17.0.11-internal+0-adhoc.delawen.jdk17u-dev)
OpenJDK 64-Bit Server VM (build 17.0.11-internal+0-adhoc.delawen.jdk17u-dev, mixed mode)
foo=should be foo, bar=3
foo=should be foo, bar=3

Delawen · 2024-03-01T09:19:55Z

get some more testing than just checking that is fixes the issue.

@GoeLin I am going to test other things with my local build with the fix, like running other Java apps. Any suggestions on what should I try to make sure it works, besides the different make tests and try to run for example IntellijIDEA or other Java apps?

Delawen · 2024-03-01T09:25:43Z

The commit contains a test for the different ways of calling a method with varargs: https://github.com/openjdk/jdk17u-dev/pull/2235/files#diff-41008904ed7aa596f5de5930e082a8cf6b239603883818fa940bb04ff511455aR65-R68

But I understand this may be a "too simple" example.

Delawen · 2024-03-01T15:05:39Z

@mlchung You did the original commit for this fix. Can you give me some ideas on how to properly test this backport and make sure I'm not breaking anything else?

// cc @AlanBateman @DasBrain @liach as you were also involved on the original PR

DasBrain · 2024-03-01T15:21:52Z

I did not read through the entire patch - @mlchung did comment on the bug report back then:

Alternative fix for backport consideration:

@@ -343,7 +343,7 @@ public class MethodHandleProxies {
                         intfc, mk.getName(),
                         MethodType.methodType(mk.getReturnType(), mk.getParameterTypes()),
                         self.getClass());
- return mh.asSpreader(Object[].class, mk.getParameterCount());
+ return mh.asFixedArity().asSpreader(Object[].class, mk.getParameterCount());

https://bugs.openjdk.org/browse/JDK-8280377?focusedId=14471119&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14471119

mlchung · 2024-03-05T20:02:00Z

The backport looks good to me. This is the same fix for JDK-8280377 and includes the regression test. Tier1-3 tests including this new regression test should provide adequate verification.

liach · 2024-03-05T20:50:06Z

The alternative backport fix is for JDK versions before 16 when Proxy.invokeDefault was not available, right?

mlchung · 2024-03-05T20:53:05Z

The alternative backport fix is for JDK versions before 16 when Proxy.invokeDefault was not available, right?

Yes. Proxy::invokeDefault API was introduced in Java 16.

Delawen · 2024-03-07T09:39:16Z

src/java.base/share/classes/java/lang/reflect/InvocationHandler.java

@@ -262,33 +261,6 @@ public static Object invokeDefault(Object proxy, Method method, Object... args)
            throws Throwable {
        Objects.requireNonNull(proxy);
        Objects.requireNonNull(method);
-


This removed piece of code is moved to Proxy.invokeDefault (which is now called at the end of this removed code). The code moved is mostly the same, except it checks if the caller object is null. If it is null, then it won't execute anything.

Delawen · 2024-03-07T09:40:02Z

src/java.base/share/classes/java/lang/reflect/InvocationHandler.java

-            // unwrap and throw the exception thrown by the default method
-            throw e.getCause();
-        }
+        return Proxy.invokeDefault(proxy, method, args, Reflection.getCallerClass());


This is where we call the previously removed piece of code.

Delawen · 2024-03-07T09:42:02Z

src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java

@@ -202,7 +202,8 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
                    if (isObjectMethod(method))
                        return callObjectMethod(proxy, method, args);
                    if (isDefaultMethod(method)) {
-                        return callDefaultMethod(defaultMethodMap, proxy, intfc, method, args);
+                        // no additional access check is performed
+                        return JLRA.invokeDefault(proxy, method, args, null);


As defined at the end of the file, JavaLangReflectAccess JLRA = SharedSecrets.getJavaLangReflectAccess(); which is an interface implemented by ReflectAccess.java (modified below)

Delawen · 2024-03-07T09:45:56Z

src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java

@@ -320,37 +321,5 @@ private static boolean isDefaultMethod(Method m) {
        return !Modifier.isAbstract(m.getModifiers());
    }

-    private static boolean hasDefaultMethods(Class<?> intfc) {


This piece of code is removed because it is no longer used. Now, instead of calling callDefaultMethod(...), we are calling Proxy.invokeDefault(...) which is created on this same PR.

Delawen · 2024-03-07T09:49:12Z

src/java.base/share/classes/java/lang/reflect/Proxy.java

+     * @throws IllegalAccessException if the proxy interface is inaccessible to the caller
+     *         if caller is non-null
+     */
+    static Object invokeDefault(Object proxy, Method method, Object[] args, Class<?> caller)


This is the piece of code that will now unify calling methods both from MethodHandleProxies (where this code is copied from) and ReflectAccess, which was using a different way of calling methods.

theRealAph

OK, I see now. Thanks for the explanation.

Delawen · 2024-03-11T09:59:46Z

/integrate

Delawen · 2024-03-11T09:59:55Z

Although I miss the bot saying something

Delawen · 2024-03-11T10:03:30Z

Is that what you needed, bot?

openjdk · 2024-03-11T10:04:15Z

@Delawen
JDK-8280377: The approval request has been updated successfully.

GoeLin · 2024-03-12T09:11:42Z

The backport looks good to me. This is the same fix for JDK-8280377 and includes the regression test. Tier1-3 tests including this new regression test should provide adequate verification.

Hi @Delawen, did you run some tests as e.g. proposed here?

openjdk · 2024-03-13T19:57:41Z

@Delawen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8280377: MethodHandleProxies does not correctly invoke default methods with varags

Reviewed-by: aph

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 111 new commits pushed to the master branch:

4ececad: 8306714: Open source few Swing event and AbstractAction tests
151091c: 8305942: Open source several AWT Focus related tests
996dfb0: 8305943: Open source few AWT Focus related tests
615c01b: 8297292: java/nio/channels/FileChannel/FileExtensionAndMap.java is too slow
8e132c4: 8163229: several regression tests have a main method that is never executed
7680369: 8296610: java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java failed with "BindException: Address already in use: connect"
e59eeb0: 8297645: Drop the test/jdk/java/net/httpclient/reactivestreams-tck-tests/TckDriver.java test
b4e64ff: 8296137: diags-examples.xml is broken
b37df14: 8163921: HttpURLConnection default Accept header is malformed according to HTTP/1.1 RFC
1e777ec: 8328165: improve assert(idx < _maxlrg) failed: oob
... and 101 more: https://git.openjdk.org/jdk17u-dev/compare/a59c2ebfc06b0b39c78e4879daa767e4dabd3be9...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@theRealAph) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Delawen · 2024-03-14T12:46:08Z

Hi @Delawen, did you run some tests as e.g. proposed here?

To be completely honest, I am having issues running tier2+ tests on my laptop, I am looking for an external infra to run them.

Delawen · 2024-03-15T08:46:23Z

@smlambert is using this PR branch for polishing the Trestle testing pipeline.

The results are here: adoptium/aqa-tests#5137 (comment)

Not all architectures pass, but (at least right now) not because of this PR, but because misconfigurations.

// cc @GoeLin

Delawen · 2024-03-20T09:59:57Z

Do we need more testing here?

jerboaa · 2024-03-27T16:17:52Z

Do we need more testing here?

I don't think we do. We have positive tier 1-3 results and the regression test. It fixes a real bug. Patch is reviewed, even though it's clean.

jerboaa · 2024-03-27T16:18:11Z

/approve yes

openjdk · 2024-03-27T16:18:44Z

@jerboaa
8280377: There is no maintainer approval request for this issue.

Delawen · 2024-04-01T07:48:56Z

/approval JDK-8280377 request Fix Request
This fix is a backport. I am applying the same fix that was done for version 19. From the two ways of calling a method via interface with a variable number of arguments (array or varargs), it now uses the same way for both, with the code moved to Proxy.java class.
It includes tests to make sure that when we call MethodHandleProxies, it does correctly invoke default methods both with and without varags

openjdk · 2024-04-01T07:49:22Z

@Delawen
JDK-8280377: The approval request was already up to date.

Delawen · 2024-04-02T08:32:39Z

/integrate

openjdk · 2024-04-02T08:33:44Z

@Delawen
Your change (at version 12f832c) is now ready to be sponsored by a Committer.

jerboaa · 2024-04-02T08:39:04Z

/sponsor

openjdk · 2024-04-02T08:39:39Z

Going to push as commit 5ecac7a.
Since your change was applied there have been 111 commits pushed to the master branch:

4ececad: 8306714: Open source few Swing event and AbstractAction tests
151091c: 8305942: Open source several AWT Focus related tests
996dfb0: 8305943: Open source few AWT Focus related tests
615c01b: 8297292: java/nio/channels/FileChannel/FileExtensionAndMap.java is too slow
8e132c4: 8163229: several regression tests have a main method that is never executed
7680369: 8296610: java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java failed with "BindException: Address already in use: connect"
e59eeb0: 8297645: Drop the test/jdk/java/net/httpclient/reactivestreams-tck-tests/TckDriver.java test
b4e64ff: 8296137: diags-examples.xml is broken
b37df14: 8163921: HttpURLConnection default Accept header is malformed according to HTTP/1.1 RFC
1e777ec: 8328165: improve assert(idx < _maxlrg) failed: oob
... and 101 more: https://git.openjdk.org/jdk17u-dev/compare/a59c2ebfc06b0b39c78e4879daa767e4dabd3be9...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-02T08:39:48Z

@jerboaa @Delawen Pushed as commit 5ecac7a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Backport a183bfb436a7dd998e602c2d16486e88c390fca1

12f832c

openjdk bot changed the title ~~Backport a183bfb436a7dd998e602c2d16486e88c390fca1~~ 8280377: MethodHandleProxies does not correctly invoke default methods with varags Feb 26, 2024

openjdk bot added backport clean labels Feb 26, 2024

openjdk bot added the rfr Pull request is ready for review label Feb 26, 2024

openjdk bot added the approval label Feb 26, 2024

openjdk bot removed the approval label Feb 29, 2024

Delawen commented Mar 7, 2024

View reviewed changes

theRealAph approved these changes Mar 8, 2024

View reviewed changes

openjdk bot added the approval label Mar 11, 2024

smlambert mentioned this pull request Mar 12, 2024

Verify initial Trestle pipelines against real-world examples of upstream PRs adoptium/aqa-tests#5137

Open

openjdk bot removed the approval label Mar 13, 2024

openjdk bot added approval ready Pull request is ready to be integrated and removed approval labels Apr 1, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 2, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 2, 2024

openjdk bot closed this Apr 2, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 2, 2024

Delawen deleted the backport-JDK-8280377 branch April 2, 2024 08:46

8280377: MethodHandleProxies does not correctly invoke default methods with varags #2235

8280377: MethodHandleProxies does not correctly invoke default methods with varags #2235

Conversation

Delawen commented Feb 26, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

mlbridge bot commented Feb 26, 2024

Webrevs

Delawen commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

GoeLin commented Feb 27, 2024

Delawen commented Mar 1, 2024 • edited Loading

Delawen commented Mar 1, 2024

Delawen commented Mar 1, 2024

Delawen commented Mar 1, 2024 • edited Loading

DasBrain commented Mar 1, 2024

mlchung commented Mar 5, 2024

liach commented Mar 5, 2024

mlchung commented Mar 5, 2024

Delawen Mar 7, 2024

Choose a reason for hiding this comment

Delawen Mar 7, 2024

Choose a reason for hiding this comment

Delawen Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Delawen Mar 7, 2024

Choose a reason for hiding this comment

Delawen Mar 7, 2024

Choose a reason for hiding this comment

theRealAph left a comment

Choose a reason for hiding this comment

Delawen commented Mar 11, 2024

Delawen commented Mar 11, 2024

Delawen commented Mar 11, 2024

openjdk bot commented Mar 11, 2024

GoeLin commented Mar 12, 2024

openjdk bot commented Mar 13, 2024 • edited Loading

Delawen commented Mar 14, 2024

Delawen commented Mar 15, 2024

Delawen commented Mar 20, 2024

jerboaa commented Mar 27, 2024

jerboaa commented Mar 27, 2024

openjdk bot commented Mar 27, 2024

Delawen commented Apr 1, 2024

openjdk bot commented Apr 1, 2024

Delawen commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

jerboaa commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

openjdk bot commented Apr 2, 2024

Delawen commented Feb 26, 2024 •

edited by openjdk bot

Loading

Delawen commented Mar 1, 2024 •

edited

Loading

Delawen commented Mar 1, 2024 •

edited

Loading

Delawen Mar 7, 2024 •

edited

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading