291230 - HTTP downloads don't progress beyond (filesize % 4GB)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Mark Mentovai]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050420 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050420 Firefox/1.0+

On the trunk, HTTP downloads don't progress beyond 1GB.  I have confirmed that
this behavior occurs on the trunk as far back as 2005-04-05.  That's the
earliest build I can get my hands on.  This bug was NOT present in the Mac 1.7
Seamonkey nightly from 2005-04-15, which makes this a regression.

Confirmed in Firefox/Mac OS X/trunk/2005-04-20
Firefox/Win32/trunk/2005-04-17
Seamonkey/Mac OS X/trunk/2005-04-05
Camino/Mac OS X/trunk/2005-04-20

When attempting to download a 5GB file, the download stops making progress after
1GB has been downloaded.  The progress bar freezes at the 1GB mark.  Network
activity continues, and tcpdump shows that the file is being received beyond
1GB, but it is not written to disk.  The file size is stuck at 1073741824 bytes.

Downloading a 5GB file over FTP is successful.  All 5GB are downloaded. 
Downloading a file over HTTP using the Seamonkey 1.7 branch nightly build from
2005-04-15 progresses past the 1GB mark.

Reproducible: Always

Steps to Reproduce:
Attempt to download a 5GB file.  (Sorry, no URL, set up your own test server and
make sure that it supports large files.)
Actual Results:  
Mozilla products from the trunk made no progress after downloading exactly 1GB.

Expected Results:  
It should have downloaded the file in its entirety.

Comment 1

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

> set up your own test server and make sure that it supports large files.)

requires finding a server that supports files > 4 GB... apache failed last I tried.

does this also happen if the file is, say, 3 GB large?

Comment 2

[Mark Mentovai]

I am using Apache 2 built with large file support.  For reasons I'm sure
everyone will understand, I can't provide a test server.  I will test with a 3GB
file later.

Comment 3

[Mark Mentovai]

OK.  It's not a static 1GB.  The bug only manifests itself when the file size is
4GB or more.  The download stops progressing at (filesize % 4GB), so when I was
testing with a 5GB file, it would give up at 1GB.  With a 4097MB test file, it
gives up at 1MB.  And with a 4GB test file (exactly), neither the fox nor the
lizard even present a "what do you want to do with this file?" dialog or display
any download progress indicator at all.  In all cases, the file continues to be
transmitted over the network, but nothing is written to disk once the stopping
point is hit, and the status bar does not make any progress.

3GB and 1536MB test files were fine.  You really need to push large files over
HTTP to get the bug to show itself.  You could toss together a CGI that faked it
if necessary.

Updated summary to reflect new findings.

Comment 4

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

wow, that's odd... just to make sure, the server is sending the correct
Content-Length header, right?

Comment 5

[Mark Mentovai]

Of course, the Content-Length is correct.  The client reads the proper
Content-Length too, because it sets the maximum size for the progress bar and
status text appropriately.

mark@sylvester bash$ curl -I http://mandangalo/~mark/5120M.bin
HTTP/1.1 200 OK
Date: Thu, 21 Apr 2005 11:41:48 GMT
Server: Apache/2.0.53
Last-Modified: Wed, 20 Apr 2005 21:08:25 GMT
ETag: "354a74-40000000-a0340440"
Accept-Ranges: bytes
Content-Length: 5368709120
Content-Type: application/octet-stream
X-Pad: avoid browser bug

Comment 6

[Mark Mentovai]

Could this have something to do with the cache, which still seems to be working
on 32 bits?

Comment 7

[Darin Fisher]

The cache stops storing data when the size of the file would exceed some limit
(like 25Mb).  Moreover, the downloading code should be independent of the cache.
 We write to the cache if we can as the file is being downloaded, but the cache
should not be part of the critical path here.  You can try disabling the cache
for HTTP downloads by setting the pref network.http.use-cache to false.

Comment 8

[Mark Mentovai]

I believe I've got this one.  Stand by for patch.

Comment 9

[Mark Mentovai]

Attachment: Allow downloads > 4GB for persistent non-chunked HTTP connections

This turned out to be a problem only for persistent non-chunked HTTP
connections.  Of course, that's what most HTTP connections are these days. 
There was a single truncation to 32 bits in that path that made HandleContent
refuse to allow progress, because *contentRead would always be zero once the
download got further than mContentLength % 4G bytes.

This bug has been present since the fix for bug 243974, but since that's where
support for >4GB downloads was initially introduced, this isn't a regression.

Comment 10

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

Comment on attachment 181648 [details] [diff] [review]
Allow downloads > 4GB for persistent non-chunked HTTP connections

thanks for finding this!

Comment 11

[Darin Fisher]

Comment on attachment 181648 [details] [diff] [review]
Allow downloads > 4GB for persistent non-chunked HTTP connections

>Index: mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp

>+            *contentRead = PR_MIN(nsInt64(count),
>+                                  mContentLength - mContentRead);

You should use a temporary variable here since PR_MIN expands
to computing the difference twice.

  nsInt64 contentRemaining = mContentLength - mContentRead;
  *contentRead = PR_MIN(nsInt64(count), contentRemaining);

sr=darin with that change.

Comment 12

[Darin Fisher]

Please attach a revised patch, and we'll see about getting it checked in.  Thanks!

Comment 13

[Mark Mentovai]

Attachment: v2/Allow downloads > 4GB for persistent non-chunked HTTP connections

Updated to use temporary variable.

Comment 14

[Mark Mentovai]

Attachment: v3/Allow downloads > 4GB for persistent non-chunked HTTP connections

Darin, I realized that the existing code already has the same inefficiency that
a sum is computed twice in the code path for non-persistent HTTP connections. 
This revised patch fixes that too.  Take it or leave it, your call.  I didn't
use PR_MAX because the commented-out call to SetContentLength is easier to
follow without it.  The only thing I don't like about this is that the same sum
gets computed yet again a few lines later, but only an ugly logic
reorganization could solve that.

Comment 15

[Darin Fisher]

Comment on attachment 181696 [details] [diff] [review]
v3/Allow downloads > 4GB for persistent non-chunked HTTP connections

>Index: mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp

>+            *contentRemaining = count - *contentRead;
>         }
>         else {
>             *contentRead = count;
>             // mContentLength might need to be increased...
>-            if (nsInt64(*contentRead) + mContentRead > mContentLength) {
>-                mContentLength = nsInt64(*contentRead) + mContentRead;
>+            nsInt64 position = mContentRead + nsInt64(count);
>+            if (position > mContentLength) {
>+                mContentLength = position;
>                 //mResponseHead->SetContentLength(mContentLength);
>             }
>         }
>-        *contentRemaining = (count - *contentRead);

now, it appears that contentRemaining is not updated in the |else|
clause.  how about we go with the v2 patch then?

Comment 16

[Darin Fisher]

Comment on attachment 181696 [details] [diff] [review]
v3/Allow downloads > 4GB for persistent non-chunked HTTP connections

ah, but there is no reason to update it in the else clause.  ok, well done :-)

Comment 17

[Mark Mentovai]

Comment on attachment 181667 [details] [diff] [review]
v2/Allow downloads > 4GB for persistent non-chunked HTTP connections

I was wondering if anyone would catch that bit about contentRemaining.	Shoulda
taken more context for the diff.  Marking v2 obsolete for clarity.

Comment 18

[Mark Mentovai]

This should block 1.1 for sure, but it would be nice to slip it in to 1.8b2/1.1a
in the mean time, to let all off the other work on large file downloads shine.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 181696 [details] [diff] [review]
v3/Allow downloads > 4GB for persistent non-chunked HTTP connections

a=asa

Comment 20

[Mark Mentovai]

biesi?  darin?  Can one of you guys check v3 in?

Comment 21

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

unless someone beats me to it, I can do it tomorrow.

Comment 22

[Darin Fisher]

got it; fixed-on-trunk