95044 - Cookies set on a 401 are not sent back the when a auth is resent.

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect, P3)

Description

[J. Nick Koston]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.3) Gecko/20010802
BuildID:    

When a 401 is sent with a cookie in the headers mozilla doesn't send back the
cookie until you hit cancel and reload the page.




Reproducible: Always
Steps to Reproduce:
1. Send 401 header and a cookie
2. Cookie doesn't get sent back until cancel is pressed and page is reloaded.


Actual Results:  Cookie not sent.

Expected Results:  Cookie should be sent.

This basiclly breaks the a few select few auth schemes.

Comment 1

[Stephen P. Morse]

Need more details here.  In particular a detailed set of steps for reproducing 
this, including the specific URL used.

Comment 2

[J. Nick Koston]

http://www.burst.net/~nick/mozilla_cookie_bug/setcookie.cgi

Wrote a script to demo this.

Comment 3

[Stephen P. Morse]

Confirming and accepting

Comment 4

[Stephen P. Morse]

Here's what's happening:

1. Go to http://www.burst.net/~nick/mozilla_cookie_bug/setcookie.cgi
2. Click on "here"
3. This causes a GET to http://www.burst.net/~nick/mozilla_cookie_bug/auth.php
4. An authentication dialog is brought up although the page has not yet
   finished loading
5. Fill in the authentication ("test", "test") and press OK
6. The nsHTTPChannel::SetRequestHeader is entered to set cookie header
7. mIsPending is true so this routine does not execute (contains NS_ENSURE_TRUE)
8. Therefore the cookie header has not been updated and the server gets the
   wrong value for the cookie.

So the problem is caused by mIsPending being true in step 7.  It was set to TRUE 
by a preceding call to nsHTTPChannel::AsyncOpen and should have been set back to 
FALSE by a call to nsHTTPChannel::OnStopRequest.  But the call to OnStopRequest 
never occured because the page never finished loading.

A simple patch that would fix this is simply to remove the following line at the 
head of nsHttpChannel::SetRequestHeader

   NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);

So I'm requesting a review of this patch.  If the reviewer feels that there is a 
more fundamental problem here (such as why the page never stops loading), please 
suggest a patch to fix the underlying problem.

cc'ing some networking people since the patch is in nsHTTPChannel, as well as 
some layout people since the problem is caused by the page not finishing loading 
at the time that the authentication dialog is displayed.

rpotts, darin, please review

Comment 5

[Stephen P. Morse]

Attachment: bypass NS_ENSURE_TRUE in SetRequestHeader

Comment 6

[Darin Fisher]

that assertion is there for a very good reason... a better fix would be to
toggle the mIsPending flag around the call to OnModifyRequest.

Comment 7

[Darin Fisher]

Attachment: alternate solution

Comment 8

[Stephen P. Morse]

Yes, this latter patch also fixes the problem.

r=morse

rpotts, please sr.  Thanks.

Comment 9

[rpotts (gone)]

Is this the only situation where an existing channel is reused?  I'm wondering
if there are any other situations where it is allowable to modify 'request
attributes' after a connection has been initiated...

If not, then i guess this approach is ok...  

Do we need to worry about the possibility of an OnModifyRequest listener calling
Cancel(...) ?  If so, could mIsPending get incorrectly set to PR_TRUE?  Or will
the proxy listener code defer calling nsHttpChannel::OnStopRequest(...) until
after nsHttpChannel::ProcessAuthentication() has returned...

If Cancelling is not an issue, then r/sr=rpotts

Comment 10

[Stephen P. Morse]

darin, can you answer the questions that rick raised?

Comment 11

[Darin Fisher]

authentication is the only case in which we reuse an existing http channel.. i
also confirmed that it is the only other place in which we call OnModifyRequest.

cancelation during an OnModifyRequest callback is almost a NOP because both
mTransaction and mCacheReadRequest would be NULL (mStatus is however set). 
nsHttpChannel::Cancel should probably return an error if !mIsPending, since it
doesn't make any sense to cancel a transaction that is not pending, or we could
just make it a NOP.

Comment 12

[Paul Chen]

nav triage team:

Marking p3

Comment 13

[Stephen P. Morse]

Fix checked in.

Comment 14

[Judson Valeski]

we'll need this on the 0.9.2 branch as well (after a day or so of shakeout on
the trunk).

Comment 15

[Paul Chen]

taking from morse since he's going on vacation

Comment 16

[Paul Chen]

fix checked into branch

Comment 17

[Tom Everingham]

verified-on-trunk:

WinNT4 2001082803
Linux rh6 2001082908
Mac os9 2001082908

Comment 18

[alberto chan]

*** Bug 99092 has been marked as a duplicate of this bug. ***

Comment 19

[Tom Everingham]

verified:

Linux rh6 2001-09-24-04-0.9.4

Comment 20

[Erica Wright [:ewright]]

Attachment: Bug 1671940 - Themes should be able to be set from the new tab page.