<AWT Dev> Review request for 8040076: java.awt.List objects allowing multiple selections are not GC-ed

Petr Pchelko petr.pchelko at oracle.com
Tue Jul 1 20:30:04 UTC 2014


Hello, Artem.

>> As Anthony said it's just for limiting visibility. 
I didn’t understand this actually. 

The new version looks good to me. 

With best regards. Petr.

> On Jul 1, 2014, at 10:40 PM, Anthony Petrov <anthony.petrov at oracle.com> wrote:
> 
> Hi Artem,
> 
> Note that assert() [1] is still not a run-time check because we specify -DNDEBUG when building Release binaries. To make it a runtime check, the code should check the condition explicitly, and if it's false, then raise a Java exception (e.g. AWTError) and return from the JNI method. However, I realize that our current code never fails the assertion anyway. So I'm leaving this issue up to you and other reviewers, in case anyone feels strongly about this.
> 
> Other than that, the fix looks fine to me. Thanks for your hard work.
> 
> 
> [1] http://msdn.microsoft.com/en-us/library/9sb57dw4(v=vs.100).aspx
> 
> --
> best regards,
> Anthony
> 
> On 7/1/2014 6:34 PM, artem malinko wrote:
>> Hi, Antony and Petr! Thank you for detailed review.  I Hope this version
>> is better.
>> 
>> Link:
>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.05/
>> 
>> Petr:
>> "4. Lines 37-43. Normally we do not use blocks to group the code together."
>> 
>> As Anthony said it's just for limiting visibility. But maybe code logic
>> would be more clear if explicitly null list reference, so I changed it.
>> 
>> "5. Are you sure that you do not need to wait for a frame to actually
>> show up on the screen so that all the peers are guaranteed to get created?"
>> I'm pretty sure. List peer will be created if it's container peer not
>> null. And container peer definitely not null at this stage because it
>> was created in the same thread when we invoked setVisible() on JFrame.
>> 
>> Everything else is adjusted according recommendations.
>> 
>> On 30.06.2014 19:14, Anthony Petrov wrote:
>>> Hi Artem,
>>> 
>>> 1. Your code still uses wrong formatting. Please just open this page
>>> to see the problem with the lines indentation:
>>> 
>>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/src/windows/native/sun/windows/awt_Component.cpp.sdiff.html
>>> 
>>> 
>>> 
>>> 2. DASSERT() is only relevant for debug builds which we use very
>>> rarely. I'd prefer to make this a run-time check. To compensate for
>>> possible performance degradation, I suggest to place it to the else{}
>>> branch of your if() statement, so that the check is only performed
>>> when it's really needed.
>>> 
>>> 
>>> 3. A standard copyright header in the test file is missing. Please see
>>> other tests for examples.
>>> 
>>> 4. The test should also contain a @bug jtreg tag and mention the
>>> concrete bug id that's being verified with this test.
>>> 
>>> 5. The dispose() call is better placed to the finally{} block of the
>>> try{} statement to ensure it's always executed.
>>> 
>>> 6. You don't really need a System.exit() call in your test. If the
>>> frame is dispose()'d in the finally{} block, the test will terminate
>>> by itself.
>>> 
>>> 7. In the catch{} block in the test() method, the if() statements
>>> should either be one-liners, or enclose their "then" branches into
>>> blocks {}.
>>> 
>>> Also, Petr wrote:
>>>> 4. Lines 37-43. Normally we do not use blocks to group the code
>>>> together.
>>> 
>>> I think this is okay in this case. A block is used to limit the
>>> visibility of the strong reference to the List object. We need it to
>>> "convert" it into a WeakReference.
>>> 
>>> 
>>>> Where is the original reference created?
>>> 
>>> It's created in the same method - CreateHWND(). Please examine the
>>> code in AwtList to see that we only need to recreate the native
>>> control (i.e. the hwnd) when an app toggles the multiple selection
>>> property. So the code and the fix make sense to me. Perhaps we should
>>> add a comment before the "if (m_peerObject == NULL)" check to explain
>>> why we do this.
>>> 
>>> 
>>> --
>>> best regards,
>>> Anthony
>>> 
>>> On 6/30/2014 2:17 PM, artem malinko wrote:
>>>> Thank you, Anthony.
>>>> 
>>>> Yes, I think assertion won't be superfluous to prevent other bugs of
>>>> same type. Here is a version with assert.
>>>> 
>>>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.02/
>>>> 
>>>> On 27.06.2014 1:12, Anthony Petrov wrote:
>>>>> Hi Artem,
>>>>> 
>>>>> Please configure you code editor so that it formats the code that you
>>>>> modify according to Java code conventions used in OpenJDK (4-spaces
>>>>> line indentation, a space after "if" and before "{", etc.)
>>>>> 
>>>>> Also, please include the bug id and synopsis to the subject line of
>>>>> your review requests. Please see other review threads on this mailing
>>>>> list for examples.
>>>>> 
>>>>> As for the fix itself, should we add an assertion check to the
>>>>> CreateHWnd() method to verify that both peer and m_peerObject refer to
>>>>> the same Java object in case the latter is already set?
>>>>> 
>>>>> --
>>>>> best regards,
>>>>> Anthony
>>>>> 
>>>>> On 6/26/2014 7:30 PM, artem malinko wrote:
>>>>>> Hello, AWT Team.
>>>>>> 
>>>>>> Please review a fix for the issue:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8040076
>>>>>> The fix is available at:
>>>>>> http://cr.openjdk.java.net/~mcherkas/artem/webrev.01/
>>>>>> 
>>>>>> When method "void AwtList::SetMultiSelect" is invoked it invokes "void
>>>>>> AwtComponent::CreateHWnd" where m_peerObject initialized. But at this
>>>>>> stage m_peerObject already initialized and already holds ref to java
>>>>>> List object. So original m_peerObject is lost and ref to java List
>>>>>> lost
>>>>>> as well. In the fix I've added check whether m_peerObject is
>>>>>> initialized
>>>>>> or not.
>>>>>> 
>>>>>> Thank you.
>>>> 
>> 



More information about the awt-dev mailing list