<AWT Dev> Review a fix for List leak

[Anthony Petrov]

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.
>