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

[artem malinko]

Thank you.

Petr:

"I didn’t understand this actually."

// other method code...

{
   List list = new List();
   frame.add(list);
   list.setMultipleMode(true);
   frame.remove(list);
   weakRef = new WeakReference<List>(list);
}

// other method code...

After closed brace "list" does not exist and the only ref to actual list object is "weakRef". If I don't use code block I should explicitly write "list = null;". Both do the same, but code block was just more convenient for me.


On 02.07.2014 0:30, Petr Pchelko wrote:
> 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.