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

Anthony Petrov anthony.petrov at oracle.com
Fri Jan 24 05:54:14 PST 2014 

Sounds good. I'm fine with the fix then, provided it's been tested 
thoroughly on all supported configurations (including OEL and Solaris 
systems that we support).

--
best regards,
Anthony

On 1/24/2014 5:26 PM, Oleg Pekhovskiy wrote:
> 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