Last Comment Bug 48202 - HTTP 307 redirect behavior violates RFC2616 [should preserve request method]
: HTTP 307 redirect behavior violates RFC2616 [should preserve request method]
[http/1.1 compliance] [adt3 RTM] che...
: embed, topembed+
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
P4 normal with 4 votes (vote)
: mozilla1.4alpha
Assigned To: Suresh
: Patrick McManus [:mcmanus]
: 58150 81636 (view as bug list)
Depends on: 76866 89419
Blocks: 68423 65092 156686
  Show dependency treegraph
Reported: 2000-08-09 07:38 PDT by walter
Modified: 2014-04-26 03:32 PDT (History)
17 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.30 KB, patch)
2003-03-18 10:34 PST, Suresh
darin.moz: superreview-
Details | Diff | Splinter Review
updated patch. (5.70 KB, patch)
2003-03-18 18:14 PST, Suresh
darin.moz: superreview-
Details | Diff | Splinter Review
updated patch. (5.54 KB, patch)
2003-03-19 15:08 PST, Suresh
darin.moz: superreview-
Details | Diff | Splinter Review
updated patch. (5.67 KB, patch)
2003-03-20 13:11 PST, Suresh
doug.turner: review+
darin.moz: superreview+
Details | Diff | Splinter Review

Description User image walter 2000-08-09 07:38:38 PDT
Mozilla M17 doesn't seem handle HTTP response 303 (See Other)
correctly (see
Comment 1 User image Asa Dotzler [:asa] 2000-08-09 13:27:33 PDT can you provide more details.  See the bug writing
guiedlines at  Please
at least describe what you see that we're doing wrong and what you expect to see.
Comment 2 User image walter 2000-08-10 10:58:35 PDT
What the RFC says, that Mozilla should do is the following: "The response to 
the request can be found under a different URI and SHOULD be retrieved using a 
GET method on that resource. This method exists primarily to allow the output 
of a POST-activated script to redirect the user agent to a selected resource. 
The new URI is not a substitute reference for the originally requested 
resource. The 303 response MUST NOT be cached, but the response to the second 
(redirected) request might be cacheable. The different URI SHOULD be given by 
the Location field in the response. Unless the request method was HEAD, the 
entity of the response SHOULD contain a short hypertext note with a hyperlink 
to the new URI(s)."

What Mozilla does is display the "entity of the response" as if it was a 200
response, without retrieving the page specified via the "Location" header.
Comment 3 User image Asa Dotzler [:asa] 2000-08-10 17:16:30 PDT
Comment 4 User image Andreas M. "Clarence" Schneider 2000-08-30 16:08:28 PDT
This shouldn't be futured. This is missing support for HTTP/1.1.
HTTP/1.1 response 307 (Temporary Redirect) isn't handled, too (2000083008,
Win NT).

(written for bug 46338; problems using it caused by bug 50143).

Walter, you should nominate this bug for nsbeta3 (add keyword "nsbeta3"; I'm
not allowed to do this). Reasons: Standards support, easy to fix.
Summary should read "303 and 307" instead of "303".
Comment 5 User image Gagan 2000-09-15 10:53:46 PDT
wont make it for beta3.
Comment 6 User image Andreas M. "Clarence" Schneider 2000-11-03 19:11:53 PST
The 303 case is fixed. See bug 56785. Verified with 10-30-04 trunk, Win NT.

For the 307 case we have a dupe: bug 58150.

I have rewritten my testcase at to make
it more useful for redirect bugs.

Additional issue:
HTTP 301 and 302 are handled wrong when used in the response to a form
submission. They should behave the same way a 307 should, that is re-submitting
the form data to the new location instead of performing a HTTP GET on it like
303 does.
For 302 the correct behavior may be unwise due to incorrect implementation in
most browsers and possible use of the unambiguous 307. But for 301 Mozilla
should conform to RFC 2616.
Comment 7 User image Darin Fisher 2000-11-22 11:19:30 PST
*** Bug 58150 has been marked as a duplicate of this bug. ***
Comment 8 User image Gagan 2000-12-08 00:40:22 PST
http bugs to "Networking::HTTP"
Comment 9 User image Darin Fisher 2001-02-06 15:44:37 PST
nominating for moz 0.9
Comment 10 User image Erik van der Poel 2001-02-12 22:32:24 PST
Just an FYI. See the stuff about 301, 302 and 307 in the following:
Comment 11 User image Darin Fisher 2001-02-28 23:41:10 PST
removed stale nsbeta3 keyword.
added nsbeta1 keyword.
Comment 12 User image Tom Mraz 2001-04-17 08:39:52 PDT
Seeing on linux too please platform/os->all
Comment 13 User image Darin Fisher 2001-04-30 10:51:39 PDT
my changes for bug 76866 will fix this bug as well.
Comment 14 User image Darin Fisher 2001-05-14 02:34:07 PDT
fixed with branch landing
Comment 15 User image Andreas M. "Clarence" Schneider 2001-05-14 14:51:47 PDT
307 works now, but not as it should (it seems to behave exactly like 303 does).
We should use the same method that was used on the old URI, i.e. if it was a
POST we should perform a POST to the new Location, using the old POST data.
RFC 2616 requires confirmation by the user before doing so (in case of a POST).
The same rules should be applied to 301.

