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

[Anthony Petrov]

On 10/02/2013 02:52 PM, Oleg Pekhovskiy wrote:
> On 01.10.2013 15:29, Anthony Petrov wrote:
>> 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.
>
> I'll investigate why the previous number was 2...
>
>> A minor comment is, lines 2404 - 2407 should be moved to the nearest
>> try{} block at line 2409.
>
> Agree.

Thanks.


>> 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?
>
> In my fix XSendEvent is used to send SelectionNotify event and in the
> doc http://www.x.org/archive/X11R7.5/doc/man/man3/XSendEvent.3.html
> there is a paragraph saying that XSendEvent always works with the server:
>
> "The event in the XEvent structure must be one of the core events or one
> of the events defined by an extension (or a BadValue error results) so
> that the X server can correctly byte-swap the contents as necessary. The
> contents of the event are otherwise unaltered and unchecked by the X
> server except to force send_event to True in the forwarded event and to
> set the serial number in the event correctly; therefore these fields and
> the display field are ignored by XSendEvent."
>
> So no optimization expected in this case.

I don't see how this statement follows from the spec. An Xlib 
implementation could still optimize requests when they manipulate 
entities belonging to the same app.


> Leaving the original method along with the new one will complicate the
> code and could produce pitfalls. IMHO it would be better to thoroughly
> test my fix.

That would be fine to do early in JDK 8, or early in JDK 9. But at this 
point it is too risky for JDK 8. That's the reason Leonid and I suggest 
to make it a workaround rather than the main method for syncing the 
native event queue.

--
best regards,
Anthony

>> 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.
>
> As I mentioned Yuri ran tests on XFCE and LXDE. But we could do even
> more of course.
>
>>
>> --
>> 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
>>>>> http://cr.openjdk.java.net/~bagiras/7033533.1/
>>>>> 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
>>>>