264599 - unfrozen necko interfaces should use 64-bit integers where appropriate

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

content lengths, offsets, etc on nonfrozen interfaces should use 64bit integers,
to support files > 2 gb / > 4 gb.

Comment 1

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

Attachment: patch v0

Comment 2

[Darin Fisher]

so, i'm a little bit worried about changing these interfaces.  maybe it would be
a good idea to post a question to n.p.m.embedding (cross post on .netlib,
.xpcom, and maybe .seamonkey) to see if anyone is going to be broken by these
changes.

on one hand, i agree that better now than later... but, we could also just
invent a whole new 64-bit API tree.

Comment 3

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

so the problem with making nsIProgressEventSink2 is that then each channel would
have to have compat code w/ nsIProgressEventSink... some helper function could
improve that, maybe a wrapper for the notificationcallbacks that autowraps, but
still...

on the other hand, I need a solution for nsIAuthPrompt anyway... ok...

Maybe best would be to do the backwards compat stuff in the helper functions
from bug 261083.

Comment 4

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

news://news.mozilla.org/<cldj2d$[e-mail]>

Comment 5

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

ok, for the next version of the patch, I'll keep nsIInputStreamPump as-is and
add nsIInputStreamPump2. since it is basically just an Init function, this is
easy to do, and doesn't break current users of it.

Comment 6

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

Attachment: patch v1

updated to trunk. using nsIInputStreamPump2.

