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

8165749: java.lang.RuntimeException: dndGesture.dragboard is null in dragDrop #372

Closed
wants to merge 2 commits into from

Conversation

jperedadnr
Copy link
Collaborator

@jperedadnr jperedadnr commented Jan 6, 2021

As commented in the JBS issue, there is one single dndGesture object in Scene, that can be instantiated from three different ways:

  • DropTargetListener, on dragEnter
  • DragGestureListener, on dragGestureRecognized or
  • MouseHandler, processing a right mouse click (these two are mutually exclusive).

This PR prevents that, for two almost simultaneous drag events (for instance via mouse and touchpad), the dndGesture for the last of these events gets initialized again, losing all existing drag info (dndGesture.dragboard) that was added to the first one, avoiding the RTE.

The existing manual test DndTest has been used on MacOS to verify the PR.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8165749: java.lang.RuntimeException: dndGesture.dragboard is null in dragDrop

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/372/head:pull/372
$ git checkout pull/372

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 6, 2021

👋 Welcome back jpereda! 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 openjdk bot added the rfr label Jan 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 6, 2021

Webrevs

@@ -3785,7 +3785,7 @@ private void process(MouseEvent e, boolean onPulse) {
backButtonDown || forwardButtonDown)) {
//old gesture ended and new one started
gestureStarted = true;
if (!PLATFORM_DRAG_GESTURE_INITIATION) {
if (!PLATFORM_DRAG_GESTURE_INITIATION && Scene.this.dndGesture == null) {
Copy link
Member

@kevinrushforth kevinrushforth Jan 6, 2021

Choose a reason for hiding this comment

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

This will prevent the creation of the new DndGesture, but we will still clear the PDR targets. Have you confirmed that this is the desired behavior? It seems reasonable, since we also clear it on a mouse move, but wanted to get your take on it.

Copy link
Collaborator Author

@jperedadnr jperedadnr Jan 6, 2021

Choose a reason for hiding this comment

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

This is really an edge case. There can be two gestures at once, an existing one (i.e from TouchPad), with its full drag information, that hasn't finished yet, and a new mouse one.

Having a single dndGesture object for both events seems wrong. But having a list/array of events for such scenario seems overkill.

Somehow some information from one or the other event might get lost or mixed up. The PDR targets are not required for the initial touch event, and we prevent the RTE, so I'd say this is a reasonable non-intrusive fix.

Copy link
Member

@kevinrushforth kevinrushforth Jan 8, 2021

Choose a reason for hiding this comment

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

In looking at it more closely, I think it might be better to avoid setting gestureStarted if there is already a drag gesture in process (started via dragEnter). Maybe something like this?

    if (Scene.this.dndGesture == null) {
        //old gesture ended and new one started
        gestureStarted = true;
        if (!PLATFORM_DRAG_GESTURE_INITIATION) {
            Scene.this.dndGesture = new DnDGesture();
        }
        clearPDREventTargets();
    }

Copy link
Collaborator Author

@jperedadnr jperedadnr Jan 9, 2021

Choose a reason for hiding this comment

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

Makes sense, I'll change it, but avoiding two nested if expressions.

@kevinrushforth kevinrushforth self-requested a review Jan 7, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jan 7, 2021

Yes, it does seem to fix the exception. I'd like to take a closer look to make sure that it isn't just masking the problem. Also, a second reviewer could be helpful on this, even though it is a simple fix.

@arapte or @pankaj-bansal can one of you also look at it?

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Jan 7, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth requested a review from arapte Jan 11, 2021
arapte
arapte approved these changes Jan 13, 2021
Copy link
Member

@arapte arapte left a comment

Change and sanity testing with it looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

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

8165749: java.lang.RuntimeException: dndGesture.dragboard is null in dragDrop

Reviewed-by: kcr, arapte

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

  • e3e600b: Merge
  • 1d45997: 8256362: JavaFX must warn when the javafx.* modules are loaded from the classpath
  • 9dd2058: 8259639: GitHub actions: build fails on Linux due to missing apt-get update
  • 2491661: 8259277: Change JavaFX release version to 17
  • 9c84c77: 8211294: ScrollPane content is blurry with 125% scaling
  • c197b62: 8242621: TabPane: Memory leak when switching skin

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added the ready label Jan 13, 2021
@jperedadnr
Copy link
Collaborator Author

@jperedadnr jperedadnr commented Jan 15, 2021

/integrate

@openjdk openjdk bot closed this Jan 15, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

@jperedadnr Since your change was applied there have been 6 commits pushed to the master branch:

  • e3e600b: Merge
  • 1d45997: 8256362: JavaFX must warn when the javafx.* modules are loaded from the classpath
  • 9dd2058: 8259639: GitHub actions: build fails on Linux due to missing apt-get update
  • 2491661: 8259277: Change JavaFX release version to 17
  • 9c84c77: 8211294: ScrollPane content is blurry with 125% scaling
  • c197b62: 8242621: TabPane: Memory leak when switching skin

Your commit was automatically rebased without conflicts.

Pushed as commit d8bb41d.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
3 participants