Changing summary "303 and 307" -> "301 and 307". 303 was fixed in bug 56785.
Comment 16 User image Darin Fisher 2001-05-22 08:59:36 PDT
-> 0.9.2
Comment 17 User image Gagan 2001-06-20 17:43:35 PDT
Comment 18 User image Darin Fisher 2001-06-21 19:20:32 PDT
*** Bug 81636 has been marked as a duplicate of this bug. ***
Comment 19 User image Tom Mraz 2001-09-25 06:39:46 PDT
Updating dependency on the duplicate.
Comment 20 User image Darin Fisher 2001-11-27 12:18:54 PST
-> 0.9.8
Comment 21 User image Darin Fisher 2002-01-11 19:01:24 PST
-> moz 1.0 hopefully
Comment 22 User image Mark Slater 2002-01-17 12:53:48 PST
Is bug 112208 related to this?
Comment 23 User image Darin Fisher 2002-01-18 14:28:27 PST
Comment 24 User image Darin Fisher 2002-03-28 09:47:18 PST
post 1.0
Comment 25 User image Darin Fisher 2002-05-17 16:26:05 PDT
mass futuring of untargeted bugs
Comment 26 User image Jaime Rodriguez, Jr. 2002-06-08 10:41:46 PDT
darin/gagan - what are the chnaces that we'd be able to get a fix for this
before 06.14.2002? pls add eta. thanks!
Comment 27 User image Darin Fisher 2002-07-26 11:24:55 PDT
targeting mozilla 1.2 to get this fixed.
Comment 28 User image Darin Fisher 2002-09-11 14:25:45 PDT
looking like this might slip...
Comment 29 User image Darin Fisher 2002-10-11 00:24:25 PDT
-> moz 1.3
Comment 30 User image Darin Fisher 2003-01-30 01:56:16 PST
301 behavior is not going to change.  we are better off matching existing
browser behavior.  307 however should be revised to prompt the user and preserve
the method.
Comment 31 User image Darin Fisher 2003-03-05 14:22:58 PST
-> suresh
Comment 32 User image Suresh 2003-03-18 10:34:40 PST
Created attachment 117618 [details] [diff] [review]

Any suggestion for a better wording of the dialog?
Comment 33 User image tenthumbs 2003-03-18 10:50:03 PST
s/resend/forward/ in the message?
Comment 34 User image Darin Fisher 2003-03-18 11:10:54 PST
Comment on attachment 117618 [details] [diff] [review]

>Index: src/nsHttpChannel.cpp

i would move the prompting code into a subroutine.

>+        if (redirectType == 307 && mUploadStream) {
>+                if (mCallbacks) {
>+                    mCallbacks->GetInterface(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));

it is not enough to just check mCallbacks, you also need to check the
mLoadGroup's notification callbacks if mCallbacks does not provide a
prompter.  see for example PromptForUserPass.  the code to query a
callback interface should be moved into a subroutine that can be
shared by each of these routines.

>+            nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mUploadStream);
>+            if (seekable)
>+                seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);

it should be an error if the QI fails.	iirc, you can safely bail
from ProcessRedirection and the calling code will clean up ok.

>+            nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(httpChannel));
>+            if (uploadChannel)
>+                uploadChannel->SetUploadStream(mUploadStream, NS_LITERAL_CSTRING(""), -1);
>+        }

calling SetUploadStream like this is not quite right.  what about the
case in which SetUploadStream was called with an explicit contentType
and/or contentLength?  you need to duplicate the previous call to
SetUploadStream or this will not work in all cases.

>Index: locale/en-US/

>+RepostFormData=The server is redirecting to a new URL. Would you like to resend the data to this new location?

this sounds a bit awkward to me, but i'm no UE expert.	please run
this by someone from UE.  thx!
Comment 35 User image Suresh 2003-03-18 18:14:06 PST
Created attachment 117672 [details] [diff] [review]
updated patch.

I'm yet to work with UE folks for a better wording in the dialog.
Comment 36 User image Darin Fisher 2003-03-19 10:47:35 PST
Comment on attachment 117672 [details] [diff] [review]
updated patch.

>Index: src/nsHttpChannel.cpp

