Cookies set using nsHttpChannel::setRequestHeader get cleared by the cookie service

RESOLVED FIXED in mozilla1.8beta4

Status

()

P1
normal
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: doronr, Assigned: Biesinger)

Tracking

({fixed1.8})

Trunk
mozilla1.8beta4
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

15 years ago
Trying to set cookies using nsIHttpChannel::setRequestHeader doesn't work since
when the channel is opened, the cookies get cleared and loaded in from the
cookie service.

Comment 1

15 years ago
In AsyncOpen, we call AddCookieToRequest.  That function calls
mRequestHead.SetCookie passing PR_FALSE, indicating that any existing value for
the Cookie header should be overwritten.  This function is also called from
DoAuthRetry.  We cannot simply merge cookies in AddCookieToRequest since that
might cause us to add the same cookie twice when calling AddCookieToRequest from
DoAuthRetry.  We cannot get around this by simply passing PR_FALSE when
AddCookieToRequest is called from DoAuthRetry either.  I think the right
solution is to store the value of the Cookie header prior to calling
AddCookieToRequest from AsyncOpen.  In AddCookieToRequest we should merge the
stored value with whatever the cookie service provides.
Component: Networking: Cookies → Networking: HTTP
Keywords: helpwanted
Target Milestone: --- → Future

Comment 2

15 years ago
darin, you make it too easy for us :)

-> me
Assignee: darin → dwitte
Keywords: helpwanted
Target Milestone: Future → mozilla1.8beta

Comment 3

15 years ago
>We cannot get around this by simply passing PR_FALSE when AddCookieToRequest is
>called from DoAuthRetry either.

hmm, can you elaborate? i'd note that the cookieservice's GetCookieString
delimits multiple cookies with ";", whereas nsHttpHeaderArray::SetHeader uses
"," when merging headers, so that could be a reason why we can't use the merging
functionality. but i don't think that's what you were getting at...

Comment 4

15 years ago
Created attachment 152745 [details] [diff] [review]
concatenate headers in AddCookiesToRequest

this does (i think) what darin said in comment 1, but i still don't see why the
merging wouldn't work.
what's the status of this patch?

Comment 6

14 years ago
Hrm... so with the current patch, wouldn't we lose the cookies set via
setRequestHeader if the channel goes through the DoAuthRetry code path?  It
seems like we need to store the cookies set via setRequestHeader in some member
variable so that we can merge them again after we have overwritten the value of
the "Cookie" request header.  Does this make sense?
Created attachment 192217 [details] [diff] [review]
unit test for this bug

adds a unit test for this bug. The reason I didn't add it to
test_http_headers.js is that this testcase depends on a working network
connection. (but maybe that would not be a problem?)

should we have an XFAIL list for tests that are currently known to fail?

A makefile target that runs these tests would also be nice
Assignee: dwitte → cbiesinger
Status: NEW → ASSIGNED
Attachment #192217 - Flags: review?(darin)
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: mozilla1.8beta1 → mozilla1.8beta4
Comment on attachment 192217 [details] [diff] [review]
unit test for this bug

I have a better testcase...
Attachment #192217 - Attachment is obsolete: true
Attachment #192217 - Flags: review?(darin)

Comment 9

14 years ago
I think we should make the tinderbox turn orange if any of these tests fail.
Created attachment 192221 [details] [diff] [review]
test + patch
Attachment #152745 - Attachment is obsolete: true
Attachment #192221 - Flags: review?(darin)
Attachment #192221 - Flags: superreview?(darin)
this patch differs from the original patch in this bug in that it appends the
user-set headers after the ones from the cookie service. I did it that way
assuming that later headers would override earlier ones, which seems like the
desired behaviour for explicitly-set headers vs implicit ones.
(In reply to comment #9)
> I think we should make the tinderbox turn orange if any of these tests fail.

filed bug 304162

Comment 13

14 years ago
Comment on attachment 192221 [details] [diff] [review]
test + patch

Looks good, r+sr=darin
Attachment #192221 - Flags: superreview?(darin)
Attachment #192221 - Flags: superreview+
Attachment #192221 - Flags: review?(darin)
Attachment #192221 - Flags: review+
Comment on attachment 192221 [details] [diff] [review]
test + patch

low-risk patch to allow XMLHttpRequest users (and other necko-using code) to
set a Cookie header
Attachment #192221 - Flags: approval1.8b4?
Comment on attachment 192221 [details] [diff] [review]
test + patch

actually, I have a slightly better testcase
Attachment #192221 - Flags: approval1.8b4?
Created attachment 192300 [details] [diff] [review]
test + patch, v1.1

the only difference is that the unit test now only does its second part if the
first one succeeded.
Attachment #192221 - Attachment is obsolete: true
Attachment #192300 - Flags: approval1.8b4?

Comment 17

14 years ago
Comment on attachment 192300 [details] [diff] [review]
test + patch, v1.1

moving r+sr forward and approving
Attachment #192300 - Flags: superreview+
Attachment #192300 - Flags: review+
Attachment #192300 - Flags: approval1.8b4?
Attachment #192300 - Flags: approval1.8b4+
checked in except for the test_all.sh change, which got checked in as part of
bug 63368

HEAD:
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.257; previous revision: 1.256
done
Checking in netwerk/protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.71; previous revision: 1.70
done
Checking in netwerk/test/Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.86; previous revision: 1.85
done
Checking in netwerk/test/unit/test_http_headers.js;
/cvsroot/mozilla/netwerk/test/unit/test_http_headers.js,v  <--  test_http_headers.js
new revision: 1.6; previous revision: 1.5
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v
done
Checking in netwerk/test/unit/test_cookie_header.js;
/cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v  <-- 
test_cookie_header.js
initial revision: 1.1
done

MOZILLA_1_8_BRANCH:
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <-- 
nsHttpChannel.cpp
new revision: 1.256.2.1; previous revision: 1.256
done
Checking in netwerk/protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.70.4.1; previous revision: 1.70
done
Checking in netwerk/test/Makefile.in;
/cvsroot/mozilla/netwerk/test/Makefile.in,v  <--  Makefile.in
new revision: 1.85.2.1; previous revision: 1.85
done
Checking in netwerk/test/unit/test_cookie_header.js;
/cvsroot/mozilla/netwerk/test/unit/test_cookie_header.js,v  <-- 
test_cookie_header.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in netwerk/test/unit/test_http_headers.js;
/cvsroot/mozilla/netwerk/test/unit/test_http_headers.js,v  <--  test_http_headers.js
new revision: 1.5.2.1; previous revision: 1.5
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.