RFR JDK8015799

[Chris Hegarty]

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
>>>
>
>