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

[Martin Fox]

On Fri, 12 May 2023 15:05:01 GMT, Lukasz Kostyra <lkostyra at openjdk.org> wrote:

>> Crashes started happening due to macOS DnD API change from macOS 10.14 onwards. 10.14 incrodues some [DnD constrains](https://developer.apple.com/documentation/macos-release-notes/appkit-release-notes-for-macos-10_14#Drag-and-Drop) 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.
>
> Lukasz Kostyra has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
> 
>  - GlassViewDelegate: Return to deprecated *PboardType symbols
>    
>    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.
>  - GlassPasteboard: Correct macOS version information in comments
>    
>    New DnD API was introduced starting macOS 10.7, not 10.14
>  - GlassView: Remove deprecated draggingSourceOperationMask
>  - DndTest: Update instructions
>    
>    On Mac to enable moving you must press Cmd, not Shift.
>  - Restore logic responsible for Cmd key support
>  - Merge branch 'master' into dnd_multiple_items-JDK-8233955
>  - Add DnD Multiple File manual test
>  - Fix preview position; cleanup code
>  - Add image previews and finish DnD code
>    
>    Fixes issues with tests caused by first commit.
>    
>    Removes old code used as placeholder.
>  - Migrate DnD native code to DraggingSession API
>    
>    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.

modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.h line 71:

> 69: 
> 70:     NSDragOperation         dragOperation;
> 71:     NSDraggingSession       *draggingSession;

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.

modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 962:

> 960:     for (NSDraggingItem* item in items)
> 961:     {
> 962:         NSPoint dragPoint = [self->nsView convertPoint:[self->lastEvent locationInWindow] fromView:nil];//[self->lastEvent locationInWindow];

Minor: left over comment at end of line

modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 1061:

> 1059:         // main thread blocked here until drag completes
> 1060: 
> 1061:         [GlassDragSource setDelegate:nil];

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.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1194126454
PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1194079640
PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1194152034