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

[Anthony Petrov]

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