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

[Sergey Bylokhov]

New version here:
http://cr.openjdk.java.net/~serb/7090424/webrev.02/

On 22.10.2013 13:52, Anthony Petrov wrote:
> 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
>>>>>>
>>>>
>>>>
>>>
>>>
>>
>>


-- 
Best regards, Sergey.