sorry for the delay, it was caused by me not realizing that the problems my
build had with gmail were due to a --disable-extensions configure argument
instead of this patch :( I still hope to get this into 1.8a6....

Comment 7

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

Comment on attachment 169321 [details] [diff] [review]
patch v1

still not giving up hope on  a alpha6 checkin; requesting sr now in hopes of of
getting it in time...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oy.  I'll try for 1.8a6, but no guarantees.... my review backlog is nasty.  :(

Comment 9

[Brendan Eich [:brendan]]

biesi: don't sweat it, I can help get this in after the freeze next week; also,
if I can free some time, maybe I can relieve bz of sr duty here?

/be

Comment 10

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

brendan: thanks; feel free to do the sr instead of bz (assuming he doesn't mind)

Comment 11

[Darin Fisher]

I spoke with biesi about this patch over IRC.  Given that no one responded to
his query to the newsgroups, I think it should be fine to change
nsIInputStreamPump instead of introducing nsIInputStreamPump2.  After this patch
lands, I think we should consider freezing nsIInputStreamPump and the
corresponding necko component that implements the interface.

Comment 12

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

Comment on attachment 169321 [details] [diff] [review]
patch v1

patch with that change coming up

Comment 13

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

Attachment: patch v2

Comment 14

[Darin Fisher]

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

>Index: netwerk/base/public/nsIStreamLoader.idl

> /**
>  * Asynchronously loads a channel into a memory buffer.
>+ *
>+ * XXX define behaviour for sizes >4 GB
>  */
> [scriptable, uuid(31d37360-8e5a-11d3-93ad-00104ba0fd40)]
> interface nsIStreamLoader : nsISupports

This interface is to be avoided when the data could be large :-)


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

>+        if (!!mOffset) {
...
>+            if (mOffset != LL_MAXUINT) {

Hmm... it almost seems like it'd be nice if nsInt64 had methods to
test for zero, max, and min.


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

>+    if (!mSuppliedEntityID.IsEmpty() || (mStartPos != LL_MAXUINT && mStartPos != nsUint64(0))) {

hmm... what's the best way to compare a 64 bit value against zero?  LL_IS_ZERO?
nsUint64::operator!()?


>+    restString.AppendInt(PRInt64(PRUint64(mStartPos)), 10);

so much casting /sigh/


r=darin

Comment 15

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

> hmm... what's the best way to compare a 64 bit value against zero?

I'd have added an |operator bool()| to nsInt64/nsUint64; but I am unsure if
using bool is allowed in mozilla code these days... |operator PRBool()| does not
work - I get told that such an operator already exists. and just testing if
(some_nsint64) is ambiguous as-is.

Comment 16

[Darin Fisher]

> I'd have added an |operator bool()| to nsInt64/nsUint64; but I am unsure if
> using bool is allowed in mozilla code these days... |operator PRBool()| does not
> work - I get told that such an operator already exists. and just testing if
> (some_nsint64) is ambiguous as-is.

What about adding isZero, isMin, and isMax methods to nsTInt64?

Comment 17

[Darin Fisher]

> What about adding isZero, isMin, and isMax methods to nsTInt64?

doesn't have to hold up this patch of course ;)

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

>Index: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp

>+    // XXX this truncates to 32bit

It'd be good to have a standard searchable string for comments like this one...
perhaps "64-bit" as in the wycywig channel comment?  Or something more
distinctive?  This applies to all comments along these lines in this patch.  If
we have something we can search on, we can probably add this to the "good first
bug" list....

>+    if (mTotalCurrentProgress == nsInt64(0) && mTotalMaxProgress == nsInt64(0))

It'd really make sense to be able to compare nsU?Int64 with native 32-bit ints.
 File a followup bug on that, maybe?  It should be possible to implement the
!=, ==, >, <, <=, and >= operators for 32-bit ints on the nsInt64 classes.... 
In any case, wouldn't comparing to LL_ZERO be cleaner here?

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp

Changing this interface per your comments should be OK from the POV of JS
callers.  File a bug on this, please?  Make sure darin, jst, and doron are
cced?

>Index: modules/libjar/nsJARChannel.cpp
>+        mProgressSink->OnProgress(this, nsnull, nsUint64(offset + count),
>+	                                nsUint64(mContentLength));

Weird indent.

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

>@@ -268,13 +268,13 @@ nsInputStreamPump::AsyncRead(nsIStreamLi
>-        rv = sts->CreateInputTransport(mStream, mStreamOffset, mStreamLength,
>+        rv = sts->CreateInputTransport(mStream, PRUint64(mStreamOffset), PRUint64(mStreamLength),
>                                        mCloseWhenDone, 

This looks very weird as a cast to PRInt64... ;)  At least comment here that
that's what you're doing?

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

> nsInputStreamTransport::Read(char *buf, PRUint32 count, PRUint32 
>+        if (!!mOffset) {

This would be covered by implementing the == operator for ints that I mention
above.

>+                    seekable->Seek(nsISeekableStream::NS_SEEK_SET, PRUint64(mOffset));

Again, please document that this is just a weird way to cast to a PRInt64....

> nsOutputStreamTransport::Write(const char *buf, PRUint32 count, PRUint32 *result)

Same comments apply.

>Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp
>-    NS_ASSERTION(mStartPos == PRUint32(-1) || mStartPos == 0,
>+    NS_ASSERTION(mStartPos == LL_MAXUINT || mStartPos == nsUint64(0),
>                  "Non-initial start position given to directory request");
>-    if (!mSuppliedEntityID.IsEmpty() || (mStartPos != PRUint32(-1) && mStartPos != 0)) {
>+    if (!mSuppliedEntityID.IsEmpty() || (mStartPos != LL_MAXUINT && mStartPos != nsUint64(0))) {

This could use the == and != operators, again.	Or LL_ZERO?

>+    restString.AppendInt(PRInt64(PRUint64(mStartPos)), 10);

Why do you need  the second cast (to PRInt64)?	Because otherwise the call is
ambiguous with native 64-bit types?  If so, comment on that here?

>Index: uriloader/base/nsDocLoader.cpp
>+      if ((oldMax < nsInt64(0)) && (mMaxSelfProgress < nsInt64(0))) {

Again, maybe LL_ZERO, or just leave it for the operator followup...

>+      if ((oldMax == nsInt64(0)) && (info->mCurrentProgress == nsInt64(0))) {

And here?

>+    if (individualProgress < nsInt64(0)) // if one of the elements doesn't know it's size

And here.

>+  if (mMaxSelfProgress >= nsInt64(0) && newMaxTotal >= nsInt64(0))

And here.

>+    if (!info->mUploading && (nsInt64(0) == info->mCurrentProgress) && (nsInt64(0) == info->mMaxProgress)) {

Same here.

>Index: webshell/tests/viewer/nsBrowserWindow.cpp
>+    if (nsUint64(0) != aProgressMax) {

Same here.

sr=bzbarsky with the nits picked and followup bugs filed.

Comment 19

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

*sigh*, ns*Int64 sucks. it is ambiguous to create an nsUint64 from an PRInt64.
LL_ZERO is an PRInt64. hence, much of what you suggested would not work.

Comment 20

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

hm, actually, maybe that's not the problem. instead, the compiler considers
converting the nsUint64 to various types (PRFloat64, PRInt32, etc) and use the
build-in operator==.

Comment 21

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

(and I used some of the nsUint64(0) to have an nsUint64 object, for compilers
where PRInt64 is a struct... although maybe we need not worry about those)

Comment 22

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

I'll make it so that a regex search for XXX.*64-bit will find all these
comments. Do note that many of those are not good first bugs, since they are
forced by the current nsIStreamListener API (people passing aOffset + aCount as
progress).

filed Bug 277662 about XMLHTTPRequest (nsIDOMLSProgressEvent), and Bug 277663
about the operators. I am not sure whether isMax() is actually needed - ==
LL_MAXUINT does work on an nsUint64, and matches == PR_UINT32_MAX for 32-bit
integers. but if you want I can file such a bug too. isZero() can be covered in
Bug 277663, I think.

attaching updated patch in a second.

Comment 23

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

Attachment: patch v3

I would like to land this in 1.8alpha6 - it changes necko APIs in an
incompatible way. the risk should be rather low, since this mainly changes some
data types to 64-bit integers.

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 170742 [details] [diff] [review]
patch v3

a=asa for checkin to 1.8a6

Comment 25

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

thanks for the reviews and approval!

checked in.