walter Reporter
	Description • 24 years ago

Mozilla M17 doesn't seem handle HTTP response 303 (See Other)
correctly (see http://www.w3.org/Protocols/rfc2616/rfc2616.html)

	Asa Dotzler [:asa]
	Comment 1 • 24 years ago

walter@data.franken.de can you provide more details.  See the bug writing
guiedlines at http://www.mozilla.org/quality/bug-writing-guidelines.html  Please
at least describe what you see that we're doing wrong and what you expect to see.

	walter Reporter
	Comment 2 • 24 years ago

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.

	Asa Dotzler [:asa]
	Comment 3 • 24 years ago

Networking

	Andreas M. "Clarence" Schneider
	Comment 4 • 24 years ago

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).

Testcase: http://clarence.de/post-test/form.html
(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".

	Gagan
	Comment 5 • 24 years ago

wont make it for beta3.

	Andreas M. "Clarence" Schneider
	Comment 6 • 24 years ago

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 http://clarence.de/post-test/form.html 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.

	Darin Fisher
	Comment 7 • 24 years ago

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

	Gagan
	Comment 8 • 24 years ago

http bugs to "Networking::HTTP"

	Darin Fisher
	Comment 9 • 24 years ago

nominating for moz 0.9

	Erik van der Poel
	Comment 10 • 24 years ago

Just an FYI. See the stuff about 301, 302 and 307 in the following:

  http://www.w3.org/TR/2001/NOTE-cuap-20010206#protocols

	Darin Fisher
	Comment 11 • 24 years ago

removed stale nsbeta3 keyword.
added nsbeta1 keyword.

	Tom Mraz
	Comment 12 • 24 years ago

Seeing on linux too please platform/os->all

	Darin Fisher
	Comment 13 • 24 years ago

my changes for bug 76866 will fix this bug as well.

	Darin Fisher
	Comment 14 • 24 years ago

fixed with branch landing

	Andreas M. "Clarence" Schneider
	Comment 15 • 24 years ago

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.

Reopening.
Changing summary "303 and 307" -> "301 and 307". 303 was fixed in bug 56785.

	Darin Fisher
	Comment 16 • 23 years ago

-> 0.9.2

	Gagan
	Comment 17 • 23 years ago

->0.9.3

	Darin Fisher
	Comment 18 • 23 years ago

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

	Tom Mraz
	Comment 19 • 23 years ago

Updating dependency on the duplicate.

	Darin Fisher
	Comment 20 • 23 years ago

-> 0.9.8

	Darin Fisher
	Comment 21 • 23 years ago

-> moz 1.0 hopefully

	Mark Slater
	Comment 22 • 23 years ago

Is bug 112208 related to this?

	Darin Fisher
	Comment 23 • 23 years ago

unlikely.

	Darin Fisher
	Comment 24 • 23 years ago

post 1.0

	Darin Fisher
	Comment 25 • 22 years ago

mass futuring of untargeted bugs

	Jaime Rodriguez, Jr.
	Comment 26 • 22 years ago

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!

	Darin Fisher
	Comment 27 • 22 years ago

targeting mozilla 1.2 to get this fixed.

	Darin Fisher
	Comment 28 • 22 years ago

looking like this might slip...

	Darin Fisher
	Comment 29 • 22 years ago

-> moz 1.3

	Darin Fisher
	Comment 30 • 22 years ago

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.

	Darin Fisher
	Comment 31 • 22 years ago

-> suresh

	Suresh Assignee
	Comment 32 • 22 years ago

Any suggestion for a better wording of the dialog?

	tenthumbs
	Comment 33 • 22 years ago

s/resend/forward/ in the message?

	Darin Fisher
	Comment 34 • 22 years ago

Comment on attachment 117618 [details] [diff] [review]
patch

>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/necko.properties

>+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!

	Suresh Assignee
	Comment 35 • 22 years ago

I'm yet to work with UE folks for a better wording in the dialog.

	Darin Fisher
	Comment 36 • 22 years ago

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::
GetInterface.

	Darin Fisher
	Comment 38 • 22 years ago

Comment on attachment 117776 [details] [diff] [review]
updated patch.

>Index: src/nsHttpChannel.cpp

>+nsHttpChannel::GetCallback(const nsIID &aIID, void **aResult)
>+{
>+    nsresult rv = NS_OK;
>+    NS_ENSURE_ARG_POINTER(aResult);
>+    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.

>+nsHttpChannel::PromptTempRedirect()
>+{
>+    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!

	Darin Fisher
	Comment 40 • 22 years ago

Comment on attachment 117898 [details] [diff] [review]
updated patch.

looks great.. thx suresh!

sr=darin

	Suresh Assignee
	Comment 41 • 22 years ago

Thanks darin for your help and review comments!!

	Doug Turner (:dougt)
	Comment 42 • 22 years ago

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.

	Jatin Billimoria
	Comment 43 • 22 years ago

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?" 

	Suresh Assignee
	Comment 44 • 22 years ago

This is fixed in trunk!!

	benc
	Comment 45 • 21 years ago

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.