<AWT Dev> [8] Review request for 8027151: AWT_DnD/Basic_DnD/Automated/DnDMerlinQL/MultipleJVM failing on windows machine

[Oleg Pekhovskiy]

Thank you for the review, Anthony,

I created new issue:
https://bugs.openjdk.java.net/browse/JDK-8027469
and linked it together with 8027452.

Thanks,
Oleg

On 29.10.2013 21:00, Anthony Petrov wrote:
> Thanks for the details, Oleg. I agree that it's a good idea to
> investigate the close() issue in JDK 9.
>
> I've also noticed that DataFlavor.getReaderForText() doesn't mention
> that user code is responsible for close()'ing the returned Reader. I
> suggest to update the specification of this method and mention that
> (please add a note about the spec change to 8027452 which you've just
> filed).
>
> With this done, the fix looks good to me.
>
> --
> best regards,
> Anthony
>
> On 10/29/2013 06:39 PM, Oleg Pekhovskiy wrote:
>> Hi Anthony,
>>
>> I could refer to the fix for JDK-7075105. the finally {} block was added
>> there and had never existed before.
>>
>> Here's the way InputStream passed to DataTransferer.getInstance().
>> translateStream() gets closed:
>>
>> In out case DataTransferer.translateStream() calls
>> DataTransferer.translateStreamToInputStream() on line 1776 where
>> the stream is passed to ReencodingInputStream constructor. It is then
>> passed to InputStreamReader constructor. Both classes override close()
>> method where ReencodingInputStream closes referenced InputStreamReader
>> and
>> InputStreamReader closes referenced InputStream.
>>
>> ReencodingInputStream is returned by
>> SunDropTargetContextPeer.getTransferData() method, called by
>> DataFlavor.getReaderForText() that returns it to the client code.
>> So it's a client code obligation to call ReencodingInputStream.close().
>>
>> This is the use case for the test mentioned in the issue description.
>> But in general we should investigate each use case to avoid memory
>> leaks. As Petr pointed out it makes sense to do it for JDK 9, because it
>> could be risky for now.
>>
>> Thanks,
>> Oleg
>>
>> On 29.10.2013 16:41, Anthony Petrov wrote:
>>> Hi Oleg,
>>>
>>> I'm not an expert in this code so I may ask some silly questions.
>>>
>>> I'm OK with the change #2.
>>>
>>> Regarding #1: could you please clarify what code is responsible for
>>> closing the stream now, after your fix? Is this documented/enforced
>>> anywhere (i.e. a finally{} block or something)? Have you run any
>>> regression tests to make sure this change doesn't introduce any memory
>>> leaks? Why was this not a problem before that we decided to fix this
>>> particular piece now, so late in the release?
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 10/28/2013 02:41 AM, Oleg Pekhovskiy wrote:
>>>> Hi all,
>>>>
>>>> please review the fix
>>>> http://cr.openjdk.java.net/~bagiras/8027151.1/
>>>> for
>>>> https://bugs.openjdk.java.net/browse/JDK-8027151
>>>>
>>>> This issue appeared after the changes made for JDK-7075105.
>>>> There were two problems:
>>>> 1. In drop target code, in SunDropTargetContextPeer.getTransferData()
>>>> method inputStream was closed in finally scope but was not yet used in
>>>> client code (indirectly). Just its reference stored for the future in
>>>> DataTransferer.getInstance().translateStream() call.
>>>>
>>>> 2. In drop target code, in DataTransferer.translateStream() there
>>>> was no
>>>> String object handler (it was moved from
>>>> DataTransferer.translateBytesOrStream() only to
>>>> DataTransferer.translateBytes() method).
>>>>
>>>> Thanks,
>>>> Oleg