<AWT Dev> [9] Review request for 7033533: realSync() doesn't work with Xfce

Oleg Pekhovskiy oleg.pekhovskiy at oracle.com
Fri Jan 24 05:26:59 PST 2014 

Hi Anthony,

thank you for the review,

seems like these lines are redundant for two reasons:
1. We already have waiting cycle before these lines (2418-2429).
2. These lines are entered only on XConvertSelection failure with simple 
X Server (for example when the main Window Manager is crashed or is 
being changed). In this case XConvertSelection doesn't convert VERSION 
property because it exists only for Window Manager (like Compiz). But 
the solution in my fix doesn't expect any property existence and works 
for all popular X Servers/Window Managers (including Mir, LXDM and Xfce).

Thanks,
Oleg

On 1/24/14 3:35 PM, Anthony Petrov wrote:
> Hi Oleg,
>
> Thanks for the update. The new version of the fix looks good to me. 
> I'm wondering if we should preserve the workaround at original lines 
> 2430-2442 in XToolkit.java for added safety?
>
> -- 
> best regards,
> Anthony
>
> On 1/22/2014 8:31 PM, Oleg Pekhovskiy wrote:
>> Hi Team,
>>
>>
>> There was a long break after last comment and now I'm able to continue
>> discussion.
>> Please review the second version of fix:
>> http://cr.openjdk.java.net/~bagiras/7033533.2/
>>
>> I investigated the order of different events and found that
>> ConfigureNotify event goes through event queues (X Server and Xlib) the
>> same way as SelectionNotify.
>> Moreover it works with and without window manager. Please look at the
>> native test and its log file attached to the issue in JIRA.
>> Thus it could unify the way XToolkit.syncNativeQueue() marks the block
>> of not handled events.
>>
>> There are several XLib functions that make X server generate
>> ConfigureNotify event:
>> XConfigureWindow(), XLowerWindow(), XRaiseWindow(), XRestackWindows(),
>> XMoveWindow(), XResizeWindow(), XMoveResizeWindow(), XMapRaised(),
>> XSetWindowBorderWidth().
>> Invisible XAWTRootWindow could be used as a target for ConfigureNotify
>> events.
>> None of mentioned above functions are used for this window and it never
>> becomes visible.
>> These functions generate only one event (ConfigureNotify), contrary to
>> XConvertSelection function which generates two (PropertyNotify,
>> SelectionNotify).
>>
>> I chose XMoveWindow(), moving XAWTRootWindow back and forth by 1 pixel
>> to avoid system optimization.
>>
>>
>> Thanks,
>> Oleg
>>
>> On 10/08/2013 05:09 PM, Anthony Petrov wrote:
>>> Hi Artem,
>>>
>>> I'm not sure the testing results below are positive since in his
>>> previous message Yuri wrote:
>>>
>>>> However some failures are suspicious. Try on Ubuntu with Unity.
>>>
>>> when testing on officially supported systems/window managers (see the
>>> quote below).
>>>
>>> Also, Leonid and I have concerns regarding reliability of the current
>>> version of the fix, and suggest to implement a solution that would try
>>> executing the current code  of the realSync() first, and only if it
>>> fails fall back to the workaround proposed by Oleg.
>>>
>>>
>>> What is your opinion on this?
>>>
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>>
>>> -------- Original Message --------
>>> Subject: Re: <AWT Dev> [8] Review request for 7033533: realSync()
>>> doesn't work with Xfce
>>> Date: Fri, 04 Oct 2013 18:52:32 +0400
>>> From: Yuri Nesterenko <yuri.nesterenko at oracle.com>
>>> To: Leonid Romanov <leonid.romanov at oracle.com>,  Anthony Petrov
>>> <anthony.petrov at oracle.com>
>>> CC: Awt-Dev <awt-dev at openjdk.java.net>
>>>
>>> OK, Ubuntu 13.10 (final beta) with Mir instead of X11
>>> looks surprisingly good. It fails in couple of places
>>> with exceptions like
>>> "sun.awt.X11.XException: Cannot write XdndAware property"
>>> bug overall the patched build run the awt regression suite
>>> quite OK.
>>>
>>> Thanks,
>>> -yan
>>>
>>> On 10/04/2013 03:52 PM, Yuri Nesterenko wrote:
>>>> So far, I've run some 300+ awt tests with patched build
>>>> on Solaris 11 sparcv9 and Ubuntu 13.04 Unity.
>>>>
>>>> Overall passrate is regular.
>>>> Most of the failing tests using realSync do fail rather
>>>> similarly with patched build and b110.
>>>>
>>>> However some failures are suspicious. Try on Ubuntu with Unity.
>>>>
>>>> test/java/awt/Toolkit/ScreenInsetsTest/ScreenInsetsTest.java
>>>> consistently fails some 6 out of 20 runs with patched build but
>>>> never in the promoted b110. It may be test issue though, somehow.
>>>>
>>>> event/MouseEvent/SpuriousExitEnter/SpuriousExitEnter_3.java
>>>> fails in half of the runs with "Incorrect event number" but
>>>> always pass with b110 (4 or 5 runs).
>>>> Event number! It may be superficial resemblance but --
>>>>
>>>> Choice/ChoiceMouseWheelTest/ChoiceMouseWheelTest.java --
>>>> coincidence here: Oleg, perhaps you should use
>>>> @library ../../regtesthelpers and
>>>> @build Util
>>>> in these regtests: import test.java.awt.regtesthelpers.Util fails.
>>>>
>>>> Thanks,
>>>> -yan
>>>>
>>>> On 10/01/2013 04:27 PM, Yuri Nesterenko wrote:
>>>>> Sure we can say that Xfce is not complying -- it is not officially
>>>>> supported by JDK -- neither is LXDE etc. -- but saying that gets
>>>>> us nowhere.
>>>>>
>>>>> As to testing, could you suggest a platform selection? I'm afraid
>>>>> we'll not be able to test Xsun properly but Xorg with Gnome on
>>>>> Linux and Solaris, Unity and Xfce4 --
>>>>> all that we can do by the weekend.
>>>>>
>>>>> Thanks,
>>>>> -yan
>>>>>
>>>>> On 10/01/2013 04:01 PM, Leonid Romanov wrote:
>>>>>> By the way, I was reading Inter-Client Communication Conventions
>>>>>> Manual
>>>>>> so I could better understand the fix, and found the following:
>>>>>>
>>>>>> "4.3. Communication with the Window Manager by Means of Selections
>>>>>>
>>>>>> For each screen they manage, window managers will acquire ownership
>>>>>> of a
>>>>>> selection named WM_Sn, where n is the screen number, as described in
>>>>>> section 1.2.6. Window managers should comply with the conventions 
>>>>>> for
>>>>>> "Manager Selections" described in section 2.8. The intent is for
>>>>>> clients
>>>>>> to be able to request a variety of information or services by 
>>>>>> issuing
>>>>>> conversion requests on this selection."
>>>>>>
>>>>>> Considering this, can we say that Xfce is not complying to the spec?
>>>>>>
>>>>>> On 10/1/2013 15:29, Anthony Petrov wrote:
>>>>>>> Hi Oleg,
>>>>>>>
>>>>>>> I second to Leonid: you should add a comment and explain why you
>>>>>>> expect exactly 4 (or more) events to be processed. Preferably, you
>>>>>>> should list each event to clearly understand this.
>>>>>>>
>>>>>>> A minor comment is, lines 2404 - 2407 should be moved to the 
>>>>>>> nearest
>>>>>>> try{} block at line 2409.
>>>>>>>
>>>>>>> A major concern is that I'm not sure the new solution is 
>>>>>>> reliable in
>>>>>>> all cases. Previously, we expected to get a notification from 
>>>>>>> another
>>>>>>> process (the WM). Now we process the notification ourselves and are
>>>>>>> the owners of the selection. I don't know for sure, but suppose 
>>>>>>> some
>>>>>>> xlib implementations could optimize this scenario and process 
>>>>>>> events
>>>>>>> locally w/o even sending them to the X server. In which case there
>>>>>>> wouldn't be any real synchronization of the native event queue.
>>>>>>> That's
>>>>>>> why communicating with another process was an important part of 
>>>>>>> this
>>>>>>> procedure.
>>>>>>>
>>>>>>> How about we try the original method first, and only if it fails,
>>>>>>> then
>>>>>>> try this workaround solution?
>>>>>>>
>>>>>>> Also, this is a very sensitive method because a lot of code 
>>>>>>> relies on
>>>>>>> it. I suggest running all automatic regression tests for AWT and
>>>>>>> Swing
>>>>>>> areas to make sure we don't introduce a regression with this fix.
>>>>>>>
>>>>>>> -- 
>>>>>>> best regards,
>>>>>>> Anthony
>>>>>>>
>>>>>>> On 09/26/2013 11:56 AM, Oleg Pekhovskiy wrote:
>>>>>>>> Hi Leonid,
>>>>>>>>
>>>>>>>> I did it mostly logically, because my fix added one more service
>>>>>>>> event
>>>>>>>> (SelectionRequest), that should be excluded from the number of
>>>>>>>> dispatched native events.
>>>>>>>> IMHO, the previous number "2" should have been commented, but, 
>>>>>>>> that
>>>>>>>> did
>>>>>>>> not happen.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Oleg
>>>>>>>>
>>>>>>>> On 25.09.2013 18:11, Leonid Romanov wrote:
>>>>>>>>> Hi,
>>>>>>>>> I'm not an expert in X11 programming, so I can't comment about 
>>>>>>>>> the
>>>>>>>>> fix
>>>>>>>>> in general, but I think the line 2436, "return getEventNumber() -
>>>>>>>>> event_number > 3", really asks for a comment.
>>>>>>>>>
>>>>>>>>> On 9/25/2013 16:38, Oleg Pekhovskiy wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> please review the fix
>>>>>>>>>>
>>>>>>>>>> for
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-7033533
>>>>>>>>>>
>>>>>>>>>> Previous implementation of XToolkit.syncNativeQueue() relied 
>>>>>>>>>> upon
>>>>>>>>>> WM_S0 atom existence and that it was owned by current window
>>>>>>>>>> manager.
>>>>>>>>>> But several WMs (like XFCE and LXDE) don't send SelectionNotify
>>>>>>>>>> event
>>>>>>>>>> to the client on XConvertSelection() for that atom. That led
>>>>>>>>>> XToolkit.syncNativeQueue() to hang until TimeOutException.
>>>>>>>>>>
>>>>>>>>>> I decided to keep XConvertSelection() usage, but make root 
>>>>>>>>>> toolkit
>>>>>>>>>> window as an owner for selection atom (with another name), and
>>>>>>>>>> handle
>>>>>>>>>> SelectionRequest event from X Server, sending SelectionNotify in
>>>>>>>>>> response (as window manager is supposed to do).
>>>>>>>>>>
>>>>>>>>>> Tested on both XFCE and LXDE.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Oleg
>>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>

More information about the awt-dev mailing list