536324 - Change nsIChannel to support 64-bit content-length

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

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]

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]

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

[[e-mail]]

I can take this. We should break this stuff sooner rather than later, if we're going to do it.

Comment 4

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

Attachment: 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

[[e-mail]]

Attachment: part 2: implementors

Comment 9

[[e-mail]]

Attachment: part 3: consumers

Comment 10

[[e-mail]]

After these three patches, there are no remaining references to the "content-length" hash property in the tree.

Comment 11

[[e-mail]]

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]

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

[[e-mail]]

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

[[e-mail]]

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]

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]

Oh, and we should definitely run these patches through tryserver.

Comment 17

[Jason Duell]

> 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]

Should we split the Content-length patches into a separate bug and land once the tree is open for bizness?

Comment 19

[[e-mail]]

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

[[e-mail]]

(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

[[e-mail]]

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)]

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

[[e-mail]]

Comment on attachment 467183 [details] [diff] [review]
part 1: make nsIChannel.contentLength 64-bit

http://hg.mozilla.org/projects/electrolysis/rev/8f84c33993c1

Comment 24

[[e-mail]]

Comment on attachment 467186 [details] [diff] [review]
part 2: implementors

http://hg.mozilla.org/projects/electrolysis/rev/b69195520ef4

Comment 25

[[e-mail]]

Comment on attachment 467187 [details] [diff] [review]
part 3: consumers

http://hg.mozilla.org/projects/electrolysis/rev/3c7ac02b55d3

Comment 26

[[e-mail]]

Merged to m-c. I'll file separate bugs for the other bits as necessary.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/8f84c33993c1
http://hg.mozilla.org/mozilla-central/rev/b69195520ef4
http://hg.mozilla.org/mozilla-central/rev/3c7ac02b55d3

Comment 28

[Jason Duell]

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

[[e-mail]]

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]

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

[[e-mail]]

Yep, I suppose we could specialcase it. Want me to roll that patch?

Comment 32

[Benjamin Smedberg]

Yes please.

Comment 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Being fixed in bug 591997?

Comment 34

[[e-mail]]

Not blocking. -> Firefox 5

Comment 35

[Boris Zbarsky [:bzbarsky]]

> "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

[[e-mail]]

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 [:bzbarsky]]

We're cool exposing it to chrome script (heck, it already is!).

Comment 38

[Boris Zbarsky [:bzbarsky]]

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)]

If people want to know what goes on the wire, shouldn't they use getRequestHeader("referer")?

Comment 40

[Boris Zbarsky [:bzbarsky]]

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)]

HttpBaseChannel::SetReferrer looks to me like it should work anytime.

Comment 42

[Boris Zbarsky [:bzbarsky]]

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)]

Yes, in the ones where nothing goes out on the wire.

Comment 44

[[e-mail]]

Reassigning to nobody. If anyone wants to work on this, feel free!

Comment 45

[Brian Crowder]

Did these happy patches simply never land?

Comment 46

[Jason Duell]

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]

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)]

-> me on Jason's request.

Comment 49

[u408661]

Taking on Josh's request

Comment 50

[Honza Bambas (:mayhemer)]

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

[u408661]

Attachment: 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

[u408661]

Attachment: part 2: implementors

Un-bitrotted part 2. Re-requesting review, as things have changed enough in the last year to warrant it.

Comment 53

[u408661]

Attachment: part 3: consumers

Un-bitrotted part 3. Re-requesting review for the same reason as part 2.

Comment 54

[u408661]

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]

Comment on attachment 668561 [details] [diff] [review]
part 2: implementors

I'm delegating this review to Steve.

Comment 56

[u408661]

Attachment: 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)]

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)]

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)]

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

[u408661]

Attachment: part 2: implementors (version for checkin)

Updated part 2 patch for checkin. Carry forward r+

Comment 61

[u408661]

Attachment: part 3: consumers (version for checkin)

Updated part 3 for checkin. Carry forward r+

Comment 62

[u408661]

Attachment: part 4: test (version for checkin)

Updated part 4 for checkin. Carry forward r+

Comment 63

[u408661]

Try looks good with the patch updates https://tbpl.mozilla.org/?tree=Try&rev=ca10b3e98d9b

Will land once the trees open.

Comment 64

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Trees are open.

Comment 65

[u408661]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c998bad111
https://hg.mozilla.org/integration/mozilla-inbound/rev/90d8a63e061c
https://hg.mozilla.org/integration/mozilla-inbound/rev/af7e6c8e6d76
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb51e245a4e

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2c998bad111
https://hg.mozilla.org/mozilla-central/rev/90d8a63e061c
https://hg.mozilla.org/mozilla-central/rev/af7e6c8e6d76
https://hg.mozilla.org/mozilla-central/rev/4bb51e245a4e

Comment 67

[Philip Chee]

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]

> This broke Thunderbird:
Standard8 pushed a bustage fix:
http://hg.mozilla.org/comm-central/rev/7a6093803e43

Comment 69

[Tom S [:evilpie]]

Added a note to https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers and a call-out box to https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIChannel.