<AWT Dev> Review request for CR 8006634 Unify LWCToolkit.invokeAndWait() and CToolkitThreadBlockedHandler

Petr Pchelko petr.pchelko at oracle.com
Thu Feb 21 00:44:50 PST 2013


Hello, Artem.

Thank you for the review, the updated version of the fix is available here:
http://cr.openjdk.java.net/~pchelko/8006634/webrev.02/

With best regards. Petr.

On Feb 18, 2013, at 2:52 PM, Artem Ananiev wrote:

> Hi, Petr,
> 
> a few comments about the fix:
> 
> 1. Having two ctors in CToolkitThreadBlockedHandler seems to be redundant. One one is used in the current code.
> 
> 2. LWCToolkit.flushPendingEventsOnAppkit() is used in a single place, in CPlatformWindow. Could we get rid of this method completely and call LWCToolkit.invokeAndWait() from CPlatformWindow directly?
> 
> 3. LWCToolkit.invokeAndWait(Runnable, Component): invocation event, created in this method, must have "component" as the source, not the toolkit instance.
> 
> Thank you for such great refactoring and cleanup!
> 
> Artem
> 
> On 2/14/2013 6:27 PM, Petr Pchelko wrote:
>> Thank you, Sergey.
>> 
>> Please have a look at the updated version of the fix at:
>> http://cr.openjdk.java.net/~pchelko/8006634/webrev.01/
>> 
>>>  I suggest do not remove LWCToolkit.flushPendingEventsOnAppkit and reuse it in CDropTargetContextPeer.flushEvents Is it possible?
>> Done.
>> 
>>> Can you please take a look to this http://java.net/jira/browse/MACOSX_PORT-768 Looks like the workaround from those issue is not necessary now.
>> There is another bug: http://bugs.sun.com/view_bug.do?bug_id=7199783 The changes in CCursorManager will be made in the fix for that bug.
>> 
>>> - It would be really helpful to add additional doce about runloops.
>> I have extended the documentation on the doAWTRunLoop method a bit, but I have now idea what else to write there.
>> 
>>> The test will be welcome, because this CR fix the possible deadlock, when we use performOnMT inside DND .
>> The nested loop is run to synchronously process SunDropTargetEvents. There is no API to set a listener on this event without changing the JDK code.  When these events are processed, the new event (DropTargetEvent) is created which is that dispatched asynchronously. So I did not find any way to make a deadlock occur without hacking the JDK code.
>> 
>>>  Synchronisation in CToolkitThreadBlockedHandler  is not necessary?
>> It extends a mutex, so the client is responsible for the synchronization. This is the same behavior as on other platforms.
>> 
>>> There are some changes related to VoiceOver  could you please check that it works?
>> Yes, I've tried it for a bit, it seems to work.
>> 
>>>  I'm not sure but looks like stopAWTRunLoop should stop runloop synchronously since after that we release CFRelease(mediatorObject). Is it possible that when we call endRunLoop the object will be released?
>> The mediator object is retained two times, because it is used by 2 threads. Than it is released first time in endRunLoop and second time in the end of doAWTRunLoop, so it can't be already released when we end a runloop. There is no difference if we run stopRunLoop with waiting YES or NO, it is done on the main thread to avoid using @syncronized on a BOOL value. Making access to it @synchronized is about 10 times slower for some reason)
>> 
>> With best regards. Petr.
>> 
>> On Feb 14, 2013, at 1:18 PM, Sergey Bylokhov wrote:
>> 
>>> Hi, Petr.
>>> From the quick review:
>>> - I suggest do not remove LWCToolkit.flushPendingEventsOnAppkit and reuse it in CDropTargetContextPeer.flushEvents Is it possible?
>>> - Can you please take a look to this http://java.net/jira/browse/MACOSX_PORT-768 Looks like the workaround from those issue is not necessary now.
>>> - It would be really helpful to add additional doce about runloops.
>>> - The test will be welcome, because this CR fix the possible deadlock, when we use performOnMT inside DND .
>>> - Synchronisation in CToolkitThreadBlockedHandler  is not necessary?
>>> - There are some changes related to VoiceOver  could you please check that it works?
>>> - I'm not sure but looks like stopAWTRunLoop should stop runloop synchronously since after that we release CFRelease(mediatorObject). Is it possible that when we call endRunLoop the object will be released?
>>> 
>>> 14.02.2013 10:58, Petr Pchelko wrote:
>>>> Hello, AWT Team.
>>>> 
>>>> Please review the fix for the issue:
>>>> http://bugs.sun.com/view_bug.do?bug_id=8006634
>>>> The fix is available at:
>>>> http://cr.openjdk.java.net/~pchelko/8006634/webrev.00/
>>>> 
>>>> This fix makes a lot of changes in nested event loops:
>>>> 
>>>> 1. CToolkitThreadBlockedHandler was made a subclass of Mutex as it is made on other platforms. Shared code relies on that with logic for entering/exiting nested event loops. Previously it did not matter as CToolkitThreadBlockedHandler did not make a actual nested loop. Now it makes a native nested loop via doAWTRunLoop.
>>>> 
>>>> 2. doAWTRunLoop method is changed
>>>> 	a. Now we have a boolean flag processEvents. If it is set to false, we do not process native events while in the nested loop, same as how it was done before the fix. If it is set to true, events are processed while in the loop. Attention is needed when using this new feature, because we short-circuit native event processing and there could be some cases when it breaks Appkit. (resizing windows with a mouse could be an example) However we need this feature in the Drag and Drop.
>>>> 	b. I have got rid of an feature to run the nested loop in NSDefaultRunLoopMode, it was never used.
>>>> 
>>>> 3. We decided that we are getting rid of the deadlock detection mechanism: it is rarely used, nobody have ever seen the error message from it. I have tested a lot without this feature and it looks like everything is fine, however I am absolutely not sure about this decision. I am including Peter and Ragini as Accessibility was using this functionality and I had to change Accessibility code a bit.
>>>> 
>>>> 4. Some native methods from the LWCToolkit were made package-private. The comment stated that deploy used them, but I've cloned there code and they apparently do not use it any more. Looks like the comment is from Apple times and we do not event have a WebkitPluginObject any more.
>>>> 
>>>> With best regards. Petr.
>>> 
>>> 
>>> --
>>> Best regards, Sergey.
>>> 
>> 




More information about the awt-dev mailing list