<AWT Dev> <AWT dev> Review Request for 8006941 : [macosx] Deadlock in drag and drop & 7199783 : Setting cursor on DragSourceContext does not work on OSX
Petr Pchelko
petr.pchelko at oracle.com
Wed Apr 3 10:27:50 PDT 2013
Hello, Anthony.
Thank you for the review.
> Why not make it a part of the PlatformWindow interface then? There's already the getLayerPtr() which is Mac-specific.
I agree to Sergey. The idea as I understand is to avoid mac specific methods in PlatformWindow, so my vote would be not to add another such method to PlatformWindow.
What so you think about it?
With best regards. Petr.
03.04.2013, в 19:25, Sergey Bylokhov <Sergey.Bylokhov at oracle.com> написал(а):
> On 4/3/13 7:11 PM, Anthony Petrov wrote:
>>
>> Why not make it a part of the PlatformWindow interface then? There's already the getLayerPtr() which is Mac-specific.
> It was added by the mistake. I hope it will be removed in the future.
>> So adding something like getContentPtr() wouldn't make it any worse. And it would still look better than a bunch of instanceof checks in the helper method.
>>
>> Otherwise the fix looks good to me.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 4/3/2013 18:41, Petr Pchelko wrote:
>>> Hello, Anthony.
>>>
>>> Thank you for the review. I have updated the fix according to your comments. The new version is here:
>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.06/
>>>
>>>> Can this condition ever be not true? I see that only LWComponentPeer.java calls this code. Why are the argument types for CDropTarget so generic?
>>> This is left here from the Apple times. Deleted.
>>>
>>>> There's the same code in CDropTarget.java. I suggest to extract it in a common utility method somewhere in the macosx.* classes.
>>> Moved to the helper method in CPlatformWindow
>>>
>>>> A general question: you mention 7199783 (implement a missing functionality), 8006941 (resolve a deadlock), and also, the test mentions a crash. Looks like these three are completely separate issues and should better be fixed with separate patches for clarity and easier maintainability. Why are you combining all this into one fix? I'd vote for fixing them separately if it is possible.
>>> I know thats bad and the problems seem completely separate and unrelated.
>>> But when the deadlock was fixed a lot of code related to cursor could be cleaned up. This code was added as a workaround for the fixed deadlock. Cleaning up this code simultaneously resolves the issue 7199783. As for the crash - it was discovered while I was making the fix and prevented from testing some corner cases. But the crash itself could not be fixed without fixing the deadlock. So all these issues are so interconnected that the only possibility left was to merge all changes into one fix.
>>>
>>> With best regards. Petr.
>>>
>>> On Apr 3, 2013, at 2:50 PM, Anthony Petrov wrote:
>>>
>>>> Hi Petr,
>>>>
>>>> A general question: you mention 7199783 (implement a missing functionality), 8006941 (resolve a deadlock), and also, the test mentions a crash. Looks like these three are completely separate issues and should better be fixed with separate patches for clarity and easier maintainability. Why are you combining all this into one fix? I'd vote for fixing them separately if it is possible.
>>>>
>>>> Here's some comments regarding the code changes:
>>>>
>>>> src/macosx/classes/sun/lwawt/macosx/CDropTarget.java
>>>>> 54 // Make sure the drop target is a LWWindowPeer:
>>>>> 55 if (!(peer instanceof LWComponentPeer))
>>>> Can this condition ever be not true? I see that only LWComponentPeer.java calls this code. Why are the argument types for CDropTarget so generic?
>>>>
>>>>
>>>> src/macosx/classes/sun/lwawt/macosx/CDragSourceContextPeer.java
>>>>> 111 PlatformWindow platformWindow = ((LWComponentPeer)rootComponent.getPeer()).getPlatformWindow();
>>>>> 112 long nativeViewPtr;
>>>>> 113 if (platformWindow instanceof CPlatformWindow) {
>>>>> 114 nativeViewPtr = ((CPlatformWindow) platformWindow).getContentView().getAWTView();
>>>>> 115 } else if (platformWindow instanceof CViewPlatformEmbeddedFrame) {
>>>>> 116 nativeViewPtr = ((CViewPlatformEmbeddedFrame) platformWindow).getNSViewPtr();
>>>>> 117 } else {
>>>>> 118 throw new InvalidDnDOperationException("Unsupported PlatformWindow implementation");
>>>>> 119 }
>>>> There's the same code in CDropTarget.java. I suggest to extract it in a common utility method somewhere in the macosx.* classes.
>>>>
>>>> The rest of the fix looks good I guess, although I'm not an expert in DnD.
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>> On 4/1/2013 16:14, Petr Pchelko wrote:
>>>>> Hello, AWT Team.
>>>>> This is a reminder. Could I please get a second review on this?
>>>>> For your convenience:
>>>>> Bug:
>>>>> http://bugs.sun.com/view_bug.do?bug_id=7199783 Setting cursor on DragSourceContext does not work on OSX
>>>>> http://bugs.sun.com/view_bug.do?bug_id=8006941 Deadlock in drag and drop
>>>>> Fix:
>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.05/
>>>>> With best regards. Petr.
>>>>> On Mar 11, 2013, at 12:30 PM, Petr Pchelko wrote:
>>>>>> Hello, AWT Team.
>>>>>>
>>>>>> Could I please get a second review on this?
>>>>>>
>>>>>> For your convenience:
>>>>>> Bug:
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7199783 Setting cursor on DragSourceContext does not work on OSX
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8006941 Deadlock in drag and drop
>>>>>> Fix:
>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.05/
>>>>>>
>>>>>> Thank you.
>>>>>> With best regards. Petr.
>>>>>>
>>>>>> On Mar 4, 2013, at 4:01 PM, Sergey Bylokhov wrote:
>>>>>>
>>>>>>> Hi, Petr.
>>>>>>> Fix looks good. Comment about the test: Swing classes should be created and used on the EDT. Change it before the push.
>>>>>>>
>>>>>>> 04.03.2013 15:15, Petr Pchelko wrote:
>>>>>>>> Hello, Sergey.
>>>>>>>>
>>>>>>>> Please have a look on the updated webrev with a test:
>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.05/
>>>>>>>>
>>>>>>>> All the rest is unchanged.
>>>>>>>>
>>>>>>>> With best regards. Petr.
>>>>>>>>
>>>>>>>> On Mar 1, 2013, at 1:21 PM, Sergey Bylokhov wrote:
>>>>>>>>
>>>>>>>>> Hi, Petr.
>>>>>>>>> If the crash can be reproduced easily I suggest to add a new test.
>>>>>>>>>
>>>>>>>>> 28.02.2013 20:21, Petr Pchelko wrote:
>>>>>>>>>> Thank you for the reviews.
>>>>>>>>>>
>>>>>>>>>> Please have a look on the updated version here:
>>>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.04/
>>>>>>>>>>
>>>>>>>>>> The CDropTarget is now also ready to work with a view-based embedded frame.
>>>>>>>>>> Also A native crash was discovered, it will be fixed with the current fix.
>>>>>>>>>>
>>>>>>>>>> I have discovered 2 additional DnD issues while testing, I would file separate issues.
>>>>>>>>>>
>>>>>>>>>> With best regards. Petr.
>>>>>>>>>>
>>>>>>>>>> On Feb 28, 2013, at 4:47 PM, Denis S. Fokin wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Petr,
>>>>>>>>>>>
>>>>>>>>>>> I would make these lines shorter...
>>>>>>>>>>> CDropTarget.m:495
>>>>>>>>>>> CDropTarget.m:609
>>>>>>>>>>>
>>>>>>>>>>> Ok, I see that in the next line I did not noticed that the parentheses
>>>>>>>>>>> are related to the whole statement.
>>>>>>>>>>> 457 if (!(root.contains(x, y) && root.isEnabled() && root.isVisible())) {
>>>>>>>>>>>
>>>>>>>>>>> Therefor, I would prefer to see || instead of &&. But it is just my personal observation.
>>>>>>>>>>>
>>>>>>>>>>> Thank you,
>>>>>>>>>>> Denis.
>>>>>>>>>>>
>>>>>>>>>>> On 2/28/2013 4:36 PM, Petr Pchelko wrote:
>>>>>>>>>>>> Hello, Denis, Sergey. Thank you for the reviews.
>>>>>>>>>>>>
>>>>>>>>>>>> The updated version of the fix is available here:
>>>>>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.03/
>>>>>>>>>>>>
>>>>>>>>>>>> Sergey wrote:
>>>>>>>>>>>>> getViewPtr() is not a good method in PlatformWindow interface because pointer to NSView is macosx specific implementation.
>>>>>>>>>>>> Removed it, replace with if's with instanceof
>>>>>>>>>>>>
>>>>>>>>>>>>> What does it mean "Returns the first " in the getDropTargetAt. Should it be hw/lw?
>>>>>>>>>>>> It should return any component which would be a valid drop target for the drag. It does not matter, lightweight or heavyweight. I have updated a comment a bit.
>>>>>>>>>>>>
>>>>>>>>>>>>> Check your fix with the example from the CR:8007155. Just modify for dnd.
>>>>>>>>>>>> Works fine.
>>>>>>>>>>>>
>>>>>>>>>>>> Denis wrote:
>>>>>>>>>>>>> Where we are posting dragExit and dragEnter messages now? Is it the event dispatching issue that you have mentioned?
>>>>>>>>>>>>> CDropTarget.m:516,614
>>>>>>>>>>>> The DragSource Enter/Exit messages are posted in Java now. The issue is that native dropTarget bounds are not the same as in Java because a native peer for DnD is a contentView of the window. So these events are posted in the wrong places or were not posted when they should have been. For DropTarget events this issue also exist and the Enter/Exit events are modified on the Java level. But for the dragSource I've decided not to use native events at all, because they are not very useful, it's easier to synthesize them in Java.
>>>>>>>>>>>>
>>>>>>>>>>>>> I would say that visibility and isEnabled() value do not make sense here.
>>>>>>>>>>>>> CDragSourceContextPeer.java
>>>>>>>>>>>>> 449 if (!(root.contains(x, y) && root.isVisible() && root.isEnabled())) {
>>>>>>>>>>>> We should find only the dropTarget which would be a valid target for the current drag. This implies that the component should be enabled, visible, and should have a valid drop target. This would produce the same behavior as on Windows.
>>>>>>>>>>>>
>>>>>>>>>>>> With best regards. Petr.
>>>>>>>>>>>>
>>>>>>>>>>>> On Feb 28, 2013, at 1:24 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi, Petr.
>>>>>>>>>>>>> getViewPtr() is not a good method in PlatformWindow interface because pointer to NSView is macosx specific implementation.
>>>>>>>>>>>>> Could you please split VERY long line in the CDropTarget.m
>>>>>>>>>>>>> What does it mean "Returns the first " in the getDropTargetAt. Should it be hw/lw?
>>>>>>>>>>>>> Check your fix with the example from the CR:8007155. Just modify for dnd.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 27.02.2013 17:43, Petr Pchelko wrote:
>>>>>>>>>>>>>> Hello, AWT Team.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please, review the fix for 2 issues:
>>>>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7199783 Setting cursor on DragSourceContext does not work on OSX
>>>>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8006941 Deadlock in drag and drop
>>>>>>>>>>>>>> The fix is available at:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.02/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I have made a single fix for these 2 issues, because they are quite closely related, and the same methods need to be changed. And they depend on one another quite a bit.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. The deadlock occurred because in CDragSourceContextPeer.dragMouseMoved methods on components were invoke on the Appkit thread. They blocked on an AWTTreeLock if EDT had already took it. EDT trying to perform a sync selector on the Appkit thread lead to a deadlock. So the logic of working with components are moved to the EDT now.
>>>>>>>>>>>>>> 2. DragSource events were dispatched absolutely incorrectly. Now we dispatch them the same way as on other platforms.
>>>>>>>>>>>>>> 3. CCursorManager contained a workaround for the issue that we were not able to perform sync selectors during the dnd. Now sync selectors are processed during drag, so this workaround is not needed any more.
>>>>>>>>>>>>>> 4. The functionality to set a cursor in DragSourceListeners is implemented. It works, however it still has a couple of issues:
>>>>>>>>>>>>>> a. Sometimes mouse events are dispatched during the dnd, which might reset a cursor. That is wrong, mouse events should not be dispatched. It is a separate issue, so this problem will disappear as mouseEvent dispatching would be fixed.
>>>>>>>>>>>>>> b. If the DropTarget supports NSDragOperationCopy cocoa sets an NSDragCopyCursor. There is no API to disable this and I have found no suitable workaround.
>>>>>>>>>>>>>> c. On the first drag in the lifetime of the application cursor is not set. This is due to a bug in cocoa. They reset a cursor in some setter which looks absolutely unrelated and called when the first dragging session is initialized. I am thinking about filing a bug against apple.
>>>>>>>>>>>>>> 5. Some cleanup: removed unused variables.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> With best regards. Petr.
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Best regards, Sergey.
>>>>>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Best regards, Sergey.
>>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best regards, Sergey.
>>>>>>>
>>>
>
>
> --
> Best regards, Sergey.
>
More information about the awt-dev
mailing list