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++.

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.

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.

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

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.

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.

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. :)

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.

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

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

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

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

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

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

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

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.

(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.

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.

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 on attachment 467183 [details] [diff] [review] part 1: make nsIChannel.contentLength 64-bit http://hg.mozilla.org/projects/electrolysis/rev/8f84c33993c1

Comment on attachment 467186 [details] [diff] [review] part 2: implementors http://hg.mozilla.org/projects/electrolysis/rev/b69195520ef4

Comment on attachment 467187 [details] [diff] [review] part 3: consumers http://hg.mozilla.org/projects/electrolysis/rev/3c7ac02b55d3

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

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

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.

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.

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.

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

Yes please.

Being fixed in bug 591997?

Not blocking. -> Firefox 5

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

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

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

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

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

Hmm... Probably. But that won't do the right thing before on-modify-request, right?

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

Er... it doesn't. It bails out in all sorts of cases, no?

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

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

Did these happy patches simply never land?

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.

This will change nsIChannel to a 64 bit content-length (only binary addons should be affected). See "Part 1" patch.

-> me on Jason's request.

Taking on Josh's request

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.

Un-bitrotted, carrying forward r+ and sr+ since it's the same bits, just updated for modern times.

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

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

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 on attachment 668561 [details] [diff] [review] part 2: implementors I'm delegating this review to Steve.

Now, with 100% more testing! r?sworkman since jduell already delegated the reviews for this bug to him.

Comment on attachment 668561 [details] [diff] [review] part 2: implementors Review of attachment 668561 [details] [diff] [review]: ----------------------------------------------------------------- Looks good r=me.

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

Updated part 2 patch for checkin. Carry forward r+

Updated part 3 for checkin. Carry forward r+

Updated part 4 for checkin. Carry forward r+

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

Trees are open.

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

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

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

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

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.