547239 - Post body not preserved after HTTP 307 redirection, in Firefox 3.6

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[holy834]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

In Firefox 3.6, post body not preserved after HTTP 307 redirection. Firefox 3.5.2 to 3.5.8(I verified that) do not have this issue and handle correctly.

In the second POST request which responds to 307, Firefox 3.6 sends a request with no post data, and "Content-Length: 0" header.

The site in repro step is just an example. It can be reproed on other sites as well.

Reproducible: Always

Steps to Reproduce:
1. open the URL: http://www.andypemberton.com/sandbox/prg/
2. select "Apply" and "307 Temporary Redirect", fill something in the input box.
3. click "Submit" and click ok if there's a prompt window.
4. watch the second POST request sent by Firefox. 
Actual Results:  
There's no post data in the second POST request.

Expected Results:  
There should have something, same as the first one, in the second request.

Log from NSPR_LOG:

0[828140]: http request [
0[828140]:   POST /sandbox/prg/prg_redirect.php HTTP/1.1
0[828140]:   Host: www.andypemberton.com
0[828140]:   User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
0[828140]:   Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
0[828140]:   Accept-Language: en-us,en;q=0.5
0[828140]:   Accept-Encoding: gzip,deflate
0[828140]:   Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
0[828140]:   Keep-Alive: 115
0[828140]:   Connection: keep-alive
0[828140]:   Referer: http://www.andypemberton.com/sandbox/prg/
0[828140]: ]

3796[828780]: http response [
3796[828780]:   HTTP/1.1 307 Temporary Redirect
3796[828780]:   Date: Fri, 19 Feb 2010 14:16:10 GMT
3796[828780]:   Server: Apache/2.0.59 (FreeBSD) PHP/5.2.3 with Suhosin-Patch DAV/2
3796[828780]:   X-Powered-By: PHP/5.2.3
3796[828780]:   Location: prg_view.php
3796[828780]:   Content-Length: 0
3796[828780]:   Keep-Alive: timeout=15, max=99
3796[828780]:   Connection: Keep-Alive
3796[828780]:   Content-Type: text/html; charset=ISO-8859-1
3796[828780]: ]

0[828140]: http request [
0[828140]:   POST /sandbox/prg/prg_view.php HTTP/1.1
0[828140]:   Host: www.andypemberton.com
0[828140]:   User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
0[828140]:   Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
0[828140]:   Accept-Language: en-us,en;q=0.5
0[828140]:   Accept-Encoding: gzip,deflate
0[828140]:   Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
0[828140]:   Keep-Alive: 115
0[828140]:   Connection: keep-alive
0[828140]:   Referer: http://www.andypemberton.com/sandbox/prg/
0[828140]:   Content-Length: 0
0[828140]: ]

Comment 1

[Boris Zbarsky [:bzbarsky]]

This was caused by bug 491201.  It's peeking the content length header and not preserving the POST data if the header is not set, even in cases when the stream contains headers.

Comment 2

[Boris Zbarsky [:bzbarsky]]

And ExplicitSetUploadStream is also buggy if aStreamHasHeaders is true (because it then sets the Content-Length header to a length that includes the headers in the upload stream!).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Or at least it would if called with any negative value of aContentLength.  If called with a positive one, it would just use that value.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Should fix the code

This should fix the code.  Needs verification and automated tests.  These last should be extra fun, since they involve that little redirect prompt...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I wonder if this could have caused Bug 542642.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 427778 [details] [diff] [review]
Should fix the code

Note that the network.http.prompt-temp-redirect pref is set to false during mochitests, so no prompt will appear.

r=me if you add tests.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Olli, I really doubt it would have, unless gmail brings up that redirect dialog.

Jonas, my point is that I don't have time to write the tests right now (at the very least today).  If Matin is not available, I can try to do it, I guess...

Comment 10

[Boris Zbarsky [:bzbarsky]]

For all the people tempted to mark bugs as duplicates of this one:

If you're not seeing a prompt about redirecting the POST, then you are NOT seeing this bug.

Comment 11

[Daniel Veditz [:dveditz]]

We need to fix this regression in 3.6.x, but we need tests to check in with it.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sure.  My question is whether the person who caused the regression or those who reviewed it have time to write those tests, or whether I need to make time to do it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So I just realized that this could also affect people in two more cases, since those also pass true for preserveMethod to SetupReplacementChannel:

1)  Proxies.  At least any codepath that ends up in DoReplaceWithProxy.
2)  Fallback to application cache on load failure.

#2 I would think is rare, but I'm surprised we haven't gotten more reports of #1!

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK, at this point we have at least 3 bug reports of POST issues that have in fact been tracked down to proxy use.  I'll mark them dependent on this bug.  And I'll try to write tests here ASAP.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: With some tests

This adds a test for 307 redirects.  I'd love to know how to test the DoReplaceWithProxy codepath, though.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed that as http://hg.mozilla.org/mozilla-central/rev/1dc9a2628570 to get some testing+baking on this on trunk, so we can get it to branch.  biesi's not back until Monday, but I hope he can double-check this code then.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wouldn't it be better to simply an sjs file to do a redirect and test that the body got submitted even in the redirected request? That way you don't need to reply on enablePriviledge

Comment 18

[Boris Zbarsky [:bzbarsky]]

> That way you don't need to reply on enablePriviledge

The test doesn't rely on enablePrivilege...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 428625 [details] [diff] [review]
With some tests

We should get this in on branch.  The risk is "somewhat", but I think the only cases that change behavior are the ones that were broken anyway, and this is causing serious user problems right now.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Needs reviews and to land on trunk before we can take it on branch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I did land this on trunk; see comment 16.

But yes, I'd like jduell and biesi to look at it before we land on branch, as long as the branch landing can wait till next week and still make 1.9.2.2.

Comment 22

[Jason Duell]

Comment on attachment 428625 [details] [diff] [review]
With some tests

Looks good.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 428625 [details] [diff] [review]
With some tests

looks good, some nits:

+    LOG(("DoReplaceWithProxy from @%x [pi=%p]", this, pi));

%x -> %p, so that this works on 64-bit systems?

Can you also use [this=%p], for consistency with most other logs?

I see that this entire file gets the %x wrong :(

+        mRequestHead.SetHeader(nsHttp::Content_Length, nsPrintfCString("%lld", aContentLength));

80 characters please

+  chan.QueryInterface(Ci.nsIUploadChannel).setUploadStream(uploadStream,
+							   "text/plain",
+							   -1);

the last two lines need an additional space of indentation

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 428625 [details] [diff] [review]
With some tests

a1922=beltzner

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I see that this entire file gets the %x wrong :(

I'll just fix throughout.

> 80 characters please

Done.

> the last two lines need an additional space of indentation

Or rather need de-tabifying.  Done.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch to address biesi's review comments

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up patch for 1.9.2

Comment 28

[Boris Zbarsky [:bzbarsky]]

Pushed additional patch on m-c:

  http://hg.mozilla.org/mozilla-central/rev/fd3719c9d884

and 1.9.2 fix:

  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0c2eccb5266e

Comment 29

[holy834]

Thank you Boris, and everyone else here who made this fix!

Comment 31

[Carsten Book [:Tomcat]]

verified 1.9.2-2 with  Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2) Gecko/20100321 Firefox/3.6.2 ID:20100321170417 while using the testpage from comment #0