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

[Anthony Petrov]

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