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

8233955: VM crashes if more than one file are added to ClipboardContent via drag and drop #1089

Closed

Conversation

lukostyra
Copy link
Contributor

@lukostyra lukostyra commented Apr 12, 2023

Crashes started happening due to macOS DnD API change from macOS 10.14 onwards. 10.14 incrodues some DnD constrains which our DnD code happened to trigger on some occasions.

Our code used deprecated dragImage API which since 10.14 had new requirement - each NSPasteboard item had to have a corresponding drag image. Not meeting this constraint raised an exception, which crashed the app. Since there was no possibility to add more drag images to dragImage API (it only took one NSImage as parameter) the code had to be rewritten - as such I upgraded it to new DnD API utilizing NSDraggingSession and related protocols.

One side-effect of new DnD API is that it now modifies NSPasteboard for us - previous behavior was more "separated" from user code perspective (write items to pasteboard -> initiate a drag via dragImage). Manually updating NSPasteboard before calling beginDraggingSessionWithItems raised another exception related to NSPasteboard already having DnD-ed elements inside it. Some system tests, however, relied on that behavior and writing to NSPasteboard (MacPasteboardShim.java used in some tests creates a Clipboard.DND for test purposes). Since this path is in tests I assumed this behavior should stay and tried to make it as close to working as possible. Tests (including those using MacPasteboardShim) pass after my changes.

Additionally, added a new manual test based on DndTest.java test which creates two temporary files and allows for testing faulty behavior.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8233955: VM crashes if more than one file are added to ClipboardContent via drag and drop

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1089

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1089.diff

Webrev

Link to Webrev Comment

Previous implementation used dragImage call which is deprecated since macOS 10.14. Additionally,
10.14 introduced a restriction not allowing for more than one drag item in the Pasteboard. This
change fixes crashes caused by old API use when DnD-ing more than one item.
Fixes issues with tests caused by first commit.

Removes old code used as placeholder.
@bridgekeeper
Copy link

bridgekeeper bot commented Apr 12, 2023

👋 Welcome back lkostyra! 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 Ready for review label Apr 12, 2023
@lukostyra
Copy link
Contributor Author

I would especially like to ask for someone macOS-and-ObjC-proficient to take a look at this - ObjC is not my strongest suite and I'm not 100% sure whether I did not introduce any memory leaks or other issues.

@mlbridge
Copy link

mlbridge bot commented Apr 12, 2023

@kevinrushforth
Copy link
Member

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review April 13, 2023 12:00
@openjdk
Copy link

openjdk bot commented Apr 13, 2023

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

This fixes the problem, but there is one regression in behavior I noticed. To reproduce:

  1. Run DndTest (in the same manual tests dir as the one you added)
  2. Hold down the CMD key
  3. Drag the "DRAG ME" label to the "DROP ME" (while holding down CMD)

BUG: When you release the button, the drag does not complete. The expected behavior is that the drag completes with a transfer mode of "MOVE"

