Last Comment Bug 536324 - Change nsIChannel to support 64-bit content-length
: Change nsIChannel to support 64-bit content-length
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Other Branch
: x86 Linux
: -- normal with 2 votes (vote)
: mozilla19
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on: 530952 589292 591997
Blocks: 740122 fennecko
  Show dependency treegraph
 
Reported: 2009-12-21 20:54 PST by Jason Duell [:jduell] (needinfo me)
Modified: 2013-03-22 13:51 PDT (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: make nsIChannel.contentLength 64-bit (1.18 KB, patch)
2010-08-18 15:17 PDT, dwitte@gmail.com
jduell.mcbugs: review+
jst: superreview+
Details | Diff | Splinter Review
part 2: implementors (36.28 KB, patch)
2010-08-18 15:20 PDT, dwitte@gmail.com
jduell.mcbugs: review+
Details | Diff | Splinter Review
part 3: consumers (43.77 KB, patch)
2010-08-18 15:20 PDT, dwitte@gmail.com
jduell.mcbugs: review+
Details | Diff | Splinter Review
part 1: make nsIChannel.contentLength 64-bit (2.27 KB, patch)
2012-10-05 12:36 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
hurley: superreview+
Details | Diff | Splinter Review
part 2: implementors (35.67 KB, patch)
2012-10-05 12:38 PDT, Nicholas Hurley [:nwgh][:hurley]
sjhworkman: review+
Details | Diff | Splinter Review
part 3: consumers (33.88 KB, patch)
2012-10-05 12:38 PDT, Nicholas Hurley [:nwgh][:hurley]
sjhworkman: review+
Details | Diff | Splinter Review
part 4: test (2.67 KB, patch)
2012-10-16 15:38 PDT, Nicholas Hurley [:nwgh][:hurley]
sjhworkman: review+
Details | Diff | Splinter Review
part 2: implementors (version for checkin) (35.68 KB, patch)
2012-10-22 10:03 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review
part 3: consumers (version for checkin) (34.37 KB, patch)
2012-10-22 10:04 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review
part 4: test (version for checkin) (2.67 KB, patch)
2012-10-22 10:07 PDT, Nicholas Hurley [:nwgh][:hurley]
hurley: review+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2009-12-21 20:54:44 PST
Another feature of necko HTTP channels that maps badly to e10s is the fact that necko channels are also nsHashPropertyBags, i.e. hashtables that clients can stuff various things into and then pull out later.

I would really like to avoid sending IPC traffic around for get/sets of these hashtables.   bz seems to think that in most cases, properties will only be retrieved by the same parent or child that set them, so we may not have to worry about this too much--just document that we don't support retrieval of properties set across a process boundary.

We have at least one known exception to this--we set a 'referer' property that may need to cross process boundaries.  There may be others.  So we should do a code or runtime audit of what gets set/retrieved to see what may need special handling.

Properties that need to be sent across the wire probably ought to be removed from the hashbag and instead put somewhere like nsIHttpChannelInternal.idl.  Actually, if we could move *all* uses to nsIHttpChannelInternal and/or something sensible like a context "void *" member variable, I'd be much happier.  Storing cruft in your objects in a big hashtable and doing lookups by name at runtime is the kind of programming that belongs in Javascript, not C++.
Comment 1 Jason Duell [:jduell] (needinfo me) 2010-01-19 17:08:15 PST
Biesi tells me we also stuff in a 64-bit Content-length field into our PropertyBags, too (as NS_CHANNEL_PROP_CONTENT_LENGTH).  This is used by nsExternalHelperAppService and nsIncrementalDownload, at least, and possibly other things.  This may also need to be propagated across process boundaries (though it also might not, assuming uriloader lives in the chrome process, and so do nsExternalHelperAppService/nsIncrementalDownload).

It's also possible that we'll need to propagate NS_CHANNEL_PROP_CONTENT_DISPOSITION, which is used by the JAR channel (which may have an Http channel target, i.e. jar:http://www.foo.com/foo.jar) and URILoader.

While I still think it's a splendid idea to refactor nsHashBag out of our channels, realistically I think we just need to keep the API for now and propagate the the handful of properties that need it.
Comment 2 Jason Duell [:jduell] (needinfo me) 2010-02-25 11:55:50 PST
We should feel free to change the API for necko to help get rid of these hashbag properties, particularly the ones that need to be propagated between child/chrome.

Bsmedberg suggests we change the APIs on mozilla-central first, and then propagate to the e10s branch.
Comment 3 dwitte@gmail.com 2010-02-26 12:07:09 PST
I can take this. We should break this stuff sooner rather than later, if we're going to do it.
Comment 4 dwitte@gmail.com 2010-03-04 15:45:57 PST
I have a patch that adds an nsIChannel2 with a contentLength64 property, and changes the dozen or so nsIChannel implementations in the tree to also implement nsIChannel2.

Frankly it's pretty gross, but we can do it if necessary. And that's just for that one property. There'll be other hashbag-related ones to come.

I'd rather wait to see if we're going to break binary compatibility on nsIChannel for other reasons. If so, the contentLength thing becomes much nicer.
Comment 5 dwitte@gmail.com 2010-08-16 19:59:07 PDT
We've decided that binary changes are OK now, right? (Is there a deadline for such changes? Last beta?)

The changes here will break script compat too, of course, but in a pretty visible way -- we'll throw when trying to QI to the property bag. Extensions should figure that out given enough beta time.
Comment 6 dwitte@gmail.com 2010-08-17 11:00:57 PDT
Getting this on blocker radar -- this needs to block b5 if we're going to do it. And we need to do something here, one way or the other. :)
Comment 7 dwitte@gmail.com 2010-08-18 15:17:11 PDT
Created attachment 467183 [details] [diff] [review]
part 1: make nsIChannel.contentLength 64-bit

This breaks the interface. Looking for r & sr here. FWIW it looks like there isn't any script in our tree that uses the "content-length" hash property, so no script changes required as a result of this.
Comment 8 dwitte@gmail.com 2010-08-18 15:20:03 PDT
Created attachment 467186 [details] [diff] [review]
part 2: implementors
Comment 9 dwitte@gmail.com 2010-08-18 15:20:34 PDT
Created attachment 467187 [details] [diff] [review]
part 3: consumers
Comment 10 dwitte@gmail.com 2010-08-18 15:21:35 PDT
After these three patches, there are no remaining references to the "content-length" hash property in the tree.
Comment 11 dwitte@gmail.com 2010-08-18 15:24:50 PDT
Comment on attachment 467183 [details] [diff] [review]
part 1: make nsIChannel.contentLength 64-bit

Actually, bz's on vacation, so I'll ping jst instead. ;)
Comment 12 Jason Duell [:jduell] (needinfo me) 2010-08-18 16:17:15 PDT
Comment on attachment 467186 [details] [diff] [review]
part 2: implementors

+r with changes discuss on IRC.

Still makes me nervous to get rid of ENSURE_ARG macros, but dwitte makes good case we'll never get passed null.
Comment 13 dwitte@gmail.com 2010-08-18 16:18:31 PDT
I've built a list of all the in-tree uses of nsIPropertyBag/nsIWritablePropertyBag on channels.

Now the hard part: figuring out what to do with them. ;)