>+    nsCOMPtr<nsIPrompt> prompt;
>+    rv = GetPrompt(getter_AddRefs(prompt));
>+    if (NS_FAILED(rv)) return rv;
>+    nsCOMPtr<nsIAuthPrompt> authPrompt = do_QueryInterface(prompt, &rv);
>+    if (NS_FAILED(rv)) return rv;

unfortunately, this isn't quite right.	the object implementing nsIPrompt
is not guaranteed to implement nsIAuthPrompt.  there may be a separate
object implementing nsIAuthPrompt.  you have to call GetInterface with
the correct IID.  i suggest you rename GetPrompt as GetCallback, and 
allow the caller to pass in a nsIID, just like nsIInterfaceRequestor::
Comment 37 User image Suresh 2003-03-19 15:08:05 PST
Created attachment 117776 [details] [diff] [review]
updated patch.
Comment 38 User image Darin Fisher 2003-03-19 21:01:40 PST
Comment on attachment 117776 [details] [diff] [review]
updated patch.

>Index: src/nsHttpChannel.cpp

>+nsHttpChannel::GetCallback(const nsIID &aIID, void **aResult)
>+    nsresult rv = NS_OK;
>+    rv = mCallbacks->GetInterface(aIID, aResult);

no need to initialize |rv| since you are just going to overwrite
it immediately anyways.  also, the NS_ENSURE macro should just
be a debug only NS_ASSERTION.

>+    nsresult rv = NS_OK;
>+    nsCOMPtr<nsIStringBundleService> bundleService =
>+            do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);

no reason to initialize |rv|.  do_GetService always assigns a value.

>+    rv = stringBundle->GetStringFromName(NS_LITERAL_STRING("RepostFormData").get(), 
>+    if (NS_SUCCEEDED(rv) && messageString) {

can GetStringFromName really return NS_OK without setting messageString?
seems like you should only have to perform one of these checks, no?  hmm..
in reading nsStringBundle::GetStringFromName, it appears that it does not
null check the result of ToNewUnicode, so yes it is possible for this
function to return NS_OK and a NULL messageString.  that is very lame; we
should file a bug against nsStringBundle to fix that.  documenting this
here would be good, so we can come back and clean it up later.

>+            nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mUploadStream, &rv);
>+            if (NS_FAILED(rv)) return rv;
>+            if (seekable)
>+                seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0);

if QI succeeds then |seekable| will be non-null.  so you don't need to check
|seekable| after checking |rv|.  also you should check the return value from
Seek since it could easily result in an I/O error of some sort.

>+            nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(httpChannel));
>+            if (uploadChannel)

if this QI fails, then it should be considered an error.  just check |rv|.

>     static void  PR_CALLBACK AsyncCall_EventCleanupFunc(PLEvent *);
>+    nsresult GetCallback(const nsIID &aIID, void **aResult);
>+    nsresult PromptTempRedirect();

please move these function decls up after ProcessAuthentication.  thx!
Comment 39 User image Suresh 2003-03-20 13:11:38 PST
Created attachment 117898 [details] [diff] [review]
updated patch.
Comment 40 User image Darin Fisher 2003-03-20 16:35:03 PST
Comment on attachment 117898 [details] [diff] [review]
updated patch.

looks great.. thx suresh!

Comment 41 User image Suresh 2003-03-20 16:36:37 PST
Thanks darin for your help and review comments!!
Comment 42 User image Doug Turner (:dougt) 2003-03-20 17:08:27 PST
Comment on attachment 117898 [details] [diff] [review]
updated patch.

use do_GetInterface instead of a direct call on mCallbacks.  (i am pretty sure
that do_GetInterface does a test for null).

nit: In GetCallback, unwind the nesting and return immediately if getinterface
is successful.

if GetStringFromName does return success but returns a null string, shouldn't
we return an error?

Maybe we should ensure that if GetCallbacks returns rv == NS_OK, then the
resulting object is always valid.  This would avoid two tests.

rv = GetCallback(NS_GET_IID(nsIPrompt), getter_AddRefs(prompt));
+	 if (NS_FAILED(rv)) return rv;
+	 if (prompt) {

fix that stuff and r=dougt.
Comment 43 User image Jatin Billimoria 2003-03-24 15:01:56 PST
Suggested wording per Suresh's request:

"This web page is being redirected to a new location. Would you like to resend
the form data you have typed to the new location?" 
Comment 44 User image Suresh 2003-03-25 10:55:26 PST
This is fixed in trunk!!
Comment 45 User image benc 2003-06-10 16:01:47 PDT
VERIFIED: Mozilla 1.4b, Mach-O, Win98

The dialog comes up for post as described.

Should I also be checking the "don't cache redirect + cache Location:<URL>"
behavior too? I cannot tell if that was broken based on the original description.

Note You need to log in before you can comment on or make changes to this bug.