Last Comment Bug 215450 - uploading files that are larger the 2GB fails
: uploading files that are larger the 2GB fails
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal with 13 votes (vote)
: mozilla17
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
: 383446 607313 660159 678648 764762 827526 (view as bug list)
Depends on: 737451 184452 720627 720957 782255
Blocks: 782102
  Show dependency treegraph
 
Reported: 2003-08-07 14:04 PDT by boris tabenkin
Modified: 2016-02-09 09:39 PST (History)
46 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v0.5 (17.70 KB, patch)
2010-01-05 03:25 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
patch v1 (21.35 KB, patch)
2010-01-06 00:02 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
cbiesinger: review-
Details | Diff | Review
patch v2 (20.08 KB, patch)
2010-02-01 21:00 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
patch v2.1 (20.09 KB, patch)
2010-02-01 21:20 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
fix v3 (69.77 KB, patch)
2011-06-14 23:41 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
fix v4 (86.56 KB, patch)
2011-08-22 03:16 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
fix v5 (87.78 KB, patch)
2011-09-28 04:27 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review-
Details | Diff | Review
Part 1. IDL changes (3.63 KB, patch)
2012-01-26 00:11 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
benjamin: review+
Details | Diff | Review
Part4. libpref part (1.79 KB, patch)
2012-01-26 01:38 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 5. xpconnect part (1.44 KB, patch)
2012-01-26 02:00 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
Part 6. libjar part (2.99 KB, patch)
2012-01-26 02:24 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
Part 7. rdf part (1.34 KB, patch)
2012-01-26 02:36 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
part 8. parser part (1.59 KB, patch)
2012-01-26 02:56 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
part 9. gfx part (1.04 KB, patch)
2012-01-26 03:01 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 10. image part (3.97 KB, patch)
2012-01-26 03:13 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 11. dom part (1.46 KB, patch)
2012-01-26 03:25 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 12. widget part (1.76 KB, patch)
2012-01-26 03:51 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 13. content part (8.10 KB, patch)
2012-01-26 04:08 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 2. xpcom part (9.06 KB, patch)
2012-01-26 17:36 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
Part 14. toolkit part (4.03 KB, patch)
2012-01-26 17:37 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 15. security part (2.13 KB, patch)
2012-01-26 17:39 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review-
Details | Diff | Review
Part 17. startupcache (1.13 KB, patch)
2012-01-26 20:55 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 16. extension part (4.88 KB, patch)
2012-01-26 20:56 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 8. parser part (1.60 KB, patch)
2012-01-26 22:28 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 5. xpconnect part (1.47 KB, patch)
2012-01-26 22:29 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 3. network part (42.77 KB, patch)
2012-01-26 22:33 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 6. libjar part (2.99 KB, patch)
2012-01-27 00:11 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 18. layout part (1.17 KB, patch)
2012-02-12 22:38 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 19. for comm-central (25.64 KB, patch)
2012-03-20 07:31 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review-
Details | Diff | Review
Part 2. xpcom part (9.16 KB, patch)
2012-03-20 07:33 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
Part 15. security part (3.60 KB, patch)
2012-03-20 08:47 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
Part 2. xpcom part v4 (10.83 KB, patch)
2012-03-23 02:58 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
no flags Details | Diff | Review
Part 20. dom/devicestorage part (3.67 KB, patch)
2012-06-18 03:14 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Part 2. xpcom part v4.1 (8.94 KB, patch)
2012-06-18 03:16 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review-
Details | Diff | Review
Part 15. security part v2 (3.60 KB, patch)
2012-06-18 03:22 PDT, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
honzab.moz: review+
Details | Diff | Review
Merged WIP 1 (103.17 KB, patch)
2012-08-06 06:57 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
v1 (102.13 KB, patch)
2012-08-10 09:15 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Review
v1 with my review comments addressed (102.25 KB, patch)
2012-08-10 10:20 PDT, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Review
Check for overflow in NS_ConsumeStream (981 bytes, patch)
2012-08-10 10:27 PDT, Honza Bambas (:mayhemer)
benjamin: review+
Details | Diff | Review
interdiff: my "r+ with comments" addressed (7.01 KB, patch)
2012-08-10 10:32 PDT, Honza Bambas (:mayhemer)
jonas: review+
joe: review+
Details | Diff | Review
comm-central v2 (26.99 KB, patch)
2012-08-10 10:58 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Review
comm-central v3 (17.91 KB, patch)
2012-08-13 06:09 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review
comm-central v3a (17.85 KB, patch)
2012-08-13 07:20 PDT, Mark Banner (:standard8)
neil: review+
Details | Diff | Review

Description boris tabenkin 2003-08-07 14:04:16 PDT
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529

When trying to upload a file that is larger then 2GB the browser does nothing. 

Reproducible: Didn't try

Steps to Reproduce:
1. build a html form thjat has a <input type="file"> tag
2. submit the form
3.
Comment 1 Bill Mason 2003-08-07 15:07:27 PDT

*** This bug has been marked as a duplicate of 215091 ***
Comment 2 Frank Wein [:mcsmurf] 2003-08-08 11:02:23 PDT
.
Comment 3 Frank Wein [:mcsmurf] 2003-08-08 11:03:46 PDT

*** This bug has been marked as a duplicate of 184452 ***
Comment 4 benc 2003-09-04 08:06:46 PDT
Best expressed as a dependency, not a duplicate. Someone will have to test each
entrypoint that is affected by the possible fix of 184452.
Comment 5 Manoj 2003-11-27 01:58:57 PST

*** This bug has been marked as a duplicate of 184452 ***
Comment 6 Frank Wein [:mcsmurf] 2003-11-27 04:06:16 PST
please let this bug open and read comment 4
Comment 7 benc 2004-09-23 07:26:03 PDT
Does the submit form use http: URL? If so, please move this to HTTP.
Comment 8 Darin Fisher 2005-01-13 07:40:50 PST
I wonder if this is still a problem in recent builds given all of biesi's work
on supporting large files.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2005-01-13 14:07:19 PST
I didn't modify the frozen nsIUploadChannel, neither the frozen nsIInputStream,
which means there is no way for the HTTP code to know it has to upload more than
2 GB :-/ so, I would be surprised if this is fixed now
Comment 10 Darin Fisher 2005-01-13 14:45:36 PST
true, true!  if we supported uploads without an explicit content-length header,
then we'd be able to support >2GB uploads with this API, but otherwise you are
correct... we need a new API.  maybe nsIUploadChannel2 is all that is needed, or
perhaps this could be another for that properties API I keep talking about ;-)
Comment 11 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2009-08-12 18:45:11 PDT
*** Bug 383446 has been marked as a duplicate of this bug. ***
Comment 12 Nissamudeen Rawther 2009-11-15 22:38:37 PST
Any update on this issue? 
With the current usage scenario, 2 GB limit is really small. Google Chrome don't have a limitation of 2GB.
Comment 13 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2010-01-05 03:25:34 PST
Created attachment 420061 [details] [diff] [review]
WIP v0.5
Comment 14 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2010-01-06 00:02:06 PST
Created attachment 420283 [details] [diff] [review]
patch v1
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2010-01-21 14:03:26 PST
Comment on attachment 420283 [details] [diff] [review]
patch v1

Thanks for the patch! A few comments:

+++ b/netwerk/base/src/nsBufferedStreams.h
+                NS_ASSERTION(avail < PR_UINT32_MAX,
+                             "Source() should have nsIInputStream2 to support 64-bit stream");

This assertion is incorrect. This assertion fires when the stream has exactly 4 GB of data available. It doesn't seem like that should be disallowed.

+++ b/netwerk/base/src/nsFileStreams.cpp
+    // PR_Available with files over 4GB returns an error, so we have to
+    // use the 64-bit version.  Available64() uses 64-bit version.

I don't think that comment makes sense anymore. Just remove it?

+++ b/netwerk/base/src/nsMIMEInputStream.cpp
+                    NS_ASSERTION(cl32 < PR_UINT32_MAX,
+                                 "mData should have nsIInputStream2 to support 64-bit stream");

as above

+    nsCOMPtr<nsIInputStream2> stream2 = do_QueryInterface(mStream);

mStream is an nsIMultiplexInputStream, which inherits nsIInputStream2. Consequently, this entire method can just be "return mStream->Available64(_retval);"

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
+                    // If return value is PR_UINT32_MAX, stream has over 4GB.

That conclusion is not valid. The value can also mean that it has exactly 4 GB.

I think it's OK to leave the code as you've written it, but please change the comment to say "the stream probably has more than 4 GB".

+            // If return value is PR_UINT32_MAX, stream has over 4GB.

same

+++ b/netwerk/protocol/http/src/nsHttpChannel.h
-#include "nsIInputStream.h"
+#include "nsIInputStream2.h"

Why this change? nsIInputStream2 is not used in the header, only nsIInputStream is.

+++ b/xpcom/io/nsIInputStream2.idl
+    long long available64();

Why is this signed? I think for consistency with nsIInputStream, and because negative numbers make no sense, it would be better to make this unsigned.

+++ b/xpcom/io/nsMultiplexInputStream.cpp
+                NS_ASSERTION(avail32 < PR_UINT32_MAX, "mStreams[i] should have nsIInputStream2 to support 64-bit stream");

again, I'd prefer you to remove this assertion
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2010-01-21 14:17:12 PST
I'm not entirely happy with this patch, since a 64-bit available shouldn't be necessary, but it seems like it may be the only way to get the total content length of the post data.
Comment 17 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2010-01-25 19:05:32 PST
Thank you for reviewing!

