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

[Sergey Bylokhov]

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.