Bug 82041

Summary: [BlackBerry]Missing cookies from HTTP response header in Network tab of Web Inspector.
Product: WebKit Reporter: Jason Liu <jasonliuwebkit>
Component: WebKit BlackBerryAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: joenotcharles, kpiascik, leo.yang, rwlbuis, staikos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jason Liu 2012-03-23 03:44:48 PDT
These cookies are been set, it's only missing from the web inspector.
Comment 1 Jason Liu 2012-03-23 04:02:22 PDT
Created attachment 133455 [details]
Patch
Comment 2 Leo Yang 2012-03-25 22:50:24 PDT
Comment on attachment 133455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> +
> +        if (m_response.httpHeaderFields().contains("Set-Cookie"))
> +            return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);

m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one?
Comment 3 Jason Liu 2012-03-25 22:52:32 PDT
(In reply to comment #2)
> (From update of attachment 133455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review
> 
> > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > +
> > +        if (m_response.httpHeaderFields().contains("Set-Cookie"))
> > +            return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> 
> m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one?

Yes, I catenate them so that overwrite doesn't happen.
Comment 4 Leo Yang 2012-03-25 23:19:34 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 133455 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review
> > 
> > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > +
> > > +        if (m_response.httpHeaderFields().contains("Set-Cookie"))
> > > +            return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > 
> > m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one?
> 
> Yes, I catenate them so that overwrite doesn't happen.

But you are setting "Set-Cookie" header twice for the first "Set-Cookie" header, which is a waste. Why use "\r\n" as delimiter? What do the other ports use?
Comment 5 Jason Liu 2012-03-26 02:36:24 PDT
Created attachment 133754 [details]
Patch
Comment 6 Leo Yang 2012-03-26 02:47:19 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 133455 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=133455&action=review
> > > 
> > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > > +
> > > > +        if (m_response.httpHeaderFields().contains("Set-Cookie"))
> > > > +            return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > > 
> > > m_response.setHTTPHeaderField(key, value) has been called at the end of this function. Why this is needed here? Are you trying to catenate all 'Set-Cookie' headers to one?
> > 
> > Yes, I catenate them so that overwrite doesn't happen.
> 
> But you are setting "Set-Cookie" header twice for the first "Set-Cookie" header, which is a waste. Why use "\r\n" as delimiter? What do the other ports use?

Never mind, you returned. But is "\r\n" the best delimiter? How other ports deal with this case?
Comment 7 Konrad Piascik 2012-03-26 06:55:20 PDT
> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> +            return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);

why do you return a value from a void method?
Comment 8 Jason Liu 2012-03-26 18:44:44 PDT
Created attachment 133958 [details]
Patch
Comment 9 Jason Liu 2012-03-26 18:48:08 PDT
(In reply to comment #7)
> > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > +            return m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> 
> why do you return a value from a void method?

Sorry, I forgot updating yesterday.
Comment 10 Konrad Piascik 2012-03-26 18:59:01 PDT
> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);

I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
Comment 11 Jason Liu 2012-03-26 19:21:20 PDT
(In reply to comment #10)
> > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> 
> I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?

I am compiling QT. Trying to find QT's implementation.
Comment 12 Jason Liu 2012-03-26 20:01:20 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > 
> > I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
> 
> I am compiling QT. Trying to find QT's implementation.

Qt uses comma(",") to separate them.
Comment 13 Leo Yang 2012-03-26 20:12:44 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > > 
> > > I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
> > 
> > I am compiling QT. Trying to find QT's implementation.
> 
> Qt uses comma(",") to separate them.

Where does qt porting do this? Network stack? or WebCore?
Comment 14 Jason Liu 2012-03-26 20:17:42 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > > 
> > > I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
> > 
> > I am compiling QT. Trying to find QT's implementation.
> 
> Qt uses comma(",") to separate them.

But Qt's inspector doesn't show ",".
Qt's inspector:
Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:11:32 GMT
cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:11:32 GMT
cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:11:32 GMT
cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:11:32 GMT

Playbook's inspector(using ","):
Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:09:22 GMT

I think "\r\n" looks better for our code.
Comment 15 Jason Liu 2012-03-26 20:18:20 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > > > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > > > 
> > > > I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
> > > 
> > > I am compiling QT. Trying to find QT's implementation.
> > 
> > Qt uses comma(",") to separate them.
> 
> Where does qt porting do this? Network stack? or WebCore?

Network stack.
Comment 16 Jason Liu 2012-03-28 00:09:42 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > > > > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > > > > 
> > > > > I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
> > > > 
> > > > I am compiling QT. Trying to find QT's implementation.
> > > 
> > > Qt uses comma(",") to separate them.
> > 
> > Where does qt porting do this? Network stack? or WebCore?
> 
> Network stack.

"\r\n" or "," (In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > > > > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> > > > 
> > > > I still would like to know about the delimiter, as Leo mentioned.  Can you tell us whether the delimiter of \r\n is a good choice and if other ports use it?
> > > 
> > > I am compiling QT. Trying to find QT's implementation.
> > 
> > Qt uses comma(",") to separate them.
> 
> But Qt's inspector doesn't show ",".
> Qt's inspector:
> Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:11:32 GMT
> cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:11:32 GMT
> cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:11:32 GMT
> cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:11:32 GMT
> 
> Playbook's inspector(using ","):
> Set-Cookie:cookieONE=ONE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTWO=TWO; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieTHREE=THREE; expires=Tue, 27-Mar-2012 04:09:22 GMT,cookieFOUR=FOUR; expires=Tue, 27-Mar-2012 04:09:22 GMT
> 
> I think "\r\n" looks better for our code.

"\r\n" or "," only affect inspector's showing.
Comment 17 Rob Buis 2012-03-28 04:17:03 PDT
Comment on attachment 133958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133958&action=review

> Source/WebCore/ChangeLog:6
> +        We shouldn't overwrite cookies of response if there are more than one

Don't you mean "one or more"?

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:318
> +        if (m_response.httpHeaderFields().contains("Set-Cookie")) {

Is there an AtomicString/static String for Set-Cookie?

> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);

Is "\r\n" used like that all the time? Any constants?
Comment 18 Jason Liu 2012-03-28 19:07:17 PDT
(In reply to comment #17)
> (From update of attachment 133958 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133958&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        We shouldn't overwrite cookies of response if there are more than one
> 
> Don't you mean "one or more"?
> 
> > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:318
> > +        if (m_response.httpHeaderFields().contains("Set-Cookie")) {
> 
> Is there an AtomicString/static String for Set-Cookie?
> 
> > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:319
> > +            m_response.setHTTPHeaderField(key, m_response.httpHeaderField(key) + "\r\n" + value);
> 
> Is "\r\n" used like that all the time? Any constants?

1. I mean the second cookie shouldn't overwrite the first one of response.
2. No, there isn't.
3. "\r\n" is only used in this special case.
Comment 19 Rob Buis 2012-03-28 19:26:10 PDT
Comment on attachment 133958 [details]
Patch

Looks good.
Comment 20 WebKit Review Bot 2012-03-28 19:33:05 PDT
Comment on attachment 133958 [details]
Patch

Clearing flags on attachment: 133958

Committed r112494: <http://trac.webkit.org/changeset/112494>
Comment 21 WebKit Review Bot 2012-03-28 19:33:11 PDT
All reviewed patches have been landed.  Closing bug.