directory listings show wrong sizes for files > 4 GB

RESOLVED FIXED in mozilla1.7final

Status

()

Core
Networking
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

({fixed1.7})

Trunk
mozilla1.7final
fixed1.7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 obsolete attachments)

this bug is distributed over all parts of necko that handle file listings...

(note: if you watn to test the patch I'll attach, you need the patch for bug 
240192 as well)
Created attachment 145887 [details] [diff] [review]
patch
Assignee: darin → cbiesinger
Status: NEW → ASSIGNED
Attachment #145887 - Flags: review?(darin)
Target Milestone: --- → mozilla1.7final

Comment 2

14 years ago
Comment on attachment 145887 [details] [diff] [review]
patch

>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp

>+    if (size != LL_INIT(0, -1) &&

does this need to use LL_NE?


>Index: xpfe/components/directory/nsDirectoryViewer.cpp

>+      if (size != LL_INIT(0, -1)) {

same question about LL_NE.


>+        // XXX RDF should support 64 bit integers (bug 240160)

XXX comment with a bug link, very nice :-)


r=darin
Attachment #145887 - Flags: review?(darin) → review+
Comment on attachment 145887 [details] [diff] [review]
patch

darin: oh, indeed, I'll make that change
Attachment #145887 - Flags: superreview?(bryner)
Attachment #145887 - Flags: superreview?(bryner) → superreview+
checked in on trunk with changes made
Comment on attachment 145887 [details] [diff] [review]
patch

it would be nice to get this into 1.7 - this just changes the type of some
variables to PRInt64 instead of PRUint32, so it's pretty low risk.
Attachment #145887 - Flags: approval1.7?

Comment 6

14 years ago
Comment on attachment 145887 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #145887 - Flags: approval1.7? → approval1.7+
checked in on 1.7 branch
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed1.7
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED

Comment 8

14 years ago
Have you checked all interactions?
Your patch seems to break the gopher support.

Somehow (-1) becomes just FF FF FF FF (32 bit) 
instead of proper (-1) or FF FF FF FF FF FF FF FF (64 bit).

Instead of unknown value the interface is getting information about files 
of size 4GB each.

Comment 9

14 years ago
jan.ruzicka@comcast.net: oh come on, if you're going to complain, please provide
steps to reproduce, including a nice url.

here's a testcase:
gopher://seanm.ca/

Comment 10

14 years ago
The actual problem is in the initialization of the instance variables.

http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsDirIndex.cpp#47

The mSize was changed to PRInt64 but the initialization is for PRUint32 line #48.

 47 nsDirIndex::nsDirIndex() : mType(TYPE_UNKNOWN),
 48                            mSize((PRUint32)(-1)),
 49                            mLastModified(-1) 

The initialization to an unknown number is used in the gopher presentation.
Blocks: 118438

Comment 11

14 years ago
(In reply to comment #9)
> jan.ruzicka@comcast.net: oh come on, if you're going to complain, please provide
> steps to reproduce, including a nice url.
> 
> here's a testcase:
> gopher://seanm.ca/

OK then it can be:
gopher://gopher.floodgap.com/

I was too bussy looking for the actual cause. look at comment #10.

Comment 12

14 years ago
Created attachment 147008 [details] [diff] [review]
this compiles

i blame the typecast. if not for that, this error would have been spotted at
compile time.
Attachment #145887 - Attachment is obsolete: true

Comment 13

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

does this fix the problem?  I don't have a xpcshell testworld for this yet.
Attachment #147008 - Flags: review?(jan.ruzicka)

Comment 14

14 years ago
Re-opening as per Jan Ruckiza's comments.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
sorry for breaking this :(
timeless's patch does look correct, r=me

Comment 16

14 years ago
Looks correct, but I have some questions.
What is with the LL_INIT(0, -1), what advanages does it have against plain -1 ?

Should all PRInt64 variables be initialized with this macro?

(mLastModified comes to mind) 
Comment on attachment 147008 [details] [diff] [review]
this compiles

Um, isn't that the same thing? Don't you need LL_INIT(-1,-1) ?
Do you need to change other places, too, in order to use the LL_ macros? (Does
any OS mozilla supports need the LL_ macros, anyway?)

Comment 19

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

> What is with the LL_INIT(0, -1),

See http://www.mozilla.org/projects/nspr/reference/html/prlong.html

> what advanages does it have against plain -1 ?

in short, it's portable to platforms where the platform or compiler do not
support 64bit types.

> Should all PRInt64 variables be initialized with this macro?

For initialization, one of LL_MaxInt, LL_MinInt, LL_Zero, LL_INIT, or another
preexisting PRInt64/PRUint64. Otherwise one can assign/initialize by using an
LL_ Arithmetic op.

> (mLastModified comes to mind) 

Please file bugs for any of them, that's really beyond the scope of this bug.

> Um, isn't that the same thing?

No. The reason the preexisting code didn't work is that it had a typecast:
> (PRUint32)(-1)

Which made it an unsigned integer which easily fit into 64 bits.

> Don't you need LL_INIT(-1,-1) ?

No.

> Do you need to change other places, too, in order to use the LL_ macros?

Biesi changed the other places.

jan: could you test the patch and confirm that it works? if it works, please
load http://bugzilla.mozilla.org/attachment.cgi?id=147008&action=edit and
change the ? by your name to a +.
Attachment #147008 - Flags: superreview?(darin)

Comment 20

14 years ago
(In reply to comment #19)
> (From update of attachment 147008 [details] [diff] [review])
> > What is with the LL_INIT(0, -1),
> 
> See http://www.mozilla.org/projects/nspr/reference/html/prlong.html
> 
> > what advanages does it have against plain -1 ?
> 
> in short, it's portable to platforms where the platform or compiler do not
> support 64bit types.
> 
 (only platforms having long long.)
[...]
> 
> > Don't you need LL_INIT(-1,-1) ?
> 
> No.
> 
> > Do you need to change other places, too, in order to use the LL_ macros?
> 
> Biesi changed the other places.
> 
> jan: could you test the patch and confirm that it works? if it works, please
> load http://bugzilla.mozilla.org/attachment.cgi?id=147008&action=edit and
> change the ? by your name to a +.
> 

Thank you for the patch and detailed explanation.


When I try to go to edit attachment #147008 [details] [diff] [review] the bugzilla replies that I don't
have a permition to edit it.



As for the LL_INIT - it is defined pretty strange:

 #define LL_INIT(hi, lo)  ((hi ## LL << 32) + lo ## LL) 

this means pretty hard math to get the parts right, because of the sign extension.
LL_INIT (-1,-1) would actually lead exactly oposite result in case of
working 64bit arithmetic. 
It would be:
    FF FF FF FF 00 00 00 00 
+   FF FF FF FF FF FF FF FF
---------------------------
= 1 00 00 00 00 FF FF FF FF

How do you initialize  -3000000000 ?

Comment 21

14 years ago
>  (only platforms having long long.)
taking that back - after carefully reading the file.

Comment 22

14 years ago
looks good to me r=jan.ruzicka

Updated

14 years ago
Blocks: 241696

Comment 23

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

recording r=jan,biesi

sorry about that, i should have known better. i'm going to see about getting
that fixed.
Attachment #147008 - Flags: review?(jan.ruzicka) → review+
> this means pretty hard math to get the parts right

indeed. initially (for my first patch here), I had used LL_INIT(-1, -1), but
that did not give the desired result...

I find it somewhat unlikely that this will work when there is no native int64
type. I blame NSPR.

Updated

14 years ago
Attachment #147008 - Flags: superreview?(darin) → superreview+
*** Bug 241928 has been marked as a duplicate of this bug. ***

Comment 26

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

mozilla/netwerk/streamconv/converters/nsDirIndex.cpp	1.8
Attachment #147008 - Attachment is obsolete: true

Comment 27

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

this change is needed for the branch. it's small, reviewed and safe.
Attachment #147008 - Flags: approval1.7?

Comment 28

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

a=chofmann for 1.7
Attachment #147008 - Flags: approval1.7? → approval1.7+

Comment 29

14 years ago
Comment on attachment 147008 [details] [diff] [review]
this compiles

mozilla/netwerk/streamconv/converters/nsDirIndex.cpp 	1.5.20.2
all fixed.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED

Comment 31

14 years ago
Can someone find the depends bugs in FTP and put them here?
I don't know how to verify this, I don't have a 4GB file handy :)
(In reply to comment #31)
> Can someone find the depends bugs in FTP and put them here?

Hm... can't find any. With this patch, filesizes on FTP servers seem to display
fine though for files > 4 GB.

> I don't know how to verify this, I don't have a 4GB file handy :)

apply bug 240192's patch if on linux, and:
dd if=/dev/zero of=large_file seek=4G bs=1 count=1

Comment 33

14 years ago
*** Bug 244496 has been marked as a duplicate of this bug. ***

Updated

14 years ago
You need to log in before you can comment on or make changes to this bug.