RFR JDK8015799

[Dmitry Samersoff]

Chris,

Looks good for me.

Thank you for doing it.

-Dmitry

On 2013-06-28 13:36, Chris Hegarty wrote:
> The latest webrev is
>   http://cr.openjdk.java.net/~jzavgren/8015799/webrev.03/
> 
> We end up with:
> 
>  private String filterHeaderField(String name, String value) {
>         if (value == null)
>             return null;
> 
>         if (SET_COOKIE.equalsIgnoreCase(name) ||
>             SET_COOKIE2.equalsIgnoreCase(name)) {
> 
>             // Filtering only if there is a cookie handler. [Assumption:
> the
>             // cookie handler will store/retrieve the HttpOnly cookies]
>             if (cookieHandler == null || value.length() == 0)
>                 return value;
>             ....
> 
> So return value is not changed. BTW. I agree with your comments.
> 
> -Chris.
> 
> 
> On 06/27/2013 06:13 PM, Dmitry Samersoff wrote:
>> Chris,
>>
>> 1. I'm not sure it's a correct to return null rather then empty value,
>>     but you understand better what is happening, so I'm leaving it up
>>     to you.
>>
>> 2. It might be better to move
>>
>>    2805         if (value == null)
>>    2806             return null;
>>
>> under if(SET_COOKIE ...), i.e. to ll. 2810
>>
>> it doesn't change anything in practice - the methods continue returning
>> null for all cases where value is null -  but makes code better
>> understandable.
>>
>> -Dmitry
>>
>>
>> On 2013-06-27 00:49, Chris Hegarty wrote:
>>> To link this email thread, both in the archives, and for others. The
>>> call for review on this bug started with:
>>>    http://mail.openjdk.java.net/pipermail/net-dev/2013-June/006607.html
>>>
>>> On 06/26/2013 08:22 PM, Kurchi Hazra wrote:
>>>>
>>>> On 6/26/2013 12:17 PM, Kurchi Hazra wrote:
>>>>> Hi John,
>>>>>
>>>>>    Why not change lines 2810-2811 to:
>>>>>      if (value == null || value.length() == 0)
>>>>>          return value;
>>>> I meant return null. For other cookie-headers too, is there any reason
>>>> for us not returning null if the length of value is 0?
>>>
>>> In the first webrev John had made this change, but I asked him to revert
>>> it and only change the Set-Cookie(2) headers.
>>>
>>> "Since all header retrieval passes through filterHeaderField, in one way
>>> or another, I'm a little concerned about changing this. Also, as the
>>> only issue we know of is with Set-Cookie(2), maybe you could add the
>>> empty string check to these headers only? ( that is to say, move the
>>> 'value.length() == 0' check into the ' if
>>> (SET_COOKIE.equalsIgnoreCase(name)..... '  "
>>>
>>> The difference is, currently if a header value is non-null and has a
>>> length of 0 ( i.e. empty string ), then empty string is returned. With
>>> the original change then null is returned.
>>>
>>> We have been bitten by subtle changes in this area before. Returning
>>> null from such an API, URLConnection.getHeaderField(s), for cases where
>>> it did not return null before may lead to NPE's in some applications.
>>>
>>> -Chris.
>>>
>>>>>
>>>>> Also, lots of formatting issue in the test, especially in
>>>>> TestCookieHandler, try-catch block indentation is off in line 54.
>>>>>   Its also best to stop the server in a finally clause at line 58.
>>>>> Alternatively, I also liked Andreas' use of autocloseable in his test
>>>>> for 6563286. See [1].
>>>
>>> Yes, please.
>>>
>>> -Chris.
>>>
>>>>>
>>>>> - Kurchi
>>>>>
>>>>> [1]
>>>>> http://cr.openjdk.java.net/~arieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html
>>>>>
>>>>>
>>>>> <http://cr.openjdk.java.net/%7Earieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html>
>>>>>
>>>>>
>>>>>
>>>>> On 6/26/2013 10:54 AM, John Zavgren wrote:
>>>>>> Please consider the following changes to the Java cookie code.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jzavgren/8015799/webrev.02/
>>>>>> <http://cr.openjdk.java.net/%7Ejzavgren/8015799/webrev.02/>
>>>>>>
>>>>>> The problem I fixed occurs when a server returns an array of cookies
>>>>>> that contains a null cookie.
>>>>>>
>>>>>> Thanks
>>>>>> John
>>>>>> -- 
>>>>>> John Zavgren
>>>>>> john.zavgren at oracle.com
>>>>>> 603-821-0904
>>>>>> US-Burlington-MA
>>>>>
>>>>
>>>> -- 
>>>> -Kurchi
>>>>
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.