"docshell.previousURI":
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10332
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10367

"docshell.internalReferrer":
http://mxr.mozilla.org/mozilla-central/source/xpinstall/src/nsInstallTrigger.cpp#149
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amContentHandler.js#73
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1760
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#2360
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9282
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8565

"docshell.newWindowTarget":
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8698
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1548

"docshell.previousFlags":
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10369
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10346

"baseURI":
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#1940
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_aboutblank.js#18
http://mxr.mozilla.org/mozilla-central/source/dom/src/jsurl/nsJSProtocolHandler.cpp#523
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#175

"channel-policy":
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#221
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#230
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#287

"content-disposition":
http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#793
http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#804
Comment 14 dwitte@gmail.com 2010-08-18 17:05:22 PDT
My interpretations so far:

"content-disposition": not used in any meaningful way. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#241 says it's the way of the future, but reality disagrees.

"channel-policy": we could add an interface and put this property on it, and have nsHttpChannel implement it. bsterne and dveditz say that'd be fine; we only really need to support it for httpchannels. Redirects from http --> non-http can just ditch the CSP info and fail.

"docshell.internalReferrer": we might be able to get equivalent info by getting the principal directly from the channel (via the notification callbacks).

"docshell.previousFlags": used for tracking redirects for history purposes.
Comment 15 Jason Duell [:jduell] (needinfo me) 2010-08-18 17:22:59 PDT
Comment on attachment 467187 [details] [diff] [review]
part 3: consumers