{
return self->dragOperation;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

sourceOperationMaskForDraggingContext replaces the deprecated draggingSourceOperationMaskForLocal: over in GlassViewDelegate.m. There's some logic there that needs to be copied over here. Basically the Apple documentation on how the Cmd key filters the set of available operations doesn't match reality and we have to compensate for that.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like it could be the reason for the functional regression in handling the CMD modifier while dragging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll take a look into that

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

On Mac to enable moving you must press Cmd, not Shift.
New DnD API was introduced starting macOS 10.7, not 10.14
New NSPasteboardType* symbols were introduced starting 10.13, and we're
targetting 10.12. This deprecation should probably be resolved once we target
macOS 10.13+.

Old NS*PboardType symbols do not have an equivalent of NSPasteboardTypeFileURL,
so this branch was removed.
{
NSPoint dragPoint = [self->nsView convertPoint:[self->lastEvent locationInWindow] fromView:nil];
NSPasteboard *pasteboard = [NSPasteboard pasteboardWithName:NSDragPboard];
NSPoint dragPoint = [self->nsView convertPoint:[self->lastEvent locationInWindow] fromView:nil];//[self->lastEvent locationInWindow];
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: left over comment at end of line

@@ -67,6 +68,8 @@ typedef enum GestureMaskType {
NSEvent *s_lastKeyEvent;

NSDragOperation dragOperation;
NSDraggingSession *draggingSession;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the dragOperation is used anymore. You also don't need draggingSession since you're not assigning anything to it. I think you can just remove it, the NSView should retain the draggingSession while the drag is going on and release it afterward.


[GlassDragSource setDelegate:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any place where the delegate is set back to nil in this PR so isDelegateSet will always return true after a mouse drag. And it could turn into a dangling pointer if the GlassViewDelegate goes away.

{
return self->dragOperation;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

@lukostyra
Copy link
Contributor Author

@kevinrushforth now that #1139 is up, should I revert fa42b1b? I could update more constants in the process too to keep DnD code clean of deprecated constants, kind of like #1137 does

@kevinrushforth
Copy link
Member

@kevinrushforth now that #1139 is up, should I revert fa42b1b?

I think you could either revert the changes you mention (so that we won't be introducing any additional deprecated constants as part of this fix) or do it in a follow-up issue. Whichever you prefer.

I could update more constants in the process too to keep DnD code clean of deprecated constants, kind of like #1137 does

That seems best left for a follow-on fix.

Btw, I'll take a look at the updated PR tomorrow.

This reverts commit fa42b1b.

fa42b1b is not needed anymore - JFX will target macOS 11 soon.
* Removed draggingSource pointer
* Removed some leftover comments
* Set delegate to nil
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

All my testing looks good. I left a couple comments about possible memory leaks.

NSMutableArray<NSDraggingItem*> *dItems = [NSMutableArray<NSDraggingItem*> arrayWithCapacity:itemCount];
for (NSPasteboardItem* i in objects)
{
[dItems addObject:[[NSDraggingItem alloc] initWithPasteboardWriter:i]];
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to call autorelease on the allocated NSDraggingItem (unless I missed seeing a release later on).

// ask the Pasteboard for ist own image representation of its contents
image = [[NSImage alloc] initWithPasteboard:pasteboard];
// create an image with contents of URL
image = [[NSImage alloc] initByReferencingFile:[[NSString alloc] initWithData:[pbItem dataForType:NSPasteboardTypeFileURL] encoding:NSUTF8StringEncoding]];
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to call autorelease on the allocated NSString.

CGFloat height = [image size].height;
if ((width > MAX_DRAG_SIZE) || (height > MAX_DRAG_SIZE))
// create an image with contents of URL
image = [[NSImage alloc] initByReferencingURL:[NSURL URLWithString:[[NSString alloc] initWithData:[pbItem dataForType:NSPasteboardTypeURL] encoding:NSUTF8StringEncoding]]];
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to call autorelease on the allocated NSString.


[image release];
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you aren't releasing the image now. You need to call [image release] to avoid a leak.

@lukostyra
Copy link
Contributor Author

We spoke about it offline, I was wondering if Apple APIs retain the resources and it turns out they seem to - I added release-s as mentioned and everything seems to work fine. I also added a release section in GlassPasteboard.m to clean up allocated by me NSDraggingItem-s, which after flushing the request and starting the drag should not be needed by us anymore. Tests all pass as they did, did not notice any regressions in both manual and automatic tests.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good with one (minor) suggested change. I'll do some final testing, then approve.

@lukostyra
Copy link
Contributor Author

I updated the code as suggested. Interesting behavior I did not expect, but the more you know - thanks for catching that.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good. I left one minor styling comment. I'll reapprove if you decide to change it.

Copy link
Member

@arapte arapte left a comment

Choose a reason for hiding this comment

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

Looks good. Needs change in copyright year of 2 new files.

Year "2011" in copyright headers was a Copy-Paste's error
@openjdk
Copy link

openjdk bot commented May 30, 2023

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

8233955: VM crashes if more than one file are added to ClipboardContent via drag and drop

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

  • 56fb71a: 8091153: Customize the Table Button Menu
  • 4b24c86: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet
  • 2f5dcfd: 8306329: Update ICU4C to 73.1
  • 8110f54: 8306328: Update libFFI to 3.4.4
  • bdadcb2: 8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem
  • 6aeaff3: 8295078: TextField blurry when inside an TitledPane -> AnchorPane
  • bff41c2: 8308114: Bump minimum version of macOS for x64 to 11.0 (matching aarch64)
  • c1a1179: 8245919: Region#padding property rendering error
  • e7974bc: 8308028: Replace more uses of System.getProperty("os.name") with PlatformUtil calls
  • 8aff525: 8307960: Create Table Column PopupMenu lazily
  • ... and 4 more: https://git.openjdk.org/jfx/compare/d9a82d103d60bc38b35ba94a21354c84a75bb22d...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 (@kevinrushforth, @arapte) 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).

@openjdk openjdk bot added the ready Ready to be integrated label May 30, 2023
@lukostyra
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label May 30, 2023
@openjdk
Copy link

openjdk bot commented May 30, 2023

@lukostyra
Your change (at version 5c909de) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented May 30, 2023

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

  • 56fb71a: 8091153: Customize the Table Button Menu
  • 4b24c86: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet
  • 2f5dcfd: 8306329: Update ICU4C to 73.1
  • 8110f54: 8306328: Update libFFI to 3.4.4
  • bdadcb2: 8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem
  • 6aeaff3: 8295078: TextField blurry when inside an TitledPane -> AnchorPane
  • bff41c2: 8308114: Bump minimum version of macOS for x64 to 11.0 (matching aarch64)
  • c1a1179: 8245919: Region#padding property rendering error
  • e7974bc: 8308028: Replace more uses of System.getProperty("os.name") with PlatformUtil calls
  • 8aff525: 8307960: Create Table Column PopupMenu lazily
  • ... and 4 more: https://git.openjdk.org/jfx/compare/d9a82d103d60bc38b35ba94a21354c84a75bb22d...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 30, 2023
@openjdk openjdk bot closed this May 30, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels May 30, 2023
@openjdk
Copy link

openjdk bot commented May 30, 2023

@kevinrushforth @lukostyra Pushed as commit afa71d4.

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

@lukostyra lukostyra deleted the dnd_multiple_items-JDK-8233955 branch November 28, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants