[10] Review request for 8185634, 8185634: Java Fx-Swing dialogs appearing behind main stage

[Kevin Rushforth]

_FX patch_

The new tests run fine for me on all three platforms. I noticed a couple 
issues while reviewing it:

1. All robot-based tests should be in the test.robot.** package 
hierarchy, such that they are only enabled with "-PUSE_ROBOT=true" -- I 
recommend to put them in a new test.robot.javafx.embed.swing package.

2. Can you replace the wild-card imports in the tests with explicit imports?


_JDK patch_

You need to rebase the patch against the latest tip of jdk/client. Once 
you do that, you will notice that CPlatformWindow.java no longer 
compiles on Mac -- you will need to add an explicit import of 
sun.lwawt.LWLightweightFramePeer.

-- Kevin


Alexander Zvegintsev wrote:
>
> Hi Sergey,
>
> I am not able to crash it on several platforms, except one case:
>
> if we are terminating JavaFX application while EDT processing some 
> long task. But it is unrelated to the fix and reproducible on current 
> builds.
>
> I've filed a separate bug JDK-8190329 
> <https://bugs.openjdk.java.net/browse/JDK-8190329>.
>
>> Will the current fix cover the native dialogs like 
>> FileDialog/PrintDialog on linux and windows? 
> It will not, Windows already fixed by JDK-8088395 
> <https://bugs.openjdk.java.net/browse/JDK-8088395>
>
>
> Test added:
> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/05/
> Thanks,
> Alexander.
> On 13/10/2017 01:14, Sergey Bylokhov wrote:
>> Looks fine.
>> I am not sure but it looks like the fix has an assumption that the 
>> CPlatformWindow.setVisible() code will be executed on EDT/Appkit but 
>> it is not the case. This code can be executed on any 
>> thread(intentionally for crash), and it will be good to check that it 
>> works as expected by some stress test which will try to force the 
>> possible crash: 4 threads:
>>  - show/dispose Swing Node
>>  - show/dispose Dialog1/2/3 using different timeouts
>>
>> Will the current fix cover the native dialogs like 
>> FileDialog/PrintDialog on linux and windows?
>>
>> On 10/10/2017 13:54, Kevin Rushforth wrote:
>>> Note that there is now a 04 version.
>>>
>>> It looks good to me, although someone more familiar with AWT should 
>>> also check the AWT changes.
>>>
>>> We will need a test program for this (as a follow-on issue if not now).
>>>
>>> -- Kevin
>>>
>>>
>>> Alexander Zvegintsev wrote:
>>>> Please review the updated version
>>>>
>>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/02/
>>>> Now we are postponing actual window closing, it happens only after 
>>>> we ensure that native window pointer is valid on Swing side.
>>>>
>>>> Thanks,
>>>> Alexander.
>>>>
>>>> On 23/09/2017 08:01, Sergey Bylokhov wrote:
>>>>> Hi, Alexander.
>>>>> How can we be sure that the parent frame will not be disposed 
>>>>> while we use a pointer?
>>>>>
>>>>> long ownerWindowPtr = peer.getOverridenWindowHandle();
>>>>> <<<<< Dispose the peer
>>>>> if (ownerWindowPtr != 0) {
>>>>>     //Place window above JavaFX stage
>>>>>     CWrapper.NSWindow.addChildWindow(
>>>>>     ownerWindowPtr, ptr, CWrapper.NSWindow.NSWindowAbove);
>>>>> <<<<< Boom
>>>>> }
>>>>>
>>>>>
>>>>> On 9/21/17 22:56, Alexander Zvegintsev wrote:
>>>>>> Hi Phil,
>>>>>>
>>>>>> Please review the updated fix with reflection incorporated
>>>>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/01/
>>>>>>
>>>>>> New issue created JDK-8187803 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8187803> as JDK 
>>>>>> counterpart of this issue.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexander.
>>>>>>
>>>>>> On 21/09/2017 22:25, Phil Race wrote:
>>>>>>> Some procedural comments :
>>>>>>> Since 90% of this is in AWT code, I'd have thought awt-dev 
>>>>>>> should be included here.
>>>>>>> I've added that list.
>>>>>>>
>>>>>>> And apart from needing separate bug ids, I don't see why the bug 
>>>>>>> below is confidential.
>>>>>>>
>>>>>>>
>>>>>>> I agree with what Kevin pointed out off-line that as in the 
>>>>>>> dialog case, the FX side
>>>>>>> of the code can use reflection and simply be a harmless 
>>>>>>> non-functional no-op
>>>>>>> if the SwingAccessor does not provide the new method.
>>>>>>>
>>>>>>> BTW
>>>>>>> 264 inline HWND GetOverridenHWnd() { return m_overridenHwnd; }
>>>>>>> should be "dd" not "d".
>>>>>>>
>>>>>>> -phil.
>>>>>>>
>>>>>>> On 09/21/2017 03:38 AM, Alexander Zvegintsev wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> please review the fix
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~azvegint/jdk/10/8185634/00/
>>>>>>>>
>>>>>>>> for the issue
>>>>>>>>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8185634
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>
>>
>