+r with fix to keep check for -1 in nsGnomeVFSInputStream::DoOpen.

We remove a lot of casts in this patch, which is great, unless we're adding compile-time warnings.  We have some cases of assigning 64 bit ints to 32 bit ones, which will probably want casts to avoid warnings.  Maybe grep make output before/after to compare warning count?

Otherwise looks great.  Thanks!   That was a a lot of code to chug through.
Comment 16 Jason Duell [:jduell] (needinfo me) 2010-08-18 17:23:30 PDT
Oh, and we should definitely run these patches through tryserver.
Comment 17 Jason Duell [:jduell] (needinfo me) 2010-08-18 17:26:43 PDT
> Redirects from http --> non-http can just ditch the CSP info and fail.

This doesn't mean all redirects from http to FTP would fail, does it?  That doesn't sound good.

Death to channel PropertyBagHashes!
Comment 18 Jason Duell [:jduell] (needinfo me) 2010-08-18 17:28:39 PDT
Should we split the Content-length patches into a separate bug and land once the tree is open for bizness?
Comment 19 dwitte@gmail.com 2010-08-18 17:54:49 PDT
Hmm, nevermind about "content-disposition", I missed a usage in nsURILoader. It was added in bug 474536.

It sounds like we could move that to a property on the channel.

I suppose the more useful thing to do at this point is figure out which of the properties in comment 13 only need to exist on either the parent or child but not both. Those can be left alone. The rest need to be dealt with.
Comment 20 dwitte@gmail.com 2010-08-18 17:58:03 PDT
(In reply to comment #17)
> This doesn't mean all redirects from http to FTP would fail, does it?  That
> doesn't sound good.

Only if the httpchannel had a CSP, in which case we can't redirect to FTP because the CSP info would be lost, allowing it to redirect back.

(In reply to comment #18)
> Should we split the Content-length patches into a separate bug and land once
> the tree is open for bizness?

I'll just land these here, and leave the bug open for more stuff.
Comment 21 dwitte@gmail.com 2010-08-19 09:24:47 PDT
bsterne's content policy tests (http://people.mozilla.org/~bsterne/content-security-policy/demo.cgi) pass in Fennec now that we have redirect stuff done, so changing "channel-policy" is definitely not a blocker.
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-20 17:33:48 PDT
Blocking beta5 on this. The primary reasoning here is to make this API change before the release instead of after to minimize the API changes we'll need to do. We've already changed nsIChannel, so one more change now is effectively free for us and others who depend on this.
Comment 23 dwitte@gmail.com 2010-08-21 00:32:45 PDT
Comment on attachment 467183 [details] [diff] [review]
part 1: make nsIChannel.contentLength 64-bit

http://hg.mozilla.org/projects/electrolysis/rev/8f84c33993c1
Comment 24 dwitte@gmail.com 2010-08-21 00:32:58 PDT
Comment on attachment 467186 [details] [diff] [review]
part 2: implementors

http://hg.mozilla.org/projects/electrolysis/rev/b69195520ef4
Comment 25 dwitte@gmail.com 2010-08-21 00:33:12 PDT
Comment on attachment 467187 [details] [diff] [review]
part 3: consumers

http://hg.mozilla.org/projects/electrolysis/rev/3c7ac02b55d3
Comment 26 dwitte@gmail.com 2010-08-23 21:43:51 PDT
Merged to m-c. I'll file separate bugs for the other bits as necessary.
Comment 28 Jason Duell [:jduell] (needinfo me) 2010-08-30 12:04:50 PDT
Looks like we want to back this out? 

bsmedberg wrote on dev.platform:

The IID of nsIChannel was changed between beta4 and beta5, for bugs 589292 and
536324. While I agree that these are virtuous changes, I don't think we should
be changing the IIDs of base interfaces so late in the beta cycle. It's going
to be very difficult to distinguish between crashes caused by third-party
extensions which were compiled against beta4, and crashes caused by issues
within our own code.

This change *might* be related to the topcrash bugs 591880 and 591678, but I
really can't tell.

Unless these changes were absolutely necessary for e10s (and it doesn't look to
me as if they were), I think these should be backed out and postponed until
after branching.
Comment 29 dwitte@gmail.com 2010-08-30 12:14:01 PDT
I'm not so sure we want to back this out. (The contentDisposition change isn't necessary, and we could back that out, but since we took the contentLength change we got that one for free.)

I don't have a complete understanding of what would break if we backed this out, but it worries me that people rely on the contentLength hash property and we have no easy way of remoting it. We could do a bunch of work inside nsHashPropertyBag itself, but that'd suck.

It might be that we get lucky and everything that depends on this lives in the child, but right now I'm not sure whether that's true or not.
Comment 30 Benjamin Smedberg [:bsmedberg] 2010-08-30 12:16:34 PDT
We can rely on the contentLength hash property by implementing nsIHashPropertyBag and providing that key directly, can't we? That wouldn't involve interface changes.
Comment 31 dwitte@gmail.com 2010-08-30 12:20:36 PDT
Yep, I suppose we could specialcase it. Want me to roll that patch?
Comment 32 Benjamin Smedberg [:bsmedberg] 2010-08-30 12:30:12 PDT
Yes please.
Comment 33 Mike Beltzner [:beltzner, not reading bugmail] 2010-08-30 14:11:16 PDT
Being fixed in bug 591997?
Comment 34 dwitte@gmail.com 2010-08-30 15:26:09 PDT
Not blocking. -> Firefox 5
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2010-09-01 07:26:23 PDT
> "docshell.internalReferrer": we might be able to get equivalent info by getting
> the principal directly from the channel (via the notification callbacks).

No, you can't.  The principal of the docshell is not immutable.  So if you want the principal it had when the channel was opened, you need to grab it when the channel is opened.
Comment 36 dwitte@gmail.com 2010-09-01 08:31:38 PDT
Would this make sense as a property (on nsIChannel?), i.e. is it something we're cool exposing to script? (Or can we expose the same information in a different way?)
Comment 37 Boris Zbarsky [:bz] (still a bit busy) 2010-09-01 09:25:58 PDT
We're cool exposing it to chrome script (heck, it already is!).
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2010-09-01 09:26:41 PDT
The other option is to make .referrer on nsIHttpChannel be the always-set thing and just have an mInternalReferrer member for what goes on the wire.  But then you might want people to know what goes on the wire...
Comment 39 Christian :Biesinger (don't email me, ping me on IRC) 2010-09-01 09:54:33 PDT
If people want to know what goes on the wire, shouldn't they use getRequestHeader("referer")?
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2010-09-01 11:22:21 PDT
Hmm... Probably.  But that won't do the right thing before on-modify-request, right?
Comment 41 Christian :Biesinger (don't email me, ping me on IRC) 2010-09-01 14:32:56 PDT
HttpBaseChannel::SetReferrer looks to me like it should work anytime.
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2010-09-01 17:45:08 PDT
Er... it doesn't.  It bails out in all sorts of cases, no?
Comment 43 Christian :Biesinger (don't email me, ping me on IRC) 2010-09-02 00:28:53 PDT
Yes, in the ones where nothing goes out on the wire.
Comment 44 dwitte@gmail.com 2011-02-09 17:25:10 PST
Reassigning to nobody. If anyone wants to work on this, feel free!
Comment 45 Brian Crowder 2011-02-10 08:42:15 PST
Did these happy patches simply never land?
Comment 46 Jason Duell [:jduell] (needinfo me) 2011-09-01 17:55:04 PDT
These patches just need to be relanded, but I want us to hold off until I figure out how much they conflict with those from bug 589292 and/or 215450.

renaming, since this is now much narrower than getting rid of nsHashBag entirely.
Comment 47 Jason Duell [:jduell] (needinfo me) 2011-09-12 20:33:58 PDT
This will change nsIChannel to a 64 bit content-length (only binary addons should be affected). See "Part 1" patch.
Comment 48 Honza Bambas (:mayhemer) 2011-09-13 11:14:15 PDT
-> me on Jason's request.
Comment 49 Nicholas Hurley [:nwgh][:hurley] 2012-10-02 12:30:15 PDT
Taking on Josh's request
Comment 50 Honza Bambas (:mayhemer) 2012-10-02 13:39:49 PDT
Ok, no problem.  But please ask next time, I just wanted to start with this today...  If the status is ASSIGNED, then I intend to work on this and sometimes I even already do so.  It is on my priority list too.
Comment 51 Nicholas Hurley [:nwgh][:hurley] 2012-10-05 12:36:56 PDT
Created attachment 668560 [details] [diff] [review]
part 1: make nsIChannel.contentLength 64-bit

Un-bitrotted, carrying forward r+ and sr+ since it's the same bits, just updated for modern times.
Comment 52 Nicholas Hurley [:nwgh][:hurley] 2012-10-05 12:38:09 PDT
Created attachment 668561 [details] [diff] [review]
part 2: implementors

Un-bitrotted part 2. Re-requesting review, as things have changed enough in the last year to warrant it.
Comment 53 Nicholas Hurley [:nwgh][:hurley] 2012-10-05 12:38:56 PDT
Created attachment 668562 [details] [diff] [review]
part 3: consumers

Un-bitrotted part 3. Re-requesting review for the same reason as part 2.
Comment 54 Nicholas Hurley [:nwgh][:hurley] 2012-10-05 12:39:48 PDT
The three patches are running through try right now, https://tbpl.mozilla.org/?tree=Try&rev=7662e20a8996

We'll see just how much I broke :)
Comment 55 Jason Duell [:jduell] (needinfo me) 2012-10-16 09:12:56 PDT
Comment on attachment 668561 [details] [diff] [review]
part 2: implementors

I'm delegating this review to Steve.
Comment 56 Nicholas Hurley [:nwgh][:hurley] 2012-10-16 15:38:12 PDT
Created attachment 672064 [details] [diff] [review]
part 4: test

Now, with 100% more testing! r?sworkman since jduell already delegated the reviews for this bug to him.
Comment 57 Steve Workman [:sworkman] (INACTIVE) 2012-10-16 16:39:10 PDT
Comment on attachment 668561 [details] [diff] [review]
part 2: implementors

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

Looks good r=me.
Comment 58 Steve Workman [:sworkman] (INACTIVE) 2012-10-16 16:39:52 PDT
Comment on attachment 668562 [details] [diff] [review]
part 3: consumers

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

Looks good - just a couple of minor things to look at.

::: extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp
@@ -432,5 @@
>        mBytesRemaining = info.size;
>  
>        // 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 != UINT64_MAX) {

Discussed this with you in person: do you need to keep this UINT64_MAX check? And change NS_MAX(..., INT32_MAX) to NS_MAX(...,INT64_MAX)?

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +107,2 @@
>      nsCString buf;
>      rv = NS_ReadInputStreamToString(instream, buf, len);

Looks like len in NS_ReadInputStreamToString is uin32_t. Could your check above be '> UINT32_MAX' instead of '> INT32_MAX'?

::: netwerk/base/src/nsStreamLoader.cpp
@@ +72,5 @@
>      chan->GetContentLength(&contentLength);
>      if (contentLength >= 0) {
> +      if (contentLength > UINT32_MAX) {
> +        // Too big to fit into uint32, so let's bail.
> +        // XXX we should really make mAllocated and mLength 64-bit instead.

Discussed this with you. I understand it's a bit of a rathole to fix mAllocated and mLength and out of the original scope of this bug. So, no worries.

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +170,1 @@
>    chan->GetContentLength(&aContentLength);

Nitpick: Since you're already changing this line, can you rename aContentLength to contentLength to clarify that it's declared locally?

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +558,5 @@
>      channel->GetURI(getter_AddRefs(uri));
>  
> +  int64_t contentLength = -1;
> +  if (channel)
> +    channel->GetContentLength(&contentLength);

Is it ok to aggregate these two 'if (channel)' statements?

::: uriloader/prefetch/nsPrefetchService.cpp
@@ +810,5 @@
>      if (mChannel) {
> +        int64_t size64;
> +        nsresult rv = mChannel->GetContentLength(&size64);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        *aTotalSize = int32_t(size64); // XXX - loses precision

FYI: Looks like this truncates the bytes (I know we discussed it in person, but I had a little niggling thought, so I checked on my Mac and I'm getting truncated output, not smart casting). So, maybe you want to do something like the old nsBaseChannel::GetContentLength, and return (-1) if it's more than INT32_MAX. OR just truncate like HttpBaseChannel::GetContentLength. In any case, I'll let you decide how you want to proceed - leave it, or whatever :)
Comment 59 Steve Workman [:sworkman] (INACTIVE) 2012-10-17 12:51:27 PDT
Comment on attachment 672064 [details] [diff] [review]
part 4: test

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

::: netwerk/test/unit/test_bug536324_64bit_content_length.js
@@ +6,5 @@
> +const Cr = Components.results;
> +
> +Cu.import("resource://testing-common/httpd.js");
> +
> +const CONTENT_LENGTH = "1152921504606846975";

Can you add a comment to say what this is in reference to (U)INT32_MAX? :)

@@ +26,5 @@
> +    httpServer.stop(do_test_finished);
> +  }
> +};
> +
> +function hugeContentLength(metadata, response) {

'huge' should be in caps ;)
Comment 60 Nicholas Hurley [:nwgh][:hurley] 2012-10-22 10:03:46 PDT
Created attachment 673909 [details] [diff] [review]
part 2: implementors (version for checkin)

Updated part 2 patch for checkin. Carry forward r+
Comment 61 Nicholas Hurley [:nwgh][:hurley] 2012-10-22 10:04:28 PDT
Created attachment 673910 [details] [diff] [review]
part 3: consumers (version for checkin)

Updated part 3 for checkin. Carry forward r+
Comment 62 Nicholas Hurley [:nwgh][:hurley] 2012-10-22 10:07:41 PDT
Created attachment 673911 [details] [diff] [review]
part 4: test (version for checkin)

Updated part 4 for checkin. Carry forward r+
Comment 63 Nicholas Hurley [:nwgh][:hurley] 2012-10-22 10:08:18 PDT
Try looks good with the patch updates https://tbpl.mozilla.org/?tree=Try&rev=ca10b3e98d9b

Will land once the trees open.
Comment 64 :Ms2ger (⌚ UTC+1/+2) 2012-10-22 10:21:21 PDT
Trees are open.
Comment 67 Philip Chee 2012-10-22 21:14:01 PDT
This broke Thunderbird:
/usr/bin/ccache /tools/gcc-4.5-0moz3/bin/g++ -o nsMsgQuickSearchDBView.o -c -I../../../mozilla/dist/stl_wrappers -I../../../mozilla/dist/system_wrappers -include /builds/slave/tb-c-cen-lnx/build/mozilla/config/gcc_hidden.h -DMOZ_LDAP_XPCOM -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"Linux2.6.32-220.el6\" -DOSARCH=Linux -DHAVE_MOVEMAIL  -I/builds/slave/tb-c-cen-lnx/build/mailnews/base/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  `/builds/slave/tb-c-cen-lnx/build/objdir-tb/mozilla/dist/sdk/bin/nspr-config --prefix=/builds/slave/tb-c-cen-lnx/build/objdir-tb/mozilla/dist --includedir=/builds/slave/tb-c-cen-lnx/build/objdir-tb/mozilla/dist/include/nspr --cflags` -I/builds/slave/tb-c-cen-lnx/build/objdir-tb/mozilla/dist/include/nss      -fPIC  -pedantic -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Werror=conversion-null -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -finline-limit=50 -fno-omit-frame-pointer    -DMOZILLA_CLIENT -include ../../../comm-config.h -MD -MF .deps/nsMsgQuickSearchDBView.pp /builds/slave/tb-c-cen-lnx/build/mailnews/base/src/nsMsgQuickSearchDBView.cpp
nsMsgSearchDBView.cpp
../../../../mailnews/base/util/nsMsgProtocol.cpp:681:10: error: prototype for 'nsresult nsMsgProtocol::GetContentLength(int32_t*)' does not match any in class 'nsMsgProtocol'
../../../../mailnews/base/util/nsMsgProtocol.h:57:1350: error: candidate is: virtual nsresult nsMsgProtocol::GetContentLength(int64_t*)
../../../../mailnews/base/util/nsMsgProtocol.cpp:687:10: error: prototype for 'nsresult nsMsgProtocol::SetContentLength(int32_t)' does not match any in class 'nsMsgProtocol'
../../../../mailnews/base/util/nsMsgProtocol.h:57:1451: error: candidate is: virtual nsresult nsMsgProtocol::SetContentLength(int64_t)
make[7]: *** [nsMsgProtocol.o] Error 1
Comment 68 Philip Chee 2012-10-23 02:30:38 PDT
> This broke Thunderbird:
Standard8 pushed a bustage fix:
http://hg.mozilla.org/comm-central/rev/7a6093803e43

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