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

artem malinko artem.malinko at oracle.com
Wed Jul 2 07:12:41 UTC 2014

Thank you.


"I didn’t understand this actually."

// other method code...

   List list = new 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.

More information about the awt-dev mailing list