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

[Kevin Rushforth]

On Thu, 11 May 2023 15:48:39 GMT, Martin Fox <duke 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.
>
> modules/javafx.graphics/src/main/native-glass/mac/GlassDraggingSource.m line 44:
> 
>> 42:     return self->dragOperation;
>> 43: }
>> 44: 
> 
> `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.

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

> modules/javafx.graphics/src/main/native-glass/mac/GlassViewDelegate.m line 988:
> 
>> 986:         }
>> 987: 
>> 988:         if (image == nil && [pbItemTypes containsObject:NSPasteboardTypeFileURL])
> 
> `NSPasteboardTypeFileURL` was introduced in MacOS 10.13 and JavaFX is currently targeted at 10.12. The compiler caught this but the warning was lost in the sea of other warnings the Mac build generates.

Good catch (which reminds me that I need to get back to your warnings PR). This might misbehave on macOS 10.12 then, but I haven't seen a machine that old in a few years, so it would be difficult to test.

We really should bump the minimum at some point (to at _least_ 10.14, but ideally to 11 to match what Apple currently supports, and to match what the minimum has always been for ARM-based Mac systems), so this seems like the right time to broach that subject.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1191426568
PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1191431942