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 walter 2000-08-09 07:38:38 PDT
Mozilla M17 doesn't seem handle HTTP response 303 (See Other)
correctly (see
Comment 1 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 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 Asa Dotzler [:asa] 2000-08-10 17:16:30 PDT
Comment 4 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 Gagan 2000-09-15 10:53:46 PDT
wont make it for beta3.
Comment 6 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 Darin Fisher 2000-11-22 11:19:30 PST
*** Bug 58150 has been marked as a duplicate of this bug. ***
Comment 8 Gagan 2000-12-08 00:40:22 PST
http bugs to "Networking::HTTP"
Comment 9 Darin Fisher 2001-02-06 15:44:37 PST
nominating for moz 0.9
Comment 10 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 Darin Fisher 2001-02-28 23:41:10 PST
removed stale nsbeta3 keyword.
added nsbeta1 keyword.
Comment 12 Tom Mraz 2001-04-17 08:39:52 PDT
Seeing on linux too please platform/os->all
Comment 13 Darin Fisher 2001-04-30 10:51:39 PDT
my changes for bug 76866 will fix this bug as well.
Comment 14 Darin Fisher 2001-05-14 02:34:07 PDT
fixed with branch landing
Comment 15 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 Darin Fisher 2001-05-22 08:59:36 PDT
-> 0.9.2
Comment 17 Gagan 2001-06-20 17:43:35 PDT
Comment 18 Darin Fisher 2001-06-21 19:20:32 PDT
*** Bug 81636 has been marked as a duplicate of this bug. ***
Comment 19 Tom Mraz 2001-09-25 06:39:46 PDT
Updating dependency on the duplicate.
Comment 20 Darin Fisher 2001-11-27 12:18:54 PST
-> 0.9.8
Comment 21 Darin Fisher 2002-01-11 19:01:24 PST
-> moz 1.0 hopefully
Comment 22 Mark Slater 2002-01-17 12:53:48 PST
Is bug 112208 related to this?
Comment 23 Darin Fisher 2002-01-18 14:28:27 PST
Comment 24 Darin Fisher 2002-03-28 09:47:18 PST
post 1.0
Comment 25 Darin Fisher 2002-05-17 16:26:05 PDT
mass futuring of untargeted bugs
Comment 26 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 Darin Fisher 2002-07-26 11:24:55 PDT
targeting mozilla 1.2 to get this fixed.
Comment 28 Darin Fisher 2002-09-11 14:25:45 PDT
looking like this might slip...
Comment 29 Darin Fisher 2002-10-11 00:24:25 PDT
-> moz 1.3
Comment 30 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 Darin Fisher 2003-03-05 14:22:58 PST
-> suresh
Comment 32 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 tenthumbs 2003-03-18 10:50:03 PST
s/resend/forward/ in the message?
Comment 34 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 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 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 Suresh 2003-03-19 15:08:05 PST
Created attachment 117776 [details] [diff] [review]
updated patch.
Comment 38 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 Suresh 2003-03-20 13:11:38 PST
Created attachment 117898 [details] [diff] [review]
updated patch.
Comment 40 Darin Fisher 2003-03-20 16:35:03 PST
Comment on attachment 117898 [details] [diff] [review]
updated patch.

looks great.. thx suresh!

Comment 41 Suresh 2003-03-20 16:36:37 PST
Thanks darin for your help and review comments!!
Comment 42 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 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 Suresh 2003-03-25 10:55:26 PST
This is fixed in trunk!!
Comment 45 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.