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

[Anthony Petrov]

Hi Sergey,

Sorry, but I can't review a fix w/o a webrev.

What kind of problems do you experience with the tool? The cr.openjdk 
should now have enough free space to host new webrevs. Please try to 
re-upload the new webrev.

--
best regards,
Anthony

On 10/22/2013 12:28 PM, Sergey Bylokhov wrote:
> I have a problem with the review tool. I assume that version .00 with
> removed volatiles and renamed handleexpose event is ok for everybody?
>
> On 10/21/13 4:53 PM, Sergey Bylokhov wrote:
>> 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
>>>>>
>>>
>>>
>>
>>
>
>