(In reply to comment #15)

> +++ b/xpcom/io/nsIInputStream2.idl
> +    long long available64();
> 
> Why is this signed? I think for consistency with nsIInputStream, and because
> negative numbers make no sense, it would be better to make this unsigned.

Because 64-bit interface of NSPR uses signed long long.  I will modify this interface to unsigned.
Comment 18 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2010-02-01 21:00:59 PST
Created attachment 424724 [details] [diff] [review]
patch v2
Comment 19 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2010-02-01 21:20:19 PST
Created attachment 424728 [details] [diff] [review]
patch v2.1
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2010-07-11 15:00:09 PDT
Makoto, now that we have gone through the Great XPCOM Interface Unfreezing you can modify nsIInputStream directly.
Comment 21 Aigars Mahinovs 2010-11-15 00:27:54 PST
Could we promote this bug up a bit somehow? This is 7 years old, relatively easy to fix bug (or so it looks from the patches posted) that will give Firefox another advantage in browser space (currently only Chrome is able to upload big files).
Comment 22 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2010-11-15 00:40:22 PST
(In reply to comment #21)
> Could we promote this bug up a bit somehow? This is 7 years old, relatively
> easy to fix bug (or so it looks from the patches posted) that will give Firefox
> another advantage in browser space (currently only Chrome is able to upload big
> files).

This is needed API change (Now API is freezed).  After Firefox 4 / Gecko 2.0 is released, I will raise the priority.
Comment 23 Aigars Mahinovs 2010-11-15 03:00:49 PST
I am glad you are on it!

But why not have a stop-gap solution in Firefox 4 and then have another bug to fix it properly in the new API? (Like actually complete the upload even if we don't send the size header or show an incorrect progress indicator for files over 2 or 4 Gb)
Comment 24 Igor 2011-05-06 04:26:14 PDT
2Comment21
Now,IE9 (x64 at least) is able to upload >2Gb too.
Comment 25 Alexander Bogdanov 2011-05-06 04:35:34 PDT
@Igor: 
x86 also can
Comment 26 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2011-06-14 23:41:48 PDT
Created attachment 539442 [details] [diff] [review]
fix v3

I should separate this to multiple...  Now testing.
Comment 27 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-16 14:10:22 PDT
*** Bug 678648 has been marked as a duplicate of this bug. ***
Comment 28 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-16 14:11:57 PDT
For what it's worth, we no longer have any frozen interfaces, so we should change all of the relevant ones to use 64bit instead of 32bit.
Comment 29 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-16 19:47:27 PDT
Josh, could we get someone on the network team to pick this up?
Comment 30 Josh Aas 2011-08-16 20:42:16 PDT
Maybe Jason can take a look?
Comment 31 Jason Duell [:jduell] (needinfo? me) 2011-08-17 14:02:08 PDT
Comment on attachment 539442 [details] [diff] [review]
fix v3

Review of attachment 539442 [details] [diff] [review]:
-----------------------------------------------------------------

One thing we'll need to add to this patch is e10s support--I'm guessing there's some work to do there.  See PHttpChannel.ipdl and the HttpChannel{Child,Parent} classes (and the FTP versions too).  There may be 32 bit data we're sending between parent/child that needs to be 64 bit.

Makoto--are you still interested in working on this bug?  I'm happy to let you do the fixes and I can advise and review patches.
Comment 32 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-18 00:57:11 PDT
I think Steve was looking at this bug earlier today.
Comment 33 Steve Workman [:sworkman] (please use needinfo) 2011-08-18 11:32:40 PDT
Yup, been looking through this at Josh's request.

Quick clarification on Jonas's comment #28:
The diff currently changes the interface and classes implementing it, but there are several functions calling available() that don't support 64bits.  I.e. they call other functions which only support 32 bits.  Makato's diff deals with these by checking for > 32 bits and casting or returning (e.g. nsFrameScriptExecutor::LoadFrameScriptInternal())

Assuming everyone is ok with this, and a rewrite of those other functions isn't required?

Also, ref comment #15, we can change some of the assertions to (size <= PR_UINT32_MAX), right?
Comment 34 Jason Duell [:jduell] (needinfo? me) 2011-08-18 15:29:04 PDT
> there are several functions calling available() that don't support 64bits.
> I.e. they call other functions which only support 32 bits.  Makato's diff
> deals with these by checking for > 32 bits and casting or returning (e.g.
> nsFrameScriptExecutor::LoadFrameScriptInternal())

That's *probably* fine. But if you could look at all these code sites and try to get a sense of what the repercussions for failing/skipping on > PR_UINT32_MAX is, that would be great.  

In other words, time for your first review! :)

Turns out I'm wrong about needing e10s work--doesn't look like we need to do anything for it.

> Also, ref comment #15, we can change some of the assertions to 
> (size <= PR_UINT32_MAX), right?

Seems like these have been taken out of the latest version?  It's OK to have them, as long as they make sense (i.e they're not testing against an unsigned 32-bit 'size', in which case they'd always be true).

Once you've fixed any bitrot in the patch, looked over it, and identified any places where things look problematic (or you just can't figure them out yourself), let me know, and I can do another +r.
Comment 35 Jason Duell [:jduell] (needinfo? me) 2011-08-18 15:29:50 PDT
Comment on attachment 539442 [details] [diff] [review]
fix v3

Since it's your first review, let's call it "feedback"
Comment 36 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2011-08-22 03:16:58 PDT
Created attachment 554809 [details] [diff] [review]
fix v4
Comment 37 Steve Workman [:sworkman] (please use needinfo) 2011-08-24 19:08:27 PDT
Comment on attachment 554809 [details] [diff] [review]
fix v4

Review of attachment 554809 [details] [diff] [review]:
-----------------------------------------------------------------

Please note:  I'm new to this code, so some of my feedback comments may need their own feedback :)

General Comments:

-- nsGIOInputStream::Available(32) needs to be updated to 64 bits as well.

-- I suggest that ::OnDataAvailable() should also be upgraded from 32 bits to 64 bits (in a future bug, not this one).

::: content/base/src/nsFrameMessageManager.cpp
@@ +768,5 @@
>    channel->Open(getter_AddRefs(input));
>    nsString dataString;
> +  PRUint64 avail = 0;
> +  if (input && NS_SUCCEEDED(input->Available(&avail)) && avail &&
> +      avail < PR_UINT32_MAX) {

Why '<' and not '<='?

::: content/base/src/nsSyncLoadService.cpp
@@ +386,5 @@
>      rv = aListener->OnStartRequest(aChannel, nsnull);
>      if (NS_SUCCEEDED(rv)) {
>          PRUint32 sourceOffset = 0;
>          while (1) {
> +            PRUint64 readCount = 0;

In subsequent call to aListener->OnDataAvailable, can you add an explicit PRUint32 cast to readCount.

Also, if (readCount > PR_UINT32_MAX) can you set it to PR_UINT32MAX?  This way, OnDataAvailable will only be told about 4GB maximum at a time.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +329,4 @@
>    rv = stream->Available(&count);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    return Base64EncodeInputStream(stream, aDataURL, count, aDataURL.Length());

Base64EncodeInputStream only takes a PRUint32 for count.  Can you do an NS_ENSURE_TRUE(count <= PR_UINT32_MAX,...)?  And explicitly cast count as PRUint32.

@@ +369,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  // no support 4GB or over
> +  NS_ENSURE_TRUE(imgSize64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Same as previous: why not '<='?

::: content/media/nsMediaStream.cpp
@@ +1057,3 @@
>    rv = mInput->Available(&size);
>    if (NS_SUCCEEDED(rv)) {
>      mSize = size;

'mSize' is a PRInt64 - can you add a check for (size <= PR_INT64_MAX) and do an explicit cast?

::: content/xul/document/src/nsXULPrototypeCache.cpp
@@ +705,5 @@
> +        if (NS_SUCCEEDED(rv)) {
> +            PRUint64 len64;
> +            rv = inputStream->Available(&len64);
> +            len = PRUint32(len64);
> +            if (NS_SUCCEEDED(rv) && len64 >= PR_UINT32_MAX) {

Last time I'll mention this ;) why do you not allow PR_UINT32_MAX?

::: dom/src/json/nsJSON.cpp
@@ +531,5 @@
>    nsresult status;
>    jsonChannel->GetStatus(&status);
>    PRUint32 offset = 0;
>    while (NS_SUCCEEDED(status)) {
> +    PRUint64 available;

Subsequent call to OnDataAvailable uses a PRUint32.  Can you do a MAX check and set it to PR_UINT32_MAX if it's more than that.  Since this one is in a while loop, it should be ok to just get PR_UINT32_MAX for every loop.

Also, please do an explicit cast in the function call for OnDataAvailable to PRUint32.

@@ +672,5 @@
>  nsJSONListener::OnDataAvailable(nsIRequest *aRequest, nsISupports *aContext,
>                                  nsIInputStream *aStream,
>                                  PRUint32 aOffset, PRUint32 aLength)
>  {
> +  PRUint64 contentLength;

Looks good.

::: gfx/thebes/gfxASurface.cpp
@@ +784,5 @@
>    CallQueryInterface(encoder.get(), getter_AddRefs(imgStream));
>    if (!imgStream)
>      return;
>  
> +  PRUint64 bufSize;

Subsequent call to imgStream->Read is looking for a PRUint32 for 'aCount', but is passed (bufSize-imgSize), which could technically be more than PR_UINT32_MAX.
Can you add a check for NS_ENSURE_TRUE((bufSize-imgSize) < PR_UINT32_MAX, ...) please?

::: layout/base/nsPresShell.cpp
@@ +8600,5 @@
>    NS_ENSURE_TRUE(file, NS_ERROR_FAILURE);
>    rv = file->InitWithPath(name);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  PRUint64 length;

Needs to have a check for PR_UINT32_MAX.  Subsequent call to NS_NewBufferedOutputStream only accepts PRUint32 for length.

::: modules/libjar/nsJARChannel.cpp
@@ +164,5 @@
>          return rv;
>      }
>  
>      // ask the JarStream for the content length
> +    rv = mJarStream->Available((PRUint64 *) &mContentLength);

Better to use a new PRUint64 to get the available length, check for PR_INT32_MAX (mContentLength is 32 bits signed), and then set mContentLength if (newVar <= PR_INT32_MAX).

::: modules/libpr0n/src/imgTools.cpp
@@ +105,5 @@
>        inStream = bufStream;
>    }
>  
>    // Figure out how much data we've been passed
> +  PRUint64 length;

inStream->ReadSegments takes a PRUint32.  Please check for MAX and return as appropriate.  Add explicit casting to the function call.

::: netwerk/base/src/nsInputStreamChannel.cpp
@@ +50,5 @@
>    // the stream can tell us.  If the stream is a pipe, then this will not work.
>  
>    PRInt64 len = ContentLength64();
>    if (len < 0) {
> +    PRUint64 avail;

SetContentLength takes a PRInt64.  There needs to be some checking here to make sure PR_INT64_MAX isn't passed.  NS_ENSURE_TRUE(avail <= PR_INT64_MAX, ..) should be fine.

::: netwerk/base/src/nsInputStreamPump.cpp
@@ +137,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
>    PeekData data(callback, closure);
> +  PRUint32 dummy32 = dummy;

Need to check for PR_UINT32_MAX here when setting dummy32.

@@ +536,5 @@
>                          mStatus = NS_ERROR_UNEXPECTED;
>                      }
>                  }
>                  else
> +                    mStreamOffset += avail32; // assume ODA behaved well

This all looks good :)

::: netwerk/base/src/nsMIMEInputStream.cpp
@@ +207,5 @@
>          if (mData) {
>              mData->Available(&cl);
>          }
>          mContentLength.AssignLiteral("Content-Length: ");
> +        mContentLength.AppendInt(cl);

AppendInt takes a PRInt32, so cl needs to be cast to PRInt32.  Also, it looks like this function should fail if (cl > PR_INT32_MAX).

::: netwerk/ipc/NeckoMessageUtils.h
@@ +214,3 @@
>  
>        aParam.mStream->Available(&bytes);
> +      if (bytes > 0 && bytes < PR_UINT32_MAX) {

I think this should fail more obviously if (bytes > PR_UINT32_MAX).  I suggest adding a separate if statement before this one...
if (bytes > PR_UINT32_MAX) {
  NS_WARNING("Cannot copy more than 4GB");
  return;
}
... and then continue with the next if statement.

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +429,4 @@
>        nsresult rv = mUploadStream->Available(&avail);
>        if (NS_FAILED(rv))
>          return rv;
>        mUploadLength = avail;

mUploadLength is a PRInt64.  Need to do NS_ENSURE_TRUE(avail <= PR_INT64_MAX, ...).

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +314,5 @@
>  }
>  
>  void
>  FTPChannelChild::DoOnDataAvailable(const nsCString& data,
> +                                   const PRUint64& offset,

In this function there is a call to OnDataAvailable which accepts a PRUint32 only.  I think there needs to be a cast/MAX check before OnDataAvailable is called.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +479,5 @@
>  
>    if (aContentLength < 0 && !aStreamHasHeaders) {
> +    PRUint64 streamLength = 0;
> +    if (NS_SUCCEEDED(aStream->Available(&streamLength))) {
> +      aContentLength = streamLength;

This assignment should be similar to the one in HttpBaseChannel::SetUploadStream()

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +343,5 @@
>  HttpChannelChild::OnTransportAndData(const nsresult& status,
>                                       const PRUint64 progress,
>                                       const PRUint64& progressMax,
>                                       const nsCString& data,
> +                                     const PRUint64& offset,

There is a call to OnDataAvailable in this function.  It takes a PRUint32 only.

::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp
@@ +126,5 @@
>  
>      nsAutoArrayPtr<char> buffer(new char[streamLen + 1]);
>      NS_ENSURE_TRUE(buffer, NS_ERROR_OUT_OF_MEMORY);
>  
>      rv = inStr->Read(buffer, streamLen, &read);

Read takes a PRUint32, so streamLen needs to be cast/checked for MAX.

::: netwerk/streamconv/test/Converters.cpp
@@ +96,5 @@
>  
> +    PRUint64 len;
> +    rv = convertedStream->Available(&len);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return mListener->OnDataAvailable(request, ctxt, convertedStream, sourceOffset, PRUint32(len));

len also needs to be checked for (len > PR_UINT32_MAX).

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +100,5 @@
>  
>          char *buffer = (char*)nsMemory::Alloc(len + 1);
>          if (!buffer) return NS_ERROR_OUT_OF_MEMORY;
>  
>          rv = inStr->Read(buffer, len, &read);

len needs to be cast to PRUint32, and checked for (len > PR_UINT32_MAX)

@@ +141,3 @@
>      dataStream->Available(&avail);
>  
>      return aListener->OnDataAvailable(request, nsnull, dataStream, 0, avail);

avail needs to be cast to PRUint32, and checked for (len > PR_UINT32_MAX)

::: parser/xml/src/nsSAXXMLReader.cpp
@@ +556,2 @@
>      if (NS_SUCCEEDED(rv))
>        offset += available;

offset and available should be capped at PR_UINT32_MAX before calling OnDataAvailable.  Otherwise, offset may increment more that what was processed in OnDataAvailable.

if (offset > PR_UINT32_MAX) offset = PR_UINT32_MAX;
same for available.
Then cast to PRUint32 in the function call.

::: rdf/base/src/nsRDFXMLDataSource.cpp
@@ +565,5 @@
>  
>          if (avail == 0)
>              break; // eof
>  
>          rv = aConsumer->OnDataAvailable(channel, nsnull, bufStream, offset, avail);

if (avail > PR_UINT32_MAX) avail = PR_UINT32_MAX
OnDataAvailable( ... (PRUint32)avail);

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2778,5 @@
>  
>    // if the user has passed PR_UINT32_MAX, then read
>    // everything in the stream
>  
> +  PRUint64 len = aLen == PR_UINT32_MAX ? n : aLen;

Please add parentheses for clarity.

PRUint64 len = (aLen == PR_UINT32_MAX) ? n : aLen;

::: startupcache/StartupCacheUtils.cpp
@@ +120,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(avail64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Why '<' and not '<='?

::: toolkit/components/places/nsFaviconService.cpp
@@ +550,5 @@
>      return NS_ERROR_FAILURE;
>  
>    // read all the decoded data
>    PRUint8* buffer = static_cast<PRUint8*>
>                                 (nsMemory::Alloc(sizeof(PRUint8) * available));

This is fine for the cast, but Read is called later and only take PRUint32.  I suggest you use NS_ENSURE_TRUE((available <= PR_UINT32_MAX)...).

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +2293,5 @@
>    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "SyncChannelStatus failed");
>  
>    while (NS_SUCCEEDED(rv))
>    {
> +    PRUint64 available;

OnDataAvailable is called later, so available needs to be capped at PR_UINT32_MAX before calling.

::: xpcom/components/nsComponentManager.cpp
@@ +573,5 @@
>  
>          if (avail > flen)
>              return;
>  
> +        if (NS_FAILED(is->Read(whole + totalRead, (PRUint32)avail, &read)))

Rather than just casting avail, it is better to cap it to PR_UINT32_MAX.

if (avail > PR_UINT32_MAX) avail = PR_UINT32_MAX;
...Read(...PRUint32(avail) ...);

::: xpcom/io/Base64.cpp
@@ +196,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // no support 4GB or over
> +    NS_ENSURE_TRUE(count < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Why '<' and not '<='?

::: xpcom/io/nsPipe3.cpp
@@ +743,5 @@
>      // return error if pipe closed
>      if (!mAvailable && NS_FAILED(mPipe->mStatus))
>          return mPipe->mStatus;
>  
>      *result = mAvailable;

Did you consider changing mAvailable to a PRUint64?

::: xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp
@@ +184,3 @@
>          PRUint32 read;
>  
>          if (NS_FAILED(stream->Available(&avail)))

Even though you already checked for (flen > PR_UINT32_MAX), I think it's a good idea to cap avail here before you call Read().  Just some defensive programming.

if (avail > PR_UINT32_MAX) avail = PR_UINT32_MAX;
...Read( ... (PRUint32)avail ..)
Comment 38 Josh Matthews [:jdm] 2011-08-24 20:46:55 PDT
>::: netwerk/ipc/NeckoMessageUtils.h
>@@ +214,3 @@
>>  
>>        aParam.mStream->Available(&bytes);
>> +      if (bytes > 0 && bytes < PR_UINT32_MAX) {
>
>I think this should fail more obviously if (bytes > PR_UINT32_MAX).  I suggest >adding a separate if statement before this one...
>if (bytes > PR_UINT32_MAX) {
>  NS_WARNING("Cannot copy more than 4GB");
>  return;
>}
>... and then continue with the next if statement.

This function must be infallible, so you should use NS_ABORT_IF_FALSE instead.
Comment 39 Honza Bambas (:mayhemer) 2011-09-13 11:15:02 PDT
Comment on attachment 554809 [details] [diff] [review]
fix v4

Stealing review here on Jason's request.
Comment 40 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2011-09-28 04:27:48 PDT
Created attachment 563032 [details] [diff] [review]
fix v5
Comment 41 Honza Bambas (:mayhemer) 2011-10-07 13:21:26 PDT
I started reviewing this.  I'm at about half, please don't modify the patch until I publish the comments.  Thanks.
Comment 42 Honza Bambas (:mayhemer) 2011-11-02 11:17:45 PDT
Comment on attachment 563032 [details] [diff] [review]
fix v5

Review of attachment 563032 [details] [diff] [review]:
-----------------------------------------------------------------

r-

This is just the first part of the review, I still have to finish few other files and also check on functionality indirectly affecting this patch.  Publishing this just because I'm paranoid and afraid of loosing the comments.  In other words, it is not yet time to update the patch probably ;)

::: content/base/src/nsSyncLoadService.cpp
@@ +402,2 @@
>              rv = aListener->OnDataAvailable(aChannel, nsnull, aIn,
> +                                            NS_MIN(sourceOffset, PRUint64(PR_UINT32_MAX)),

Result should be explicitly cast to PRUint32.  That will be to long - please do:

PRUint32 sourceOffset32 = (PRUint32)NS_MIN(sourceOffset, PRUint64(PR_UINT32_MAX));

and use sourceOffset32 in ODA call.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +2351,2 @@
>        postDataStream->Available(&uploadTotal);
>        mUploadTotal = uploadTotal;

You can just: postDataStream->Available(&mUploadTotal);

::: content/media/nsMediaStream.cpp
@@ +1057,3 @@
>    rv = mInput->Available(&size);
>    if (NS_SUCCEEDED(rv)) {
>      mSize = size;

mSize is signed int, you need to check for PR_INT64_MAX before you assign and fail with some NS_ERROR_FILE_TOO_BIG or OOM.

::: dom/src/json/nsJSON.cpp
@@ +536,1 @@
>      rv = aStream->Available(&available);

This has to be the same code as in nsSyncLoadService.cpp (truncate to max of PR_UINT32, cast to PRUint32, call ODA() and loop again).

@@ +677,2 @@
>    aStream->Available(&contentLength);
>    nsresult rv = NS_OK;

I think you can remove these 3 lines of code at all.

::: extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp
@@ +468,4 @@
>  
>        // Update the content length attribute on the channel.  We do this
>        // synchronously without proxying.  This hack is not as bad as it looks!
> +      if (mBytesRemaining != PR_UINT64_MAX)

This should be if (mBytesRemaining <= PR_UINT32_MAX).

(Bug 233305 comment 3).

::: extensions/pref/autoconfig/src/nsReadConfig.cpp
@@ +314,5 @@
>  
>      PRUint32 fs, amt = 0;
> +    PRUint64 fs64;
> +    rv = inStr->Available(&fs64);
> +    NS_ENSURE_SUCCESS(rv, rv);

use if (NS_FAILED(rv)) return rv here to be consistent with the rest of the file.

@@ +316,5 @@
> +    PRUint64 fs64;
> +    rv = inStr->Available(&fs64);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    // XXX no support over 4GB
> +    NS_ENSURE_TRUE(fs64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Out of scope of this bug: 64bit build should be able to allocate >4GB.  The check should be for ~PRSize(0), IMO.  But it doesn't belong here.

::: gfx/thebes/gfxASurface.cpp
@@ +793,5 @@
>  
>    // ...leave a little extra room so we can call read again and make sure we
>    // got everything. 16 bytes for better padding (maybe)
>    bufSize += 16;
> +  NS_ENSURE_TRUE(bufSize <= PR_UINT32_MAX, );

Just return, this is quit dirty.  And the check should be before the += 16 for PR_UINT32_MAX - 16.

@@ +806,5 @@
>    {
>      imgSize += numReadThisTime;
>      if (imgSize == bufSize) {
>        // need a bigger buffer, just double
>        bufSize *= 2;

Before this line we should check for for bufSize <= PR_UINT32_MAX / 2.

::: js/src/xpconnect/loader/mozJSComponentLoader.cpp
@@ +963,3 @@
>              NS_ENSURE_SUCCESS(rv, rv);
> +            // no support 4GB or over script stream
> +            NS_ENSURE_TRUE(len64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

Condition *is* correct here.  Return FILE_TOO_BIG?

::: layout/base/nsPresShell.cpp
@@ +7833,5 @@
>    rv = file->InitWithPath(name);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  PRUint64 length64;
> +  rv = encoder->Available(&length);

length64 as an arg!  (there had to be a warning on the line you use length64, treat warnings as errors please).

@@ +7835,5 @@
>  
> +  PRUint64 length64;
> +  rv = encoder->Available(&length);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(length64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG?

::: modules/libjar/nsJARChannel.cpp
@@ +164,5 @@
>          return rv;
>      }
>  
>      // ask the JarStream for the content length
> +    rv = mJarStream->Available((PRUint64 *) &mContentLength);

No.  This should first read to PRUint64 local var, check for being < PR_INT32_MAX, if not, fallback (or just leave) mContentLength to -1.

::: modules/libpref/src/Preferences.cpp
@@ +819,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
> +  // XXX no support over 4GB
> +  NS_ENSURE_TRUE(fileSize64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG ?

::: netwerk/base/src/nsInputStreamPump.cpp
@@ +470,5 @@
>          avail = 0;
>      }
>      else if (NS_SUCCEEDED(rv) && avail) {
>          // figure out how much data to report (XXX detect overflow??)
> +        if (avail + mStreamOffset > mStreamLength)

Could (avail + mStreamOffset) ever overflow?

@@ +504,5 @@
>              PRUint32 odaOffset =
>                  mStreamOffset > PR_UINT32_MAX ?
>                  PR_UINT32_MAX : PRUint32(mStreamOffset);
>  
> +            PRUint32 odaAvail = PRUint32(NS_MIN(avail, PRUint64(PR_UINT32_MAX)));

Maybe use the same code as used for odaOffset to be consistent?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +477,5 @@
>      if (contentType.IsEmpty()) {
>        mUploadStreamHasHeaders = PR_TRUE;
>        mRequestHead.SetMethod(nsHttp::Post); // POST request
>      } else {
>        if (contentLength < 0) {

I think we will have to change this API, len == 0 means "do get length your self", instead of len == -1.

REVIEW TODO: think of this API change request a bit more

@@ +482,5 @@
>          // Not really kosher to assume Available == total length of
>          // stream, but apparently works for the streams we see here.
> +        PRUint64 contentLength64;
> +        nsresult rv = stream->Available(&contentLength64);
> +        if(NS_FAILED(rv) || contentLength64 > PR_INT32_MAX) {

This is one of the core places to fix this bug actually.

If contentLength arg is < 0, then why not accept CL of full 64 bits?

Only if call to Available() failed, return NS_ERROR_FAILURE.

Actually, contentLength64 should be whole |if (stream) { }| block scope var.  If contentLength arg is >= 0, then assign it to contentLength64 and use contentLength64 for the rest of the logic.

@@ +487,5 @@
>            NS_ERROR("unable to determine content length");
>            return NS_ERROR_FAILURE;
>          }
> +        // contentLength64 is under PR_INT32_MAX
> +        contentLength = PRInt32(contentLength64);

This is non-sense.  I really don't think using this method (a "32-bits" method) should limit to 32 bits of length when aContentLength is < 0.

@@ +492,4 @@
>        }
>        // SetRequestHeader propagates headers to chrome if HttpChannelChild 
>        nsCAutoString contentLengthStr;
>        contentLengthStr.AppendInt(PRInt64(contentLength));

Here use the contentLength64.

@@ +524,5 @@
>  
>    if (aContentLength < 0 && !aStreamHasHeaders) {
> +    PRUint64 streamLength = 0;
> +    if (NS_SUCCEEDED(aStream->Available(&streamLength))) {
> +      aContentLength = streamLength;

This can go to negative values when streamLength is over 1<<63 (however unlikely).

Use PRUint64 var to operate with.  if aContentLength < 0 then read the stream's Available to it.  Otherwise assign aContentLength to it.  Then you can use it bellow, also for creating the Content-Length header value.

@@ +530,3 @@
>      if (aContentLength < 0) {
>        NS_ERROR("unable to determine content length");
>        return NS_ERROR_FAILURE;

This must fail only when call to Available() failed (NS_FAILED).

Giving r- based on these changes.
Comment 43 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2011-11-10 00:53:36 PST
Thanks, Honza.

>::: extensions/pref/autoconfig/src/nsReadConfig.cpp
>@@ +316,5 @@
>> +    PRUint64 fs64;
>> +    rv = inStr->Available(&fs64);
>> +    NS_ENSURE_SUCCESS(rv, rv);
>> +    // XXX no support over 4GB
>> +    NS_ENSURE_TRUE(fs64 <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);
>
> Out of scope of this bug: 64bit build should be able to allocate >4GB.  The check
> should be for ~PRSize(0), IMO.  But it doesn't belong here.

Should we support over 4GB config file?  Most situation is that it is corrupted.


>::: netwerk/protocol/http/HttpBaseChannel.cpp
>@@ +477,5 @@
>>      if (contentType.IsEmpty()) {
>>        mUploadStreamHasHeaders = PR_TRUE;
>>        mRequestHead.SetMethod(nsHttp::Post); // POST request
>>      } else {
>>        if (contentLength < 0) {
>
>I think we will have to change this API, len == 0 means "do get length your self",
>instead of len == -1.

I think we should support Content-Length: 0 case.  How do you think?
Comment 44 Honza Bambas (:mayhemer) 2011-11-15 08:46:43 PST
(In reply to Makoto Kato from comment #43)
> Should we support over 4GB config file?  Most situation is that it is
> corrupted.

Depends, we can report this as a separate bug.  But that is really minor.

> I think we should support Content-Length: 0 case.  How do you think?

Yes, I have to think more about this particular piece of code first.  The answer will be in the final review.
Comment 45 Josh Aas 2011-12-28 13:58:01 PST
Makoto - we'd like to finish this soon (in the next week or two), are you able to finish it or should we assign an engineer to do it?
Comment 46 Honza Bambas (:mayhemer) 2012-01-03 01:38:50 PST
(In reply to Josh Aas (Mozilla Corporation) from comment #45)
> Makoto - we'd like to finish this soon (in the next week or two), are you
> able to finish it or should we assign an engineer to do it?

I told Makato to let me first finish the review.  I plan to do it soon with other large reviews I have on the list.
Comment 47 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-05 22:13:48 PST
I have to split patch for review.
Comment 48 Honza Bambas (:mayhemer) 2012-01-09 09:49:19 PST
Comment on attachment 563032 [details] [diff] [review]
fix v5

Review of attachment 563032 [details] [diff] [review]:
-----------------------------------------------------------------

So, here is the rest of the review.  Sorry for the delay, but this is quit big.

This needs a test... at least manually check it works.

::: netwerk/base/src/nsInputStreamPump.cpp
@@ +470,5 @@
>          avail = 0;
>      }
>      else if (NS_SUCCEEDED(rv) && avail) {
>          // figure out how much data to report (XXX detect overflow??)
> +        if (avail + mStreamOffset > mStreamLength)

This should be if (avail > mStreamLength - mStreamOffset)

::: netwerk/base/src/nsSyncStreamListener.cpp
@@ +202,1 @@
>      mStatus = mPipeIn->ReadSegments(writer, closure, avail, result);

Just curious why you don't use the same style of your changes as in Read() method of this class.

::: netwerk/ipc/NeckoMessageUtils.h
@@ +209,5 @@
>      if (!serializable) {
>        NS_WARNING("nsIInputStream implementation doesn't support nsIIPCSerializable; falling back to copying data");
>  
>        nsCString streamString;
> +      PRUint64 bytes = 0;

No need to init IMO.

To clean this code up a bit (not for this bug): better to check on result of mStream->Available call, and if success && bytes > 0 or a failure different from NS_BASE_STREAM_CLOSED, treat it as bytes > 0.

@@ +214,3 @@
>  
>        aParam.mStream->Available(&bytes);
> +      NS_ABORT_IF_FALSE(bytes > PR_UINT32_MAX, "nsIInputStream has over 4GB data");

Please add a comment why you don't want to all this.

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +55,5 @@
>  #include "nsIMIMEService.h"
>  
> +#ifndef PR_INT64_MAX
> +#define PR_INT64_MAX (~((PRInt64)(1) << 63))
> +#endif

Also on more places in this patch, what is the reason not to include "prtypes.h" here?  I don't say this is wrong, just please add a comment why rather define this.

@@ +433,4 @@
>        nsresult rv = mUploadStream->Available(&avail);
>        if (NS_FAILED(rv))
>          return rv;
> +      NS_ENSURE_TRUE(avail <= PR_INT64_MAX, NS_ERROR_FAILURE);

FILE_TOO_BIG?

::: netwerk/protocol/ftp/nsFtpControlConnection.cpp
@@ +77,2 @@
>          if (NS_SUCCEEDED(rv) && n != avail)
>              avail = n;

I'd change this as:

if (NS_SUCCEEDED(rv))
   avail = n;

Otherwise you should cast n or avail in the n != avail condition.

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +457,5 @@
>          return mStatus;
>      }
>  
>      nsresult rv;
> +    PRUint64 avail = 0;

Also cast avail bellow in this method in call to ReadSegments.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +445,3 @@
>      if (NS_FAILED(mRequestStream->Available(&size)))
>          size = 0;
> +    return size > PR_UINT32_MAX ? PR_UINT32_MAX : (PRUint32)size;

Hmm... this API (nsAHttpTransaction::Available()) should for consistency be changed to PRUint64 result as well.  This simple change is allowed at this moment only because the consumers are not expecting the real size and are ready to get a 32-bits clipped value.

This should be changed in a follow bug.  Please CC Patrick McManus to that bug.

::: netwerk/streamconv/test/Converters.cpp
@@ +95,5 @@
>      if (NS_FAILED(rv)) return rv;
>  
> +    PRUint64 len;
> +    rv = convertedStream->Available(&len);
> +    NS_ENSURE_SUCCESS(rv, rv);

Why added ?

@@ +96,5 @@
>  
> +    PRUint64 len;
> +    rv = convertedStream->Available(&len);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    return mListener->OnDataAvailable(request, ctxt, convertedStream, sourceOffset, PRUint32(NS_MIN(len, PRUint64(PR_UINT32_MAX))));

Also, in case Available would return something more then PR_UINT32_MAX, you must loop this ODA call until all data gets consumed from convertedStream (i.e. push all <=4GB pieces one by one) or ODA returns a failure.

As I understand, if this would be a last call to 	TestConverter::OnDataAvailable, then not all data would get to mListener when there would be over 4GB data in convertedStream.

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +140,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    PRUint64 avail;
> +    rv = dataStream->Available(&avail);
> +    NS_ENSURE_SUCCESS(rv, rv);

Why added?

@@ +145,3 @@
>  
> +    return aListener->OnDataAvailable(request, nsnull, dataStream, 0,
> +                                      PRUint32(NS_MIN(avail, PRUint64(PR_UINT32_MAX))));

The same, you have to loop all 4GB chunks to aListener.

::: rdf/base/src/nsRDFXMLDataSource.cpp
@@ +574,2 @@
>          if (NS_SUCCEEDED(rv))
>              offset += avail;

Offset should be also PRUint64 and clamped at 4GB, as in SAX.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2969,5 @@
>  
>    // if the user has passed PR_UINT32_MAX, then read
>    // everything in the stream
>  
> +  PRUint64 len = aLen == PR_UINT32_MAX ? n : aLen;

PRUint64 len = (aLen == PR_UINT32_MAX) ? n : aLen;

::: startupcache/StartupCacheUtils.cpp
@@ +120,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(avail64 < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG ?

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +2308,5 @@
>        break; // blocking input stream has none available when done
>  
>      rv = listener->OnDataAvailable(mImportChannel, nsnull, bufferedstream, 0,
> +                                   available > PR_UINT32_MAX ?
> +                                     PR_UINT32_MAX : available);

Separate this ternary operator to its own line, don't forget to cast available to PRUint32.

::: widget/src/windows/JumpListBuilder.cpp
@@ +655,4 @@
>    rv = iconStream->Available(&bufSize);
>    NS_ENSURE_SUCCESS(rv, rv);
> +  // XXX no support over 4GB stream
> +  NS_ENSURE_TRUE(bufSize <= PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);

FILE_TOO_BIG ?

@@ +660,5 @@
>    // Setup a buffered output stream from the stream object
>    // so that we can simply use WriteFrom with the stream object
>    nsCOMPtr<nsIOutputStream> bufferedOutputStream;
>    rv = NS_NewBufferedOutputStream(getter_AddRefs(bufferedOutputStream),
>                                    outputStream, bufSize);

Cast

::: xpcom/components/nsComponentManager.cpp
@@ +556,5 @@
>  
> +    PRUint64 flen64;
> +    nsresult rv = is->Available(&flen64);
> +    NS_ENSURE_SUCCESS(rv, );
> +    NS_ENSURE_TRUE(flen64 < PR_UINT32_MAX, ); // no support 4GB or over

Just return, this soon won't build.  If you want to log, use NS_ERROR.  In case of 4GB limit I think it would be worth to.

@@ +581,1 @@
>              return;

The check above should be (just) for PRUint64(totalRead) + avail < flen64.  You must protect not going past the the allocated memory.  It could happen when Available of the stream would grow.  It is actually an already present bug in this not well written code.

::: xpcom/io/Base64.cpp
@@ +196,4 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // no support 4GB or over
> +    NS_ENSURE_TRUE(count < PR_UINT32_MAX, NS_ERROR_OUT_OF_MEMORY);
> +    aCount = PRUint32(count);

You'll have to update this, see http://hg.mozilla.org/mozilla-central/rev/aa47e51cbd8a

::: xpcom/io/nsPipe3.cpp
@@ +743,5 @@
>      // return error if pipe closed
>      if (!mAvailable && NS_FAILED(mPipe->mStatus))
>          return mPipe->mStatus;
>  
>      *result = mAvailable;

I'm not sure you need to change mAvailable to be 64-bits here.  The limit for the pipe's buffer is 4GB anyway.  See the nsPipe::Init method and maxCount = PRUint32(-1) / segmentSize math.

You may just return a casted mAvailable here I think.

This is sensitive code, I would like to put minimal changes to it.

::: xpcom/io/nsScriptableInputStream.cpp
@@ +59,5 @@
>  nsScriptableInputStream::Available(PRUint32 *_retval) {
>      if (!mInputStream) return NS_ERROR_NOT_INITIALIZED;
> +    PRUint64 avail;
> +    nsresult rv = mInputStream->Available(&avail);
> +    NS_ENSURE_SUCCESS(rv, rv);

Just return rv please, this is not a fatal error here.  Throw indicates a valid state of the stream.

@@ +71,5 @@
>      PRUint32 count = 0;
>      char *buffer = nsnull;
>  
> +    rv = Available(&count);
> +    NS_ENSURE_SUCCESS(rv, rv);

Just return rv please.  This adds an error log, seems the original coder intended to not pollute error output.

@@ +76,3 @@
>  
>      count = NS_MIN(count, aCount);
>      buffer = (char*)nsMemory::Alloc(count+1); // make room for '\0'

Please see bug 716556.

::: xpcom/io/nsStreamUtils.cpp
@@ +650,5 @@
>                  rv = NS_OK;
>              break;
>          }
>          if (avail == 0)
>              break;

You should cast avail on all places or, what I prefer, just temporarily keep it in some avail64 and then cast to avail after check for (avail > maxCount).

::: xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp
@@ +166,5 @@
>      return header;
>  }
>  
>  XPTHeader*
>  xptiInterfaceInfoManager::ReadXPTFileFromInputStream(nsIInputStream *stream)

This method had been removed completely.  Just for reference:
https://bugzilla.mozilla.org/attachment.cgi?id=579975&action=diff#a/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp_sec1

Now the file is read directly using NSPR:
http://mxr.mozilla.org/mozilla-central/source/xpcom/build/FileLocation.cpp#177

So no worries about searching an Available consumer to search for ;)
Comment 49 Honza Bambas (:mayhemer) 2012-01-09 09:51:32 PST
(In reply to Honza Bambas (:mayhemer) from comment #44)
> > I think we should support Content-Length: 0 case.  How do you think?
> 
> Yes, I have to think more about this particular piece of code first.  The
> answer will be in the final review.

You are right, leave it.
Comment 50 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 00:11:41 PST
Created attachment 591713 [details] [diff] [review]
Part 1. IDL changes
Comment 51 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 01:38:22 PST
Created attachment 591721 [details] [diff] [review]
Part4. libpref part
Comment 52 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 02:00:41 PST
Created attachment 591725 [details] [diff] [review]
Part 5. xpconnect part
Comment 53 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 02:24:23 PST
Created attachment 591727 [details] [diff] [review]
Part 6. libjar part
Comment 54 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 02:36:54 PST
Created attachment 591728 [details] [diff] [review]
Part 7. rdf part
Comment 55 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 02:56:29 PST
Created attachment 591732 [details] [diff] [review]
part 8. parser part
Comment 56 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 03:01:20 PST
Created attachment 591733 [details] [diff] [review]
part 9. gfx part
Comment 57 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 03:13:04 PST
Created attachment 591737 [details] [diff] [review]
Part 10. image part
Comment 58 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 03:25:52 PST
Created attachment 591741 [details] [diff] [review]
Part 11. dom part
Comment 59 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 03:51:38 PST
Created attachment 591745 [details] [diff] [review]
Part 12. widget part
Comment 60 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 04:08:37 PST
Created attachment 591750 [details] [diff] [review]
Part 13. content part
Comment 61 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 17:36:00 PST
Created attachment 592009 [details] [diff] [review]
Part 2. xpcom part
Comment 62 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 17:37:04 PST
Created attachment 592010 [details] [diff] [review]
Part 14. toolkit part
Comment 63 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 17:39:49 PST
Created attachment 592012 [details] [diff] [review]
Part 15. security part
Comment 64 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 20:55:56 PST
Created attachment 592044 [details] [diff] [review]
Part 17. startupcache
Comment 65 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 20:56:27 PST
Created attachment 592045 [details] [diff] [review]
Part 16. extension part
Comment 66 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 22:28:47 PST
Created attachment 592059 [details] [diff] [review]
Part 8. parser part
Comment 67 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 22:29:45 PST
Created attachment 592061 [details] [diff] [review]
Part 5. xpconnect part
Comment 68 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-26 22:33:42 PST
Created attachment 592062 [details] [diff] [review]
Part 3. network part

waiting for bug 720627 ...
Comment 69 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-01-27 00:11:05 PST
Created attachment 592072 [details] [diff] [review]
Part 6. libjar part
Comment 70 Honza Bambas (:mayhemer) 2012-02-07 10:01:55 PST
BTW, I would prefer a merged patch (instead/besides).  I like to compare patches directly, this makes it very hard.  But for this round let's go with this.
Comment 71 Honza Bambas (:mayhemer) 2012-02-07 15:17:16 PST
Makato, I'm missing changes to layout/base/nsPresShell.cpp.
Comment 72 Honza Bambas (:mayhemer) 2012-02-07 15:33:47 PST
And also to xpcom/components/nsComponentManager.cpp
Comment 73 Honza Bambas (:mayhemer) 2012-02-07 16:01:05 PST
Adding Jeff, I'd like to extend httpd.js to support or at least accept and intercept uploads with >32 bit length.
Comment 74 Honza Bambas (:mayhemer) 2012-02-08 12:18:49 PST
Comment on attachment 591713 [details] [diff] [review]
Part 1. IDL changes

Review of attachment 591713 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

I'd like Benjamin to check on the change to nsIScriptableInputStream.
Comment 75 Honza Bambas (:mayhemer) 2012-02-08 12:20:38 PST
Comment on attachment 592062 [details] [diff] [review]
Part 3. network part

Review of attachment 592062 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab, preliminarily giving r+.  But before an official one I want to test this.  Adding some simple support to httpd.js and try creating a test is IMO the best way.  Not many widely used servers now support >4G uploads.

::: netwerk/protocol/device/AndroidCaptureProvider.cpp
@@ +138,4 @@
>  {
>    mozilla::ReentrantMonitorAutoEnter autoMonitor(mMonitor);
>  
>    *aAvailable = mAvailable;

Cast?

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +429,4 @@
>        nsresult rv = mUploadStream->Available(&avail);
>        if (NS_FAILED(rv))
>          return rv;
>        mUploadLength = avail;

Here you were checking for avail < INT64_MAX.  It should be preserved.  We should never have mUploadStream && mUploadLength < 0.  It would break logic in nsFileCopyEvent::DoCopy().

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +483,5 @@
> +       hasHeaders = true;
> +     } else {
> +       method = nsHttp::Put;
> +       hasHeaders = false;
> +     }

Indention.

@@ +491,5 @@
> +
> +  // if stream is null, ExplicitSetUploadStream returns error.
> +  // So we need special case for GET method.
> +  mUploadStreamHasHeaders = false;
> +  mRequestHead.SetMethod(nsHttp::Get); // revert to GET request

Nice!

@@ +511,5 @@
>    // Ensure stream is set and method is valid 
>    NS_ENSURE_TRUE(aStream, NS_ERROR_FAILURE);
>  
>    if (aContentLength < 0 && !aStreamHasHeaders) {
> +    aStream->Available((PRUint64*)&aContentLength);

This should be static cast.

Also I think we really should add an NS_FAILED(rv) check here.

If call to Available fails, then calls to any method of the stream fail.

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +738,5 @@
>  
>      while ((trans = Request(0)) != nsnull) {
>          avail = trans->Available();
>          if (avail) {
> +            rv = trans->ReadSegments(this, (PRUint32)NS_MIN(avail, (PRUint64)PR_UINT32_MAX), &n);

Also according the 80 lines limit, please separate the NS_MIN evaluation to its own line.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +167,5 @@
>      nsCOMPtr<nsIHttpActivityObserver> mActivityDistributor;
>  
>      nsCString                       mReqHeaderBuf;    // flattened request headers
>      nsCOMPtr<nsIInputStream>        mRequestStream;
> +    PRUint64                        mRequestSize;

You may also remove the comment about 32 bits in nsHttpTransaction::OnTransportStatus

::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp
@@ +125,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    streamLen = (PRUint32)NS_MIN(streamLen64, PRUint64(PR_UINT32_MAX - 1));

Nice, one I didn't catch before ;)

::: netwerk/streamconv/test/Converters.cpp
@@ +104,1 @@
>      convertedStream->Available(&len);

Ah, I didn't realize previously you wanted to check on validity of |len|.  But this is probably OK for the testing code.

@@ +106,5 @@
> +    PRUint64 offset = sourceOffset;
> +    while (len > 0) {
> +        PRUint32 count = saturated(len);
> +        rv = mListener->OnDataAvailable(request, ctxt, convertedStream, saturated(offset), count);
> +        NS_ENSURE_SUCCESS(rv, rv);

Yes, but please only if (NS_FAILED(rv)) return rv;

::: netwerk/streamconv/test/TestStreamConv.cpp
@@ +150,5 @@
> +    while (avail > 0) {
> +        PRUint32 count = saturated(avail);
> +        rv = aListener->OnDataAvailable(request, nsnull, dataStream,
> +                                        saturated(offset), count);
> +        NS_ENSURE_SUCCESS(rv, rv);

Same here, just return.
Comment 76 Honza Bambas (:mayhemer) 2012-02-08 12:20:46 PST
Comment on attachment 591721 [details] [diff] [review]
Part4. libpref part

Review of attachment 591721 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 77 Honza Bambas (:mayhemer) 2012-02-08 12:21:03 PST
Comment on attachment 592061 [details] [diff] [review]
Part 5. xpconnect part

Review of attachment 592061 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +910,2 @@
>              NS_ENSURE_SUCCESS(rv, rv);
> +            NS_ENSURE_TRUE(len64 <= PR_UINT32_MAX, NS_ERROR_FILE_TOO_BIG);

The condition was correct :)  please change it to |<|.
Comment 78 Honza Bambas (:mayhemer) 2012-02-08 12:21:19 PST
Comment on attachment 592072 [details] [diff] [review]
Part 6. libjar part

Review of attachment 592072 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with the comments addressed.

::: modules/libjar/nsJAR.cpp
@@ +469,2 @@
>    if (NS_FAILED(rv)) return rv;
> +  NS_ENSURE_TRUE(len64 <= PR_UINT32_MAX, NS_ERROR_FILE_CORRUPTED); // bug 164695

Again, this was correct, change the condition to <.
Comment 79 Honza Bambas (:mayhemer) 2012-02-08 12:21:37 PST
Comment on attachment 591728 [details] [diff] [review]
Part 7. rdf part

Review of attachment 591728 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 80 Honza Bambas (:mayhemer) 2012-02-08 12:21:51 PST
Comment on attachment 592059 [details] [diff] [review]
Part 8. parser part

Review of attachment 592059 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 81 Honza Bambas (:mayhemer) 2012-02-08 12:22:24 PST
Comment on attachment 591733 [details] [diff] [review]
part 9. gfx part

Review of attachment 591733 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with comments addressed

::: gfx/thebes/gfxASurface.cpp
@@ +802,4 @@
>    if (NS_FAILED(rv))
>      return;
>  
> +  if (bufSize64 - 16 > PR_UINT32_MAX)

This is wrong, I think.  The check has to be (bufSize64 > PR_UINT32_MAX - 16)

@@ +803,5 @@
>      return;
>  
> +  if (bufSize64 - 16 > PR_UINT32_MAX)
> +    return;
> +  PRUint32 bufSize = (PRUint32)bufSize64;

Blank line before this one please.
Comment 82 Honza Bambas (:mayhemer) 2012-02-08 12:22:38 PST
Comment on attachment 591737 [details] [diff] [review]
Part 10. image part

Review of attachment 591737 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 83 Honza Bambas (:mayhemer) 2012-02-08 12:22:49 PST
Comment on attachment 591741 [details] [diff] [review]
Part 11. dom part

Review of attachment 591741 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 84 Honza Bambas (:mayhemer) 2012-02-08 12:22:59 PST
Comment on attachment 591745 [details] [diff] [review]
Part 12. widget part

Review of attachment 591745 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 85 Honza Bambas (:mayhemer) 2012-02-08 12:23:23 PST
Comment on attachment 591750 [details] [diff] [review]
Part 13. content part

Review of attachment 591750 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with the comments addressed

::: content/base/src/nsFrameMessageManager.cpp
@@ +796,2 @@
>      nsCString buffer;
> +    PRUint32 avail = (PRUint32)NS_MIN(avail64, (PRUint64)PR_UINT32_MAX);

Hmm... hard to say what is right here.  But I think we should fail with FILE_TOO_BIG if avail64 > PR_UINT32_MAX.  Processing just part of the data is IMO not correct.
Comment 86 Honza Bambas (:mayhemer) 2012-02-08 12:23:40 PST
Comment on attachment 592010 [details] [diff] [review]
Part 14. toolkit part

Review of attachment 592010 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: toolkit/components/places/nsFaviconService.cpp
@@ +644,1 @@
>      return NS_ERROR_FAILURE;

Good!  You just may want to return NS_ERROR_FILE_TOO_BIG for (available64 > PR_UINT32_MAX / sizeof(PRUint8))
Comment 87 Honza Bambas (:mayhemer) 2012-02-08 12:24:15 PST
Comment on attachment 592012 [details] [diff] [review]
Part 15. security part

Review of attachment 592012 [details] [diff] [review]:
-----------------------------------------------------------------

r-

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +2777,5 @@
>    nsresult rv = data->Available(&n);
>    if (NS_FAILED(rv))
>      return rv;
> +  if (n > PR_UINT32_MAX)
> +    return NS_ERROR_FILE_TOO_BIG;

No, you have to respect the len argument.  We should actually read MIN(n, len).

Please file a followup to change this API to 64 bit.

@@ +2977,5 @@
>    // if the user has passed PR_UINT32_MAX, then read
>    // everything in the stream
>  
>    if (aLen == PR_UINT32_MAX)
> +    aLen = (PRUint32)n;

Same here..
Comment 88 Honza Bambas (:mayhemer) 2012-02-08 12:24:45 PST
Comment on attachment 592045 [details] [diff] [review]
Part 16. extension part

Review of attachment 592045 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp
@@ +465,5 @@
>        // Update the content length attribute on the channel.  We do this
>        // synchronously without proxying.  This hack is not as bad as it looks!
> +      if (mBytesRemaining != PR_UINT64_MAX)
> +        // XXX 64-bit
> +        mChannel->SetContentLength((PRInt32)mBytesRemaining);

Clamp to PR_UINT32_MAX for now?  This definitely needs a followup bug, can you file one please?

Braces around the two-lines of comment + command.

(My previous review comment on this line was wrong, btw!)
Comment 89 Honza Bambas (:mayhemer) 2012-02-08 12:24:56 PST
Comment on attachment 592044 [details] [diff] [review]
Part 17. startupcache

Review of attachment 592044 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 90 Honza Bambas (:mayhemer) 2012-02-08 12:26:35 PST
Comment on attachment 592009 [details] [diff] [review]
Part 2. xpcom part

Review of attachment 592009 [details] [diff] [review]:
-----------------------------------------------------------------

Left r?.

::: xpcom/io/nsPipe3.cpp
@@ +137,5 @@
>  
>      nsresult Fill();
>      void SetNonBlocking(bool aNonBlocking) { mBlocking = !aNonBlocking; }
>  
> +    PRUint64 Available() { return mAvailable; }

Cast.

However, why are you adding this change?

@@ +744,5 @@
>      // return error if pipe closed
>      if (!mAvailable && NS_FAILED(mPipe->mStatus))
>          return mPipe->mStatus;
>  
>      *result = mAvailable;

Cast

::: xpcom/io/nsScriptableInputStream.cpp
@@ +73,3 @@
>      if (NS_FAILED(rv)) return rv;
>  
> +    PRUint32 count = (PRUint32)NS_MIN(count64, (PRUint64)aCount);

This still waits for bug 716556...
Comment 91 Benjamin Smedberg [:bsmedberg] 2012-02-09 13:00:11 PST
Comment on attachment 591713 [details] [diff] [review]
Part 1. IDL changes

This is certainly ok from an XPCOM perspective. I'm a little worried that we might end up with JS issues because JS numbers don't have full 64-bit precision. Is that something we need to audit for somehow?
Comment 92 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-02-09 13:11:09 PST
In practice, that will only be an issue for things that are > 53 bits, right?  That gets us up to 8 petabytes or so.  Probably safe for a few years...
Comment 93 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-02-12 21:48:31 PST
(In reply to Honza Bambas (:mayhemer) from comment #72)
> And also to xpcom/components/nsComponentManager.cpp

By bug 695843 (changeset 502c67d69baa), nsIInputStream->Available() usages is removed.
Comment 94 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-02-12 22:38:00 PST
Created attachment 596577 [details] [diff] [review]
Part 18. layout part
Comment 95 Honza Bambas (:mayhemer) 2012-02-13 10:52:28 PST
Comment on attachment 596577 [details] [diff] [review]
Part 18. layout part

Review of attachment 596577 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab
Comment 96 Honza Bambas (:mayhemer) 2012-02-14 07:30:12 PST
Comment on attachment 592009 [details] [diff] [review]
Part 2. xpcom part

I think you can now merge this to a patch in bug 716556.
Comment 97 Honza Bambas (:mayhemer) 2012-03-19 12:16:16 PDT
Makato, any updates from you?  Or should someone else take your work and finish it instead?
Comment 98 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-03-20 07:31:29 PDT
Created attachment 607540 [details] [diff] [review]
Part 19. for comm-central
Comment 99 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-03-20 07:33:11 PDT
Created attachment 607541 [details] [diff] [review]
Part 2. xpcom part
Comment 100 Honza Bambas (:mayhemer) 2012-03-20 08:17:19 PDT
Comment on attachment 607541 [details] [diff] [review]
Part 2. xpcom part

Review of attachment 607541 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't address my requirements from comment 90.


This applies to all my review comments for all patches ("r+ with comments" means there are things to update and I trust you do them).  Please address or discuss them before landing all the patches.
Comment 101 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-03-20 08:22:18 PDT
(In reply to Honza Bambas (:mayhemer) from comment #100)
> This applies to all my review comments for all patches ("r+ with comments"
> means there are things to update and I trust you do them).  Please address
> or discuss them before landing all the patches.

oh, I have missed comment #90... I will update tomorrow.
Comment 102 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-03-20 08:47:18 PDT
Created attachment 607570 [details] [diff] [review]
Part 15. security part

before this, I have to fix updateFromStream makes 64bit.  if len param is just 4GB, it isn't clear whether 4GB is unknown or just size.
Comment 103 David :Bienvenu 2012-03-21 13:59:03 PDT
Comment on attachment 607540 [details] [diff] [review]
Part 19. for comm-central

looks ok, but can you wrap some of the long lines at 80 chars? thx!

-      m_outputStream->WriteFrom(mPostDataStream, NS_MIN(avail, mSuspendedReadBytes), &amountWritten);
+      m_outputStream->WriteFrom(mPostDataStream, (PRUint32)NS_MIN<PRUint64>(avail, mSuspendedReadBytes), &amountWritten);

-      mOutListener->OnDataAvailable(mChannel, mURL, mInputStream, 0, avail);
+      mOutListener->OnDataAvailable(mChannel, mURL, mInputStream, 0, (PRUint32)NS_MIN<PRUint64>(avail, PR_UINT32_MAX));

-      rv2 = mOutListener->OnDataAvailable(request, mURL, mInputStream, 0, bytesInStream);
+      mOutListener->OnDataAvailable(request, mURL, mInputStream, 0, (PRUint32)NS_MIN<PRUint64>(bytesInStream, PR_UINT32_MAX));

-        m_channelListener->OnDataAvailable(this, m_channelContext, mDisplayInputStream, 0, inlength);
+        m_channelListener->OnDataAvailable(this, m_channelContext, mDisplayInputStream, 0, (PRUint32)NS_MIN<PRUint64>(inlength, PR_UINT32_MAX));
Comment 104 Honza Bambas (:mayhemer) 2012-03-22 16:58:10 PDT
Comment on attachment 607541 [details] [diff] [review]
Part 2. xpcom part

Review of attachment 607541 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsStreamUtils.cpp
@@ +638,3 @@
>  
>  nsresult
>  NS_ConsumeStream(nsIInputStream *stream, PRUint32 maxCount, nsACString &result)

Actually, Makato, maxCount should change here to 64 bit as well.

If you change OnDataAvailable to be 64 bit, then you'll hit this anyway.
Comment 105 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-03-23 02:58:16 PDT
Created attachment 608640 [details] [diff] [review]
Part 2. xpcom part v4
Comment 106 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-03-23 03:24:28 PDT
Comment on attachment 608640 [details] [diff] [review]
Part 2. xpcom part v4

Although NS_ConsumeStream changes to 64-bit, nsString cannot handle over 4GB length.  I add overflow check to it.
Comment 107 Honza Bambas (:mayhemer) 2012-03-26 17:18:37 PDT
Comment on attachment 608640 [details] [diff] [review]
Part 2. xpcom part v4

Review of attachment 608640 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsStreamUtils.cpp
@@ +641,3 @@
>  {
> +    // XXX nsIInputStearm supports 64-bit length stream, but nsString support
> +    // until 4GB 

Aggrhh... I'm sorry.  I didn't realize this consumes to nsACString... 

As I see this code now, I realize it's a bad advice to make maxCount 64 bit...


e.g. nsStreamCipher::UpdateFromStream that delegates to consume from string (quit strange implementation btw), should call NS_ConsumeStream+UpdateFromString in loops for max 32bit lengths, similar to as you do for OnDataAvailable on some places.

This method's signature has to stay the same...

@@ +661,5 @@
>          PRUint32 length = result.Length();
> +        // nsString cannot handle over 4GB length.
> +        if (length + avail64 > PR_UINT32_MAX)
> +            return NS_ERROR_FILE_TOO_BIG;
> +        PRUint32 avail = (PRUint32)avail64;

The code at [1] was correct then.  Please just add similar check as you do here but in form:

if (avail > PR_UINT32_MAX - length)
  return NS_ERROR_OUT_OF_MEMORY;

Subtract since (avail + length) may overflow: this will prevent potential security issue with buffer overflow when we consume more then just a single stream to just a single string and length of the streams together may overlap 2^32.

There is currently no place in the platform code that would do that.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=607541&action=diff#a/xpcom/io/nsStreamUtils.cpp_sec1
Comment 108 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-06-14 07:56:48 PDT
*** Bug 764762 has been marked as a duplicate of this bug. ***
Comment 109 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-06-18 03:14:58 PDT
Created attachment 633976 [details] [diff] [review]
Part 20. dom/devicestorage part
Comment 110 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-06-18 03:16:05 PDT
Created attachment 633977 [details] [diff] [review]
Part 2. xpcom part v4.1
Comment 111 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-06-18 03:22:13 PDT
Created attachment 633979 [details] [diff] [review]
Part 15. security part v2
Comment 112 Honza Bambas (:mayhemer) 2012-06-20 08:47:12 PDT
Comment on attachment 633977 [details] [diff] [review]
Part 2. xpcom part v4.1

Review of attachment 633977 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of a non-addressed comment.

::: xpcom/io/nsStreamUtils.cpp
@@ +662,3 @@
>          result.SetLength(length + avail);
>          if (result.Length() != (length + avail))
>              return NS_ERROR_OUT_OF_MEMORY;

You didn't address my comment 107 here.

Add also check before call to SetLength on the result:

if (avail > PR_UINT32_MAX - length)
  return NS_ERROR_OUT_OF_MEMORY;
Comment 113 Honza Bambas (:mayhemer) 2012-06-20 08:56:12 PDT
Comment on attachment 633979 [details] [diff] [review]
Part 15. security part v2

Review of attachment 633979 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab.

That's it, thanks.
Comment 114 Honza Bambas (:mayhemer) 2012-06-20 09:06:31 PDT
Comment on attachment 633976 [details] [diff] [review]
Part 20. dom/devicestorage part

Review of attachment 633976 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with a nit.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +787,5 @@
>      }
>  
> +    while (bufSize) {
> +      PRUint32 wrote;
> +      rv = bufferedOutputStream->WriteFrom(stream, static_cast<PRUint32>(NS_MIN<PRUint64>(bufSize, PR_UINT32_MAX)), &wrote);

Nicer would be to have a separate local var PRUint32 read = stream, static_cast<PRUint32>(NS_MIN<PRUint64>(bufSize, PR_UINT32_MAX));  This is hard to read.
Comment 115 Honza Bambas (:mayhemer) 2012-07-18 08:04:16 PDT
Taking this bug now since there is no activity and we want to get this done.  This is nearly done, just unbitrott (hopefully no major changes needed) and get some last part for XPCOM reviewed.
Comment 116 Honza Bambas (:mayhemer) 2012-08-03 10:47:15 PDT
Comment on attachment 607540 [details] [diff] [review]
Part 19. for comm-central

Major issues in this bug, r-.
Comment 117 Honza Bambas (:mayhemer) 2012-08-06 06:57:39 PDT
Created attachment 649259 [details] [diff] [review]
Merged WIP 1

For backup, folded, rebased and buildable patch.  Not tested so far.
Comment 118 Honza Bambas (:mayhemer) 2012-08-10 09:15:04 PDT
Created attachment 650904 [details] [diff] [review]
v1

This bitrotts so fast!  Merged again and this time also testes with 64 patched PHP

r=honzab except for xpcom parts.  Subpatch for just those parts will follow.
Comment 119 Honza Bambas (:mayhemer) 2012-08-10 10:20:20 PDT
Created attachment 650929 [details] [diff] [review]
v1 with my review comments addressed

r=honzab except the xpcom parts.
Comment 120 Honza Bambas (:mayhemer) 2012-08-10 10:27:50 PDT
Created attachment 650935 [details] [diff] [review]
Check for overflow in NS_ConsumeStream

Only change in the xpcom part patch I was missing and hence gave r-.  

This should get reviewed by some xpcom peer, extremely simple patch, tho.
Comment 121 Honza Bambas (:mayhemer) 2012-08-10 10:32:07 PDT
Created attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

This is contained in the "v1 with my review comments addressed" patch.  It is actually just an interdiff after my review+ comments were addressed (by me).

Probably, respected peers for each module should re-review this as well, some of the changes are of the same weight as the missing xpcom change, that I asked to r from Benjamin.


Josh (or anyone), may I ask you to arrange those reviews, or just land this as is?
Comment 122 Josh Aas 2012-08-10 10:40:06 PDT
Temporarily taking this bug so I remember to arrange reviews and get this landed.
Comment 123 Honza Bambas (:mayhemer) 2012-08-10 10:58:57 PDT
Created attachment 650947 [details] [diff] [review]
comm-central v2

No idea who should review this.
Comment 124 Joe Drew (not getting mail) 2012-08-10 11:02:18 PDT
Comment on attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

Review of attachment 650940 [details] [diff] [review]:
-----------------------------------------------------------------

stealing review from BenWa
Comment 125 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-10 11:24:03 PDT
Comment on attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

Review of attachment 650940 [details] [diff] [review]:
-----------------------------------------------------------------

Just one non-critical nit.

::: xpcom/io/nsStreamUtils.cpp
@@ +626,5 @@
>          // resize result buffer
>          PRUint32 length = result.Length();
> +        if (avail > PR_UINT32_MAX - length)
> +            return NS_ERROR_FILE_TOO_BIG;
> +        

There's some extra whitespace here.
Comment 126 Josh Aas 2012-08-10 17:16:04 PDT
One more try run before I land it:

https://tbpl.mozilla.org/?tree=Try&rev=4b1a019515e3
Comment 127 Josh Aas 2012-08-10 19:45:23 PDT
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/14e988e17b79
Comment 128 Honza Bambas (:mayhemer) 2012-08-11 04:40:06 PDT
Thanks for the reviews and finishing this Josh!

A note about how I was testing this:
- Fedora x64 box
- Apache x64
- PHP x64 with patch for https://bugs.php.net/bug.php?id=44522 (from 2012-03-26 03:59) applied
- limits for PHP upload raised to some 9GB, as well as script timeout
- simple PHP upload form
- Win32 build of Firefox on win7 x64
- uploading some 7.5GB file
- sha1 checksum confirmed the file has arrived correctly to the server
Comment 129 Ryan VanderMeulen [:RyanVM] 2012-08-11 19:54:38 PDT
https://hg.mozilla.org/mozilla-central/rev/14e988e17b79
Comment 130 Ian Neal 2012-08-12 02:21:41 PDT
(In reply to Ryan VanderMeulen from comment #129)
> https://hg.mozilla.org/mozilla-central/rev/14e988e17b79

This landing seems to have completely broken comm-central builds for both Thunderbird and SeaMonkey:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/util/nsMsgLineBuffer.cpp: In member function ‘char* nsMsgLineStreamBuffer::ReadNextLine(nsIInputStream*, PRUint32&, bool&, nsresult*, bool)’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/util/nsMsgLineBuffer.cpp:324:51: error: no matching function for call to ‘nsIInputStream::Available(PRUint32*)’
../../../mozilla/dist/include/nsIInputStream.h:66:60: note: candidate is: virtual nsresult nsIInputStream::Available(PRUint64*)
NEXT ERROR make[6]: *** [nsMsgLineBuffer.o] Error 1
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/src/nsMessenger.cpp: In member function ‘virtual nsresult nsSaveMsgListener::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, PRUint32, PRUint32)’:
/builds/slave/comm-cen-trunk-lnx/build/mailnews/base/src/nsMessenger.cpp:1908:40: error: no matching function for call to ‘nsIInputStream::Available(PRUint32*)’
../../../mozilla/dist/include/nsIInputStream.h:66:60: note: candidate is: virtual nsresult nsIInputStream::Available(PRUint64*)
NEXT ERROR make[6]: *** [nsMessenger.o] Error 1
Comment 131 Ian Neal 2012-08-12 02:23:38 PDT
Reopening as there are unreviewed patches that will need checking in.
Comment 132 Ian Neal 2012-08-12 02:31:24 PDT
Comment on attachment 650947 [details] [diff] [review]
comm-central v2

Spreading the review net slightly wider, whomever can get to it first.
Comment 133 neil@parkwaycc.co.uk 2012-08-12 03:56:29 PDT
Comment on attachment 650947 [details] [diff] [review]
comm-central v2

>-    PRUint32 numBytesToCopy = NS_MIN(numFreeBytesInBuffer - 1 /* leave one for a null terminator */, numBytesInStream);
>+    PRUint32 numBytesToCopy = (PRUint32)NS_MIN<PRUint64>(numFreeBytesInBuffer - 1 /* leave one for a null terminator */, numBytesInStream);
Bah, we could really do with uint32_t NS_MIN(uint32_t, uint64_t) and/or uint32_t NS_MIN(uint64_t, uint32_t) overloads. Or just a private version in nsMsgUtils.h would suffice.

>-          rv = aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096U), &bytesWritten);
>+          rv = aOutStream->WriteFrom(mInStream, (PRUint32)NS_MIN<PRUint64>(avail, 4096), &bytesWritten);
Can't you write 4096ULL?

>+      *pProgress = NS_MAX<PRUint64>(totalBytes - bytesLeft, PR_UINT32_MAX);
Don't you mean NS_MIN here?
Comment 134 neil@parkwaycc.co.uk 2012-08-12 04:12:11 PDT
Comment on attachment 650940 [details] [diff] [review]
interdiff: my "r+ with comments" addressed

>+    if (avail64 > PR_UINT32_MAX) {
>+      return;
>+    }
>     nsCString buffer;
>     PRUint32 avail = (PRUint32)NS_MIN(avail64, (PRUint64)PR_UINT32_MAX);
[Because of the new range check, this could have been turned into a cast...]
Comment 135 Mark Banner (:standard8) 2012-08-13 06:09:17 PDT
Created attachment 651342 [details] [diff] [review]
comm-central v3

v3 patch for comm-central, updated with Neil's comments.
Comment 136 Mark Banner (:standard8) 2012-08-13 07:20:24 PDT
Created attachment 651373 [details] [diff] [review]
comm-central v3a

Adds an inline statement to nsMsgUtils.h to fix linker bustage.
Comment 137 neil@parkwaycc.co.uk 2012-08-13 14:47:58 PDT
Comment on attachment 651373 [details] [diff] [review]
comm-central v3a

So, having a single NS_MIN overload causes more problems than it solves, and the compiler will happily let you pass the parameters in reverse order and silently do the wrong thing. Fortunately having both overloads seems to work. (I know we're unlikely to actually see any 4GB values in mailnews, but...)

>-          rv = aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096U), &bytesWritten);
>+          rv = aOutStream->WriteFrom(mInStream, NS_MIN(avail, 4096ULL), &bytesWritten);
So, it turns out I was wrong here, because on some systems PRUint64 is long, not long long. In that case, what happens is that you end up calling your NS_MIN(uint32_t, uint64_t) overload, thus silently truncating avail to 32 bits. Oops.

>-      m_outputStream->WriteFrom(mPostDataStream, NS_MIN(avail, mSuspendedReadBytes), &amountWritten);
>+      m_outputStream->WriteFrom(mPostDataStream, NS_MIN(mSuspendedReadBytes, avail), &amountWritten);
With both overloads you don't have to worry about parameter order, so this can be reverted. (The other cases where the parameters are the wrong way around suffer from the same problem as above.)

>+inline uint32_t NS_MIN(uint32_t a, uint64_t b)
>+{
>+  return b < a ? b : a;
>+}
Nit: Please cast the 64-bit variable to 32-bit in case compilers start warning about it.

>+      *pProgress = NS_MIN<PRUint64>(totalBytes - bytesLeft, PR_UINT32_MAX);
Nit: don't need the <PRUint64> here. (Note that *pProgress is 32-bit, so we want the overload version anyway.)

r=me with those fixed.
Comment 138 Mark Banner (:standard8) 2012-08-13 15:22:21 PDT
Comment on attachment 651373 [details] [diff] [review]
comm-central v3a

Checked in with comments addressed:

https://hg.mozilla.org/comm-central/rev/9b46fc7fd4a6

Thanks to Honza for providing the patch originally.
Comment 139 David Bruant 2012-10-13 16:49:48 PDT
Why is this bug marked dev-doc-needed? What needs to be documented?
Comment 140 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-10-14 17:50:58 PDT
(In reply to David Bruant from comment #139)
> Why is this bug marked dev-doc-needed? What needs to be documented?

We need update the document of nsIInputStream. available method is changed.
Comment 141 Thomas McGrew 2013-01-08 06:11:06 PST
*** Bug 827526 has been marked as a duplicate of this bug. ***
Comment 142 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2014-05-07 22:26:58 PDT
*** Bug 607313 has been marked as a duplicate of this bug. ***
Comment 143 Honza Bambas (:mayhemer) 2016-02-09 09:39:41 PST
*** Bug 660159 has been marked as a duplicate of this bug. ***

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