8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown #26335

plummercj · 2025-07-15T22:07:40Z

Fix how ThreadReference.popFrame() and ThreadReference.forceEarlyReturn deal with JDWP OPAQUE_FRAME error.

Before virtual threads, OpaqueFrameException did not exist and these API always threw NativeMethodException when JDWP OPAQUE_FRAME error was returned. For virtual threads OpaqueFrameException was added to handle the case where a virtual thread was not suspended at an event, so the JDI implementation was updated to throw OpaqueFrameException if it detected that a native method was not the cause. It turns out however that JVMTI (and therefore JDWP) can return OPAQUE_FRAME error for reasons other than a native method or the special virtual thread case, and for platform threads we were incorrectly throwing NativeMethodException in these cases. This PR fixes that. For platform threads we now only throw NativeMethodException if a native method is detected, and otherwise throw OpaqueFrameException.

The spec language is also being cleaned up to better align with JVMTI. Rather than calling out all the reasons for OpaqueFrameException, a more generic explanation is given.

This is somewhat of a preliminary PR so I can get some feedback. I still need to do a CSR and complete testing.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8363896 to be approved

Issues

JDK-8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown (Enhancement - P4)
JDK-8363896: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown (CSR)

Reviewers

Alex Menkov (@alexmenkov - Reviewer) Review applies to 05cce179
Serguei Spitsyn (@sspitsyn - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26335/head:pull/26335
$ git checkout pull/26335

Update a local copy of the PR:
$ git checkout pull/26335
$ git pull https://git.openjdk.org/jdk.git pull/26335/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26335

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26335.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-07-15T22:08:26Z

👋 Welcome back cjplummer! 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 · 2025-07-15T22:08:38Z

@plummercj 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:

8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown

Reviewed-by: sspitsyn, alanb, amenkov

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 359 new commits pushed to the master branch:

6fb6f3d: 8361638: java.lang.classfile.CodeBuilder.CatchBuilder should not throw IllegalArgumentException for representable exception handlers
44b19c0: 8365532: java/lang/module/ModuleReader/ModuleReaderTest.testImage fails
a65f200: 8365512: Replace -Xcomp with -Xmixed for AOT assembly phase
... and 356 more: https://git.openjdk.org/jdk/compare/9bef2d1610647dec18f9e81cbac3dddbbf99dd6d...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

plummercj · 2025-07-15T22:08:52Z

/csr needed

openjdk · 2025-07-15T22:09:22Z

@plummercj has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@plummercj please create a CSR request for issue JDK-8309400 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2025-07-15T22:09:53Z

@plummercj The following label will be automatically applied to this pull request:

serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-07-15T22:12:49Z

Webrevs

alexmenkov · 2025-07-16T00:27:47Z

It looks good.
I added cleanup suggestion in the implementation

alexmenkov · 2025-07-16T00:26:16Z

src/jdk.jdi/share/classes/com/sun/tools/jdi/StackFrameImpl.java

+                // We first need to find out if the current frame is native, or if the
+                // previous frame is native, in which case we throw NativeMethodException
+                for (int i = 0; i < 2; i++) {
+                    StackFrameImpl sf;


There is nothing implementation-specific here.
I'd suggest to:

StackFrameImpl -> StackFrame;

MethodImpl -> Method;

remove validateStackFrame at line 408 ('MethodImpl.location()' calls it)

Are you suggesting renaming the classes? This is a pretty conventional naming when you have classes implementing a spec defined in an interface class. There are a lot more than just StackFrame and Method that are doing this.

alexmenkov · 2025-07-16T01:46:29Z

src/jdk.jdi/share/classes/com/sun/tools/jdi/StackFrameImpl.java

+                for (int i = 0; i < 2; i++) {
+                    StackFrameImpl sf;
+                    try {
+                        sf = (StackFrameImpl)thread.frame(i);
+                    } catch (IndexOutOfBoundsException e) {
+                        // This should never happen, but we need to check for it.
+                        break;
+                    }
+                    sf.validateStackFrame();
+                    MethodImpl meth = (MethodImpl)sf.location().method();
+                    if (meth.isNative()) {
+                        throw new NativeMethodException();
                    }


Are you suggesting renaming the classes?

No, I mean

Suggested change

for (int i = 0; i < 2; i++) {

StackFrameImpl sf;

try {

sf = (StackFrameImpl)thread.frame(i);

} catch (IndexOutOfBoundsException e) {

// This should never happen, but we need to check for it.

break;

}

sf.validateStackFrame();

MethodImpl meth = (MethodImpl)sf.location().method();

if (meth.isNative()) {

throw new NativeMethodException();

}

for (int i = 0; i < 2; i++) {

StackFrame sf;

try {

sf = thread.frame(i);

} catch (IndexOutOfBoundsException e) {

// This should never happen, but we need to check for it.

break;

}

Method meth = sf.location().method();

if (meth.isNative()) {

throw new NativeMethodException();

}

Ok. I see now.

sspitsyn

I've posted one concern on the spec updates. Otherwise, it looks okay to me.
I guess, a CSR will be filed for this update. I'm not sure what do you mean by CR in the description. Do I understand this right that a potential JDWP spec update we touched out of this PR will be considered separately?

sspitsyn · 2025-07-17T08:19:53Z

src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java

-     * @throws OpaqueFrameException if this thread is a suspended virtual thread and the
-     * target VM was unable to pop the frames.
+     * @throws OpaqueFrameException if target VM is unable to pop this frame
+     * (e.g. a virtual thread is not suspended at an event).


Nit: I'm not sure the comment at line 406 is fully correct. The IncompatibleThreadStateException will be thrown if a virtual thread is not suspended. So, the OpaqueFrameException will be thrown only when a virtual thread is suspended but not at an event. The same concern should apply for the line 488.

We are talking about the difference between "not suspended" vs "not suspended at an event". The former results in IncompatibleThreadStateException. For the latter I felt the wording implied it was suspended, but not at an event. Maybe it should just say "suspended, but not at an event"?

The JVMTI spec does not have this issue because it doesn't mention virtual threads as part of the OPAQUE_ERROR description. It instead mentions the method being native as an example of what can cause OPAQUE_ERROR. We can't do that here because a native method results in NativeMethodException, not OpaqueFrameException.

Maybe it should just say "suspended, but not at an event"?

Yes, this would make it clear.
And yes, the JVMTI vs JDI spec difference is reasonable.

plummercj · 2025-07-17T14:44:29Z

I've posted one concern on the spec updates. Otherwise, it looks okay to me. I guess, a CSR will be filed for this update. I'm not sure what do you mean by CR in the description. Do I understand this right that a potential JDWP spec update we touched out of this PR will be considered separately?

I meant CSR. I'll fix that. JDWP will be updated by #26336.

sspitsyn · 2025-07-17T23:04:55Z

I meant CSR. I'll fix that. JDWP will be updated by #26336.

Okay, thanks. I'll wait for CSR to review it.

AlanBateman

Looks good to me.

plummercj · 2025-07-28T22:45:32Z

The CSR is now ready for review: JDK-8363896:

plummercj · 2025-08-19T14:50:21Z

Thanks for the reviews Alan, Alex, and Serguei!

/integrate

plummercj · 2025-08-20T15:30:46Z

/integrate

openjdk · 2025-08-20T15:32:21Z

Going to push as commit 9041f4c.
Since your change was applied there have been 423 commits pushed to the master branch:

3e60ab5: 8348611: Eliminate DeferredLintHandler and emit warnings after attribution
5ca8d7c: 8284499: Add the ability to right-click and open in new tab JavaDoc Search results
ebf5ae8: 8359222: [asan] jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest triggers stack-buffer-overflow error
... and 420 more: https://git.openjdk.org/jdk/compare/9bef2d1610647dec18f9e81cbac3dddbbf99dd6d...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-08-20T15:32:30Z

@plummercj Pushed as commit 9041f4c.

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

openjdk · 2025-08-20T15:32:41Z

@plummercj The command integrate can only be used in open pull requests.

fix OpaqueFrameException related issues

5a07934

openjdk bot changed the title ~~8309400~~ 8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown Jul 15, 2025

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Jul 15, 2025

openjdk bot added the serviceability [email protected] label Jul 15, 2025

alexmenkov reviewed Jul 16, 2025

View reviewed changes

update based on feedback

05cce17

alexmenkov approved these changes Jul 16, 2025

View reviewed changes

sspitsyn approved these changes Jul 17, 2025

View reviewed changes

minor rewording

e637685

AlanBateman approved these changes Jul 22, 2025

View reviewed changes

sspitsyn approved these changes Jul 23, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Aug 15, 2025

openjdk bot added the integrated Pull request has been integrated label Aug 20, 2025

openjdk bot closed this Aug 20, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 20, 2025

8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown #26335

8309400: JDI spec needs to clarify when OpaqueFrameException and NativeMethodException are thrown #26335

Uh oh!

Conversation

plummercj commented Jul 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 15, 2025

Uh oh!

openjdk bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plummercj commented Jul 15, 2025

Uh oh!

openjdk bot commented Jul 15, 2025

Uh oh!

openjdk bot commented Jul 15, 2025

Uh oh!

mlbridge bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

alexmenkov commented Jul 16, 2025

Uh oh!

alexmenkov Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

plummercj Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

plummercj Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

plummercj Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

sspitsyn Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

plummercj commented Jul 17, 2025

Uh oh!

sspitsyn commented Jul 17, 2025

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

plummercj commented Jul 28, 2025

Uh oh!

plummercj commented Aug 19, 2025

Uh oh!

plummercj commented Aug 20, 2025

Uh oh!

openjdk bot commented Aug 20, 2025

Uh oh!

openjdk bot commented Aug 20, 2025

Uh oh!

openjdk bot commented Aug 20, 2025

Uh oh!

Uh oh!

plummercj commented Jul 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 15, 2025 •

edited

Loading

mlbridge bot commented Jul 15, 2025 •

edited

Loading