<AWT Dev> [8] Review Request: 7090424 TestGlyphVectorLayout failed automately with java.lang.StackOverflowError

[Sergey Bylokhov]

Hi, Anthony.
Since cr have some issues with the space. I upload 2 files, where 
handleExposeEvent was renamed to postPaintEvent():

http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XWindow.java.sdiff.html
http://cr.openjdk.java.net/~serb/7090424/webrev%2c01/src/solaris/classes/sun/awt/X11/XContentWindow.java.sdiff.html

On 18.10.2013 15:47, Sergey Bylokhov wrote:
> Hi, Anthony.
> On 18.10.2013 15:17, Anthony Petrov wrote:
>> Hi Sergey,
>>
>> In XAWT, we usually use the StateLock to synchronize access to peer 
>> fields (such as background, label, etc.) I don't think that switching 
>> to volatile is a good idea since it prevents us from performing 
>> atomic read/writes to the fields. And this is exactly what we need 
>> for this fix, actually. In other words, the following pattern works 
>> perfectly:
>>
>> synchronized (lock) {
>>    if (a != b) {
>>       a = b;
>>       // do stuff, or set a flag to do it later w/o the lock
>>    }
>> }
>>
>> whereas the following doesn't:
>>
>> volatile a;
>> if (a != b) {
>>    a = b;
>>    // do stuff
>> }
>>
>> The latter doesn't work because the value of 'a' may change from 
>> another thread after the if() statement in the first thread is executed.
> If it will be changed that's ok. It is safe in this context since 
> there is no races. a will be the latest setted value and repaint 
> action will be done after a was set. Non trivial setters(like 
> setLabel/setText) are called under the locks in shared code.
>>
>> Please note that this is critical for AWT because it is a 
>> multi-threaded GUI toolkit.
>>
>> src/solaris/classes/sun/awt/X11/XListPeer.java
>>> -                target.paint(g);
>>> +                handleExposeEvent(target, 0, 0, getWidth(), 
>>> getHeight());
>>
>> (the same applies to XWindow.repaint): can you please rename 
>> XWindow.handleExposeEvent(Component...) to postPaintEvent() and make 
>> it final? A good javadoc for this method would also be appreciated, 
>> because currently seeing the name handle*() I'd think it needs to be 
>> done under the awtLock().
> Will do.
>>
>> Also, for the corresponding changes in XWindow.repaint(), could you 
>> please elaborate a bit more? Looking at the code I see that 
>> XWindow.paint() calls paintPeer(). And in repaint(), you either call 
>> paint() or paintPeer() depending on the current thread. Why is it 
>> needed? Can we just call paintPeer() (or paint() for that matter) 
>> unconditionally since they both seem to result in the same call?
> paintPeer is used to draw the native part of the peer(text of the 
> label, border of the button etc).
> paint is a part of Component.paintAll(). And as a result of it call it 
> should paint the native part of the peer and it should call 
> appropriate paint method from the target.
>> Also, why don't we post an event if reapint() is invoked on the EDT?
> We do this in osx, but in xawt there are "smart" caches, which are 
> initialized during the paint of the peer. See 
> XLabelPeer&cachedFontMetrics for example . Note that this cache is 
> completely broken.
>>
>> -- 
>> best regards,
>> Anthony
>>
>> On 10/16/2013 06:00 PM, Sergey Bylokhov wrote:
>>> Hello.
>>> Please review the fix for jdk 8.
>>> The fix has two parts
>>>   - Repaint method in the peer now paint the component in place if it
>>> was called on EDT only.
>>>   - Most of setters were changed to stop recursion if they were called
>>> on EDT.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-7090424
>>> Webrev can be found at: 
>>> http://cr.openjdk.java.net/~serb/7090424/webrev.00
>>>
>
>


-- 
Best regards, Sergey.