<AWT Dev> <Awt Dev> [9] Review Request for 8132664: closed/javax/swing/DataTransfer/DefaultNoDrop/DefaultNoDrop.java locks on Windows

Tue Sep 8 11:06:51 UTC 2015

On 08.09.15 13:42, Semyon Sadetsky wrote:
>
>
> On 9/8/2015 1:31 PM, Sergey Bylokhov wrote:
>> On 08.09.15 13:15, Semyon Sadetsky wrote:
>>>
>>>
>>> On 9/8/2015 12:33 PM, Sergey Bylokhov wrote:
>>>> On 08.09.15 12:04, Semyon Sadetsky wrote:
>>>>>
>>>>>
>>>>> On 9/7/2015 6:56 PM, Sergey Bylokhov wrote:
>>>>>> On 07.09.15 16:41, Semyon Sadetsky wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 9/7/2015 3:28 PM, Sergey Bylokhov wrote:
>>>>>>>> On 03.09.15 17:43, Semyon Sadetsky wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8/5/2015 2:33 PM, Sergey Bylokhov wrote:
>>>>>>>>>> On 05.08.15 14:20, Semyon Sadetsky wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 8/5/2015 1:39 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>> On 05.08.15 13:18, Semyon Sadetsky wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 8/5/2015 12:27 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>> On 04.08.15 14:54, Semyon Sadetsky wrote:
>>>>>>>>>>>>>>> On 8/3/2015 6:05 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>>> Hi, Semyon
>>>>>>>>>>>>>>>> Did you try to change dwMilliseconds from INFINITE to the
>>>>>>>>>>>>>>>> timeout(10 seconds by default?) which is passed to the
>>>>>>>>>>>>>>>> method?
>>>>>>>>>>>>>>>> It does not help? Because even when dnd is not used we
>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>> not wait event for infinite time.
>>>>>>>>>>>>>>> It would not help to fix the issue because 10 seconds is
>>>>>>>>>>>>>>> too big
>>>>>>>>>>>>>>> interval. But for consistency it is not bad to have a time
>>>>>>>>>>>>>>> limit.
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8132664/webrev.01/
>>>>>>>>>>>>>> Note that syncNativeQueue is intended to wait until the
>>>>>>>>>>>>>> native
>>>>>>>>>>>>>> event queue is flushed or until timeout will expire. So
>>>>>>>>>>>>>> even if
>>>>>>>>>>>>>> timeout is expired we collect the native events during this
>>>>>>>>>>>>>> period
>>>>>>>>>>>>>> of time.Can you double check that the event counter is
>>>>>>>>>>>>>> incremented
>>>>>>>>>>>>>> during dnd? I do not know how we block the toolkit thread,
>>>>>>>>>>>>>> probably we create some nested loops which ignore our event
>>>>>>>>>>>>>> posted
>>>>>>>>>>>>>> from syncNativeQueue, can we change that?
>>>>>>>>>>>>> Yes, this is an internal secondary loop which waits for mouse
>>>>>>>>>>>>> release event.
>>>>>>>>>>>> Can we change the condition and process the sync event in this
>>>>>>>>>>>> loop?
>>>>>>>>>>> Why? Will receive all events on the toolkit thread when
>>>>>>>>>>> doDragDrop
>>>>>>>>>>> returns.
>>>>>>>>>>
>>>>>>>>>> When how we get dragenter/exit events?
>>>>>>>>>
>>>>>>>>> On the platform side they are not events but callbacks which are
>>>>>>>>> converted into events on the java side and added to the queue. So
>>>>>>>>> they
>>>>>>>>> will be detected by isEQEmpty() and waitForIdle() will be
>>>>>>>>> repeated.
>>>>>>>>
>>>>>>>> This is not directly related to this fix. Most of our
>>>>>>>> callbacks/events
>>>>>>>> posts events to the EQ, but there is a possibility for a lag
>>>>>>>> between
>>>>>>>> callback and a post events, and this is why the syncNativeQueue was
>>>>>>>> added.
>>>>>>>> Let's return to the beginning: the syncNativeQueue method
>>>>>>>> according to
>>>>>>>> its specifications should try to flush the native system, track the
>>>>>>>> native activity and should not wait more than timeout parameter.
>>>>>>>
>>>>>>> It is not possible to send sync events to the DD loop. Only
>>>>>>> mouse/keyboard event can return control to the app. I would not
>>>>>>> emulate
>>>>>>> them in the syncnative.
>>>>>>> Always waiting the max time is not good. Maybe wait a bit and
>>>>>>> check if
>>>>>>> no callbacks happened?
>>>>>>
>>>>>> It will be good to check that callbacks were occurred in the same way
>>>>>> as native events. This can be done in this fix. As for question about
>>>>>> wait or not when timeout is over, is a shared problem, which affect
>>>>>> the osx also. It can be fixed separately. I guess the logic in
>>>>>> realSync can be tweaked and the default timeout(10*4 sec) is not a
>>>>>> optimal as a default value, 2 or 3 seconds without any events should
>>>>>> be enough.
>>>>> Could you explain your logic?
>>>>
>>>> My logic is to follow the specification.
>>> I cannot accept this as an answer because it is an abstract declaration.
>>> Please, try to be more specific.
>>
>> Just implement as I described it above.
>>
> Sorry?

Just change implementation of syncNativeQueue to don't block forever if 
timeout parameter is positive. And wait forever in case of negative 
timeout. This is enough to fix for now, now need to add some workarounds 
for dnd, since the realsync itself should be reworked.

>>
>>> I can only make guesses what specification section you mean. For
>>> example, the next is waitForIdle() method's spec:
>>>      /**
>>>       * Waits until all events currently on the event queue have been
>>> processed.
>>>       * @throws  IllegalThreadStateException if called on the AWT event
>>> dispatching thread
>>>       */
>>> What does specifically contradicts to this specification?
>>
>> Check the documentation of the realSync method.
>>
> realSync is an internal method. It is not a part of the specification.
> It has this description of the timeout parameter:
>       * @param timeout the maximum time to wait in milliseconds,
> negative means "forever".
>
> That is as it used in the fix. No?
>>
>>>>
>>>>> Why to wait short wait period doesn't work
>>>>> if we are in DD and there are no callbacks?
>>>>
>>>> "No callbacks" during what period? My point: this period should be
>>>> passed to this method.
>>> Period is passed to the method.
>>> Java_sun_awt_windows_WToolkit_syncNativeQueue(JNIEnv *env, jobject self,
>>> jlong timeout)
>>> It's meaning is a _maximum_ time the method should wait. Before the fix
>>> it was ignored and the method never returned.
>>> With the fix the timeout is taken into account but this aspect of the
>>> fix is not related to the original issue and it cannot fix the issue
>>> alone.
>>> Or you meant another method? Again, please be more specific otherwise
>>> this discussion can be infinite.
>>>>
>>>>> No callbacks means no
>>>>> activity and in analogous situations the method returns true.
>>>>
>>>>
>>>>
>>>>> I see a clear disadvantage: even with 2-3 second wait period the test
>>>>> will executes much longer than on Linux platform because D&D is
>>>>> triggered every 3 mouse drags . Also other D&D tests suites will
>>>>> demand
>>>>> much more time to pass the windows platform. Users can report bugs if
>>>>> AWT Robot starts to produce multi-seconds delay after drag events.
>>>>> Maybe you can provide an example scenario which demonstrates that
>>>>> it is
>>>>> really necessary to pay this price?
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>> Event counter is not changed during toolkit thread blocking of
>>>>>>>>>>>>> cause. Not sure that we can change that. But since toolkit
>>>>>>>>>>>>> queue is
>>>>>>>>>>>>> blocked we can assume that we are synced.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The timeout value is maintained on the shared level and
>>>>>>>>>>>>>> actually
>>>>>>>>>>>>>> this test will fail with timeout on osx as well JDK-7185258.
>>>>>>>>>>>>>> The
>>>>>>>>>>>>>> test will fail even if the time out will be changed to
>>>>>>>>>>>>>> ±100ms,
>>>>>>>>>>>>>> because it call realsync on each pixel move, ±200 times. This
>>>>>>>>>>>>>> problem can be fixed in different ways like tweak of
>>>>>>>>>>>>>> timeouts and
>>>>>>>>>>>>>> numbers of iterations, or changing the test to call w4idle
>>>>>>>>>>>>>> only
>>>>>>>>>>>>>> after the latest move(actually I think this method can be
>>>>>>>>>>>>>> moved to
>>>>>>>>>>>>>> the robot class).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So I still think that the right fix for a deadlock, which is
>>>>>>>>>>>>>> subject of this bug, is simply change the syncNativeQueue and
>>>>>>>>>>>>>> waits using a timeout if it is positive, and waits forever if
>>>>>>>>>>>>>> timeout is negative (the same bug on osx JDK-8080504).
>>>>>>>>>>>>> I'm not sure that waiting brings any value. What do you
>>>>>>>>>>>>> propose to
>>>>>>>>>>>>> return if it timed out? The event counter will not be changed
>>>>>>>>>>>>> regardless of waiting.
>>>>>>>>>>>> But it should be changed, because we get native events from the
>>>>>>>>>>>> system during dnd and in each such callback we should update
>>>>>>>>>>>> this
>>>>>>>>>>>> counter. If callbacks were not called=>counter was not updated
>>>>>>>>>>>> then
>>>>>>>>>>>> sync assume that currently we do not process events. If
>>>>>>>>>>>> callbacks
>>>>>>>>>>>> were called then sync assume that we have events in the native
>>>>>>>>>>>> queue
>>>>>>>>>>>> and should try to sync again on the next iteration.
>>>>>>>>>>> No. Events are not processed while toolkit is blocked in
>>>>>>>>>>> doDragDrop(). The application state is frozen on that period.
>>>>>>>>>>> That is
>>>>>>>>>>> Windows approach.
>>>>>>>>>>>>
>>>>>>>>>>>>> With such waiting the test will fail because of either jtreg
>>>>>>>>>>>>> timeout
>>>>>>>>>>>> Default timeout is 120 seconds for everything, the test try to
>>>>>>>>>>>> sync
>>>>>>>>>>>> the queue 200 of times after each move, so yes it can fail with
>>>>>>>>>>>> timeout even if spend in nativesync 200 ms, the possible
>>>>>>>>>>>> solutions
>>>>>>>>>>>> were in my previous email.
>>>>>>>>>>>>> either InfiniteLoop exception.
>>>>>>>>>>>> This exception will be disabled by default lately in jdk9
>>>>>>>>>>>> timeframe,
>>>>>>>>>>>> right now it helps to find some related issues.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 03.08.15 17:26, Semyon Sadetsky wrote:
>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Please review fix for JDK9:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8132664
>>>>>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8132664/webrev.00/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> DoDragDrop() is blocking, so upon drag operation is
>>>>>>>>>>>>>>>>> triggered
>>>>>>>>>>>>>>>>> the toolkit thread is blocked and the WM_AWT_WAIT
>>>>>>>>>>>>>>>>> cannot be
>>>>>>>>>>>>>>>>> processed which in its turn blocks the AWT robot.
>>>>>>>>>>>>>>>>> The solution is to escape AWT robot waiting in
>>>>>>>>>>>>>>>>> syncNativeQueue() if drag operation is in progress.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

-- 
Best regards, Sergey.