243974 - Download Manager can't handle files > 4gb

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Stefan Kopic]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

The Download Manager has problems with files over 4gb. For example the DVD-ISO
from Linux Fedora Core 2 (=> URL) has a size of 4gb+79mb. The download manager
shows only the 79mb and cancels the download after the 79mb are reached.

Other software like e.g. Opera 7.5 has similar problems (shows only 79mb, too),
but doesn't stop the transfer.

Reproducible: Always
Steps to Reproduce:
1. start a download with a size bigger than 4gb (e.g. 4gb+79mb)
2. download manager shows wrong size (79mb)
2. wait until the file size above the 4gb is reached (=> >79mb)

Actual Results:  
The download stops after 79mb.

Expected Results:  
- recognize the real file size
- not cancel the download

Some other programs have similar problems. Checked with wget 1.8 (Linux),
Leechget (WinXP), Opera 7.5 (WinXP). Only Opera continued downloading but
without showing the real file size or correct estimated transfer time.

Tried to save the file on a WinXP NTFS partition that can handle files >4gb.

Comment 1

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

hmm, interesting. haven't seen this specific bug before.

probably httpchannel truncates the content-length to 32bit, and stops reading
from the socket after having read the truncated number of bytes?

Comment 2

[Darin Fisher]

yeah, that's probably true.  if the file were served using 'Transfer-Encoding:
chunked', then it's possible that we might be able to download the whole thing.
 if a Content-Length header is specified, then we will indeed treat it as a
32-bit numeric value :-(

Comment 3

[Makoto Kato [:m_kato]]

dup 184452, please.

Comment 4

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

(In reply to comment #3)
> dup 184452, please.

hmm, this is about a very specific problem of the http channel... don't think it
should be marked as duplicate

Comment 5

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

Attachment: patch

this is not exactly the minimal patch to fix this bug... it contains parts of
bug 227057, but is not complete for that (nsIResumableEntityID should really
just be a string...) but it would be some effort to untangle that now, so I
decided to leave this as-is; if you want I can try to split this up again.

Yes, the XPCOM part is needed. Otherwise, nsInputStreamPump cancels the
channel, thinking the consumer didn't read any data, while in fact the counter
just overflowed and a number like 10 is smaller than a number like
PR_UINT32_MAX.

with this patch, TestProtocols can successfully read a 4294967306 bytes file
(should be 4 GB + 10 bytes) over HTTP (using a simple script as a "server",
apache doesn't seem to support files > 4 GB on x86, much to my annoyance)

other things that would need doing is make some (more) interfaces use 64 bit
numbers (like nsIWebProgressListener, nsITransportEventSink,
nsIProgressEventSink), but that would make this a tree-wide change, which I
didn't feel like currently

Comment 6

[Darin Fisher]

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

>Index: netwerk/base/src/nsInputStreamPump.cpp

>+                        printf("*** offsets: after: %lld, before: %lld\n", offsetAfter, offsetBefore);

you didn't mean to keep this printf in, right?


>Index: netwerk/base/src/nsInputStreamPump.h

>-    PRUint32                      mStreamOffset;
>-    PRUint32                      mStreamLength;
>+    nsInt64                       mStreamOffset;
>+    nsInt64                       mStreamLength;

slightly concerning to me that this also means a change from
unsigned to signed arithmetic.	please be sure to double-check
that we aren't making any assumptions anywhere about these being
unsigned.


>Index: netwerk/base/src/nsResumableEntityID.cpp

>+    mSize(LL_INIT(0xffffffff, 0xffffffff)) {

LL_MAXUINT?

>+    if (LL_EQ(mSize, LL_INIT(0xffffffff, 0xffffffff)))

same here,

>+        size = LL_INIT(0xffffffff, 0xffffffff);

and here.


>Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp

>+      mStartPos(LL_INIT(0xffffffff, 0xffffffff))

are you trying to avoid the overhead of calling LL_MaxUint?
i don't like hardcoding magic numbers like this.  what if
you were to accidentally type only 7 f's? ;-)


>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp

>+    mFileSize          = LL_INIT(0xffffffff, 0xffffffff);

more occurances of this guy.


>+        PR_sscanf(mResponseMsg.get() + 4, "%llu", &mFileSize);
>+        // XXX this so sucks
>+        PRUint32 size32;
>+        LL_L2UI(size32, mFileSize);
>+        if (NS_FAILED(mChannel->SetContentLength(size32))) return FTP_ERROR;

so create a new internal API for FTP... or is it the case that the
nsFTPChannel's mContentLength is only ever used with GetContentLength?


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+    PRUint64 size = LL_INIT(0xffffffff, 0xffffffff);

ding!


>Index: netwerk/protocol/http/src/nsHttpResponseHead.cpp

>+        mHeaders.SetHeader(nsHttp::Content_Length, nsPrintfCString("%lld", len));

hmm... the default buffer size for nsPrintfCString is not large
enough for 64-bit integers.  perhaps we should increase the default
buffer size from 15 to 20 (not including trailing null byte). 


ok, please come up with a better solution for LL_MAXUINT if you don't
want to simply use it.

it also seems like nsUint64 would be handy in some cases, no?


overall this patch looks really good... thanks for doing this biesi!!

Comment 7

[Darin Fisher]

i also saw this code passing 64-bit values to nsITransportEventSink.  i guess
those get silently downcast to 32-bit values.  at some point we need to rev all
of our progress APIs to work with 64-bit values.  my gut feeling is that we'll
need to preserve existing progress APIs (even though they are not frozen) just
because of the fact that so many extensions and embedders already use them. 
nsIWebProgressListener is effectively frozen whether we like it or not! :-(

Comment 8

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

(In reply to comment #6)
> you didn't mean to keep this printf in, right?

oops, indeed not

> so create a new internal API for FTP... or is it the case that the
> nsFTPChannel's mContentLength is only ever used with GetContentLength?

yeah, it is. 

> ok, please come up with a better solution for LL_MAXUINT if you don't
> want to simply use it.

I'll use LL_MaxUint() - I was just not aware it existed. bug 245923 filed.

> it also seems like nsUint64 would be handy in some cases, no?

indeed. I was kinda trying to only touch netwerk/ in this patch. Bug 245927
filed; adding dependency for now...

(In reply to comment #7)
> i also saw this code passing 64-bit values to nsITransportEventSink.  i guess
> those get silently downcast to 32-bit values.

yes, nsInt64 has an operator PRUint32()

>  at some point we need to rev all
> of our progress APIs to work with 64-bit values.  my gut feeling is that we'll
> need to preserve existing progress APIs (even though they are not frozen) just
> because of the fact that so many extensions and embedders already use them. 
> nsIWebProgressListener is effectively frozen whether we like it or not! :-(

that's unfortunate :( there's always the possibility of nsIWebProgressListener2
of course...

Comment 9

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

Attachment: patch v2

now using LL_MaxUint, and a few other changes

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 150552 [details] [diff] [review]
patch v2

Looks good.

Comment 11

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

this was checked in 2004-06-16 12:51

Comment 12

[Frank Wein [:mcsmurf]]

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

Comment 13

[Ryan Polk (Quark)]

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

Comment 14

[José Jeria]

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