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

[Kevin Rushforth]

On Wed, 17 May 2023 08:50:23 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 incrementally with two additional commits since the last revision:
> 
>  - GlassViewDelegate: Address review issues
>    
>    * Removed draggingSource pointer
>    * Removed some leftover comments
>    * Set delegate to nil
>  - Revert "GlassViewDelegate: Return to deprecated *PboardType symbols"
>    
>    This reverts commit fa42b1b13755dec05d77b3865ed657e954854286.
>    
>    fa42b1b13 is not needed anymore - JFX will target macOS 11 soon.

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

modules/javafx.graphics/src/main/native-glass/mac/GlassPasteboard.m line 788:

> 786:                 for (NSPasteboardItem* i in objects)
> 787:                 {
> 788:                     [dItems addObject:[[NSDraggingItem alloc] initWithPasteboardWriter:i]];

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

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

> 974:         {
> 975:             // create an image with contents of URL
> 976:             image = [[NSImage alloc] initByReferencingFile:[[NSString alloc] initWithData:[pbItem dataForType:NSPasteboardTypeFileURL] encoding:NSUTF8StringEncoding]];

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

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

> 987:         {
> 988:             // create an image with contents of URL
> 989:             image = [[NSImage alloc] initByReferencingURL:[NSURL URLWithString:[[NSString alloc] initWithData:[pbItem dataForType:NSPasteboardTypeURL] encoding:NSUTF8StringEncoding]]];

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

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

PR Review: https://git.openjdk.org/jfx/pull/1089#pullrequestreview-1431803867
PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1197174973
PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1197175780
PR Review Comment: https://git.openjdk.org/jfx/pull/1089#discussion_r1197176435