Currently one needs to convert the data to string then pass that to .send() http://mozilla.pettay.fi/xhr_upload/xhr_file_upload_demo.html Something like this was discussed in webapps-wg mailing list at some point.

I totally agree. Unfortunately it's probably too late for 1.9.1 at this point.

I agree also, with some caveats. This behavior isn't standard right now. On WebApps-WG mailing list and F2F, we agreed that "chunk transfer" was a desirable feature, so taking nsIDOMFile parameter and "blobbing" it like how Gears API works seems to be another feature request. Any thoughts about that? That mechanism is described below: 1. Blob API (could be exposed on nsIDOMFile): http://code.google.com/apis/gears/upcoming/api_blob.html 2. Posting string data OR Blobs: http://code.google.com/apis/gears/api_httprequest.html#HttpRequest You could have ProgressEvents on individual Blob posts, naturally. Objections to this would be good to know, since some people don't like this API :-)

Well, in this case I think "File" makes more sense than "Blob". XHR should upload multipart data so that at least filename could be included. Blob seems to contain only data. Btw, a bit extended demo http://mozilla.pettay.fi/xhr_upload/xhr_file_upload_demo_with_speed.html

We've got an implementation for uploading files via XHR. Woo! A local file input stream gets built on the underlying nsIFile within the nsDOMFile argument (accessed via the new nsIDOMFileInternal interface). The stream, in turn, gets fed into the XHR's upload channel off the main thread, where the file's contents then get uploaded segments at a time. Some additional test cases were added to ensure proper file data sending/retrieval. Feedback would be greatly appreciated!

Comment on attachment 394434 [details] [diff] [review] Patch for XHR file send v1 >diff --git a/content/base/public/nsDOMFile.h b/content/base/public/nsDOMFile.h >+[scriptable, uuid(047CA6C4-52B3-46F1-8976-E198B724F72F)] >+interface nsIDOMFileInternal : nsISupports >+{ >+ readonly attribute nsIFile internalFile; >+}; Just call the attribute 'file'. >+nsDOMFile::GetInternalFile(nsIFile **aFile) >+{ >+ *aFile = nsnull; >+ >+ if (mFile) { >+ *aFile = mFile; >+ NS_ADDREF(*aFile); >+ } >+ >+ return NS_OK; >+} Hmm.. I wonder if there's a risk that content code can call this if there's some other bugs in the code. Ping me when we're in the office. >diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp ... >+ // Feed local file input stream into our upload channel >+ if (stream) { >+ postDataStream = stream; >+ charset.Truncate(); Also truncate the defaultContentType. If we can't figure out what content type the file is we should send an empty content-type. >diff --git a/content/base/test/test_XHRSendData.html b/content/base/test/test_XHRSendData.html >+var testData = "blahblahblahblahblahblahblaaaaaaaah. blah."; >+testFileTxt = createFileWithDataExt(testData, ""); //no ext should default to text/plain >+testFilePng = createFileWithDataExt(testData, ".png"); >+testFileJpg = createFileWithDataExt(testData, ".jpg"); >+testFileGif = createFileWithDataExt(testData, ".gif"); >+testFilePdf = createFileWithDataExt(testData, ".pdf"); >+testFileXml = createFileWithDataExt(testData, ".xml"); You never seem to delete the files. You should in order to not leave stuff on the testers system.

Hmm, setting an empty content-type when uploading causes our XHR object to loop indefinitely within ::Send(). I wonder why that is.... Will investigate further.

Modified the nsIDOMFileInternal interface and made sure to delete any files that get generated within the test_XHRSendData mochitest. With regards to defaultContentType, I set it to a default value of "application/octet-stream" if the MIME type of a file is undetectable. Clearing the defaultContentType value can result in an empty Content-Type, which causes undesired headers to be set in nsHttpChannel::SetUploadStream, and I'm not sure we're willing to touch that sector of the code. Not yet, at least. Thoughts, Jonas?

Comment on attachment 395144 [details] [diff] [review] Patch for XHR file send v2 >+[scriptable, uuid(047CA6C4-52B3-46F1-8976-E198B724F72F)] >+interface nsIDOMFileInternal : nsISupports >+{ >+ readonly attribute nsIFile file; >+}; Why does this have to be a scriptable interface?

Don't think it necessarily *needs* to be a scriptable interface, although it could be useful for interacting with certain APIs that only return a nsIDOMFile or nsIDOMFileList. I'd like to learn, though, what are some reasons that it shouldn't be a scriptable interface?

web page could QueryInterface nsIDOMFile to nsIDOMFileInternal?

Addition of a new upload channel interface (aptly named nsIUploadChannel2), so that nsHttpChannel may define a new SafeSetUploadStream() method. It differs from nsHttpChannel::SetUploadStream mainly in that 1) it's capable of setting an empty Content-Type header, and 2) it doesn't explicitly set the stream's request method. Also, the nsIDOMFileInternal interface now provides read *and* write access to an nsDOMFile's underlying nsIFile. To allay perhaps some of Olli's concerns, I've spoken with Johnny and Jonas, both of whom are a-ok with the chrome QI'ing an nsIDOMFile to an nsIDOMFileInternal. Biesi, I'm hoping you can take a look at nsHttpChannel (specifically SafeSetUploadStream() and SetupReplacementChannel() ) and ensure that all the relevant modifications are sound.

Comment on attachment 395258 [details] [diff] [review] Patch for XHR file send v3 r- for now per the IRC discussion

(In reply to comment #11) > Also, the nsIDOMFileInternal interface now provides read *and* write access to > an nsDOMFile's underlying nsIFile. To allay perhaps some of Olli's concerns, > I've spoken with Johnny and Jonas, both of whom are a-ok with the chrome QI'ing > an nsIDOMFile to an nsIDOMFileInternal. And you have tested (maybe even added a test) that content script cannot QI to nsIDOMFileInternal?

Modifications per biesi's requests; more descriptive method name for setting an upload stream (ExplicitSetUploadStream vs. SafeSetUploadStream), as well as the ability to specify the HTTP request method via ExplicitSetUploadStream.

Comment on attachment 395367 [details] [diff] [review] Patch for XHR file send v4 fair enough. I'd probably just have named it setUploadStream2. + * @param aStreamHasHeaders + * True if the stream already contains headers for the HTTP request. Hm, if this interface is so HTTP-specific, it should probably be called nsIHttpUploadChannel. Can you rename it to that? Also, while making this interface, the content length really ought to be a 64-bit integer, can you make it a long long? + const char *ctype = mRequestHead.PeekHeader(nsHttp::Content_Type); + const char *clen = mRequestHead.PeekHeader(nsHttp::Content_Length); + if (clen) + uploadChannel->ExplicitSetUploadStream(mUploadStream, + nsDependentCString(ctype), + atoi(clen), + nsDependentCString(mRequestHead.Method()), + mUploadStreamHasHeaders); This is not ideal for a few reasons: - if ctype is null, you crash - if clen is > 2^31, this will not do the right thing - if clen is null, you crash (I think) + //Ensure stream is set and method is valid need a space after // Hm, the semantics of a null stream object differ from SetUploadStream, I'm not sure that's such a good idea...

(In reply to comment #15) > This is not ideal for a few reasons: > - if ctype is null, you crash http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOMStrings.cpp#219 indicates otherwise. > - if clen is > 2^31, this will not do the right thing Arg, is there an atoi that can do long longs? > - if clen is null, you crash (I think) clen can't be null since there is no removeRequestHeader function. However it could have been set to a non-integer value. Do we care to support that? The old code didn't. > Hm, the semantics of a null stream object differ from SetUploadStream, I'm not > sure that's such a good idea... The semantics of the old function sort of sucked in general. Seems like the new semantics is better all around?

> Arg, is there an atoi that can do long longs? nsCRT::atoll

(In reply to comment #16) > (In reply to comment #15) > > This is not ideal for a few reasons: > > - if ctype is null, you crash > > http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOMStrings.cpp#219 > indicates otherwise. That's not the nsDependentCString implementation for internal code. see http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsTDependentString.h#87 in combination with http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsCharTraits.h#625 > > - if clen is null, you crash (I think) > > clen can't be null since there is no removeRequestHeader function. However it > could have been set to a non-integer value. Do we care to support that? The old > code didn't. Passing an empty header value to SetRequestHeader clears the header http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpHeaderArray.cpp#57 > > Hm, the semantics of a null stream object differ from SetUploadStream, I'm not > > sure that's such a good idea... > > The semantics of the old function sort of sucked in general. Seems like the new > semantics is better all around? Well, it's kinda nice to be able to clear the upload stream again?

Made changes per biesi's most recent feedback. After some GDB-related fun, I've found out that 1) a null ctype does indeed cause crashing, and 2) a null clen does NOT cause crashing. Thus, changes have been made so a null ctype within SetupReplacementChannel will no longer crash. Also, I updated nsIUploadChannel2 functionality so that it supports 64-bit values for the Content-Type header. Finally, I've added 307-redirect tests so as to ensure code coverage for some of the modifications made to SetupReplacementChannel. Biesi, I opted to leave the name nsIUploadChannel2 as is, so as to maintain association with its predecessor. Let me know if you (or anyone else, for that matter) feels strongly about this, and I'll rename the interface.

Comment on attachment 395905 [details] [diff] [review] Patch for XHR file send v5 + if (!ctype) ctype = ""; move the assignment to the next line. that way it's possible to set a breakpoint on it. + nsDependentCString(mRequestHead.Method()), please limit your lines to 80 characters. maybe: if (clen) { uploadChannel->ExplicitSetUploadStream( mUploadStream, ...); + mRequestHead.SetHeader(nsHttp::Content_Length, nsPrintfCString("%d", aContentLength)); again, 80 chars please r=biesi on the netwerk/ part

Comment on attachment 395905 [details] [diff] [review] Patch for XHR file send v5 oh, it'd also be nice to have some necko tests for this new API (see netwerk/test/unit)

The new IDL shouldn't be going into SDK_XPIDLSRCS, should it? You should be using PR_FALSE, not false, in the caller, right? Your API docs say the stream typically needs to implement ReadSegments, but file input streams don't. Are the API docs wrong? Or am I just missing something?

In particular, note bug 343971.

Oh, and GetInternalFile can be a two-liner: NS_IF_ADDREF(*aFile = mFile); return NS_OK;

Made the formatting changes suggested by biesi. Also, 1) moved nsIUploadChannel2.idl from SDK_XPIDLSRCS to XPIDLSRCS, 2) used PR_FALSE in lieu of |false|, and 3) condensed GetInternalFile(), all per bz's suggestions. Thanks for the feedback, folks. Regarding the API docs: they claim that *most* implementations of nsIUploadChannel require the stream implement readSegments, meaning some don't, so perhaps file input streams are just conveniently passed into the few interfaces that don't require readSegments? A little confusing, I know. But it seems to me that the API docs may just be imprecise, not necessarily wrong.

> But it seems to me that the API docs may just be imprecise, not necessarily > wrong. Ah, I see what's going on here. In general unbuffered upload streams are not guaranteed to be supported. The implementation details fix for bug 137155 happened to make them work in HTTP case. That's not guaranteed, and your code is passing in an unbuffered stream; it could break any time someone makes some changes to HTTP guts. So we should really be wrapping a buffered stream around the XHR stream if the XHR stream is unbuffered...

Comment on attachment 396373 [details] [diff] [review] Patch for XHR file send v6 >-for each(test in tests) { >- xhr = new XMLHttpRequest; >- xhr.open("POST", "file_XHRSendData.sjs", false); >- if (test.contentType) >- xhr.setRequestHeader("Content-Type", test.contentType); >- xhr.send(test.body); >+try { >+ for each(test in tests) { >+ xhr = new XMLHttpRequest; >+ xhr.open("POST", "file_XHRSendData.sjs", false); >+ if (test.contentType) >+ xhr.setRequestHeader("Content-Type", test.contentType); >+ xhr.send(test.body); Why do you need the try/catch here? > NS_IMETHODIMP >+nsHttpChannel::ExplicitSetUploadStream(nsIInputStream *aStream, >+ const nsACString &aContentType, >+ PRInt64 aContentLength, >+ const nsACString &aMethod, >+ PRBool aStreamHasHeaders) >+{ >+ // Ensure stream is set and method is valid >+ NS_ENSURE_TRUE(aStream, NS_ERROR_FAILURE); >+ >+ if (aContentLength < 0) { >+ PRUint32 streamLength; >+ aStream->Available(&streamLength); >+ aContentLength = streamLength; >+ if (aContentLength < 0) { >+ NS_ERROR("unable to determine content length"); >+ return NS_ERROR_FAILURE; >+ } >+ } >+ >+ nsresult rv = SetRequestMethod(aMethod); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ mRequestHead.SetHeader(nsHttp::Content_Length, nsPrintfCString("%d", aContentLength)); Don't you need this to be "%ld"? Would be good if you could make the test also test that the correct Content-Length header is set. Looks good otherwise. Do fix those things and the thing bz pointed out.

> Why do you need the try/catch here? So that the following line of code |cleanUpData()| gets called, ensuring that any files we created during the test get deleted even in the case of an exception. > Don't you need this to be "%ld"? Yes, "%d" is indeed incorrect, but shouldn't it be "%lld"? Changes in the latest (and hopefully final!) version of the patch: -printf() arguments fixed. -Tests now check for correct Content-Length headers when sending a file. -Wrapped our file input stream in a buffered input stream, per bz's request.

Wouldn't the buffered check make more sense outside the switch?

Yes, it would absolutely make more sense. Fixed, thanks bz!

Comment on attachment 396871 [details] [diff] [review] Patch for XHR file send v8 >+catch (e) { >+ alert("Exception: " + e); >+} >+cleanUpData(); Except that this will make the test hang on the alert() (since often nobody is around to click the 'ok' button), so cleanUpData won't be called :) Just remove the catch and put cleanUpData() in a 'finally' instead. r/sr=me with that fixed.

Ah yes, thank you. That is true. Alerts are bad things, and I'm not quite sure why I succumbed to their deviousness. Hopefully the last version of the patch! Again!

Comment on attachment 396908 [details] [diff] [review] Patch for XHR file send v9 Looks great. Just attach a 'hg export' patch that includes your name and checkin comment. Sorry for not pointing that out before.

Oops; there appears to be residual code in this patch that'll cause some commit troubles. Should be fixed now.

*ehrm* The previous post was a lie. This is the real patch.

It looks like this got landed: http://hg.mozilla.org/mozilla-central/rev/7d5e1bcb4729 and then backed out due to mochitest failures (although another patch was backed out right afterwards; not clear if it was known which was responsible): http://hg.mozilla.org/mozilla-central/rev/5260e85b49fc http://hg.mozilla.org/mozilla-central/rev/a8e7a19257ab Was there any progress on standardizing this idea?

This will definitely be standardized as part of XHR Level 2. Apple proposed this ages ago and no-one seemed against the idea. Not sure if they implement it yet.

So the latest version of the patch ended up causing some Cross-Site XHR issues, since request methods weren't being properly preserved in redirects without a body (e.g. for a DELETE request). It was a simple one-line fix, but it's slightly (but just slightly!) icky; the request method can be set twice if ExplicitSetUploadStream() gets called in nsHttpChannel::SetupReplacementChannel(). Shouldn't be a big deal at all, but I'd feel better getting another set of eyes on this minuscule change.

Comment on attachment 399325 [details] [diff] [review] Fix for CrossSiteXHR bug. Er, yes.

Latest version of patch incorporating cross-site XHR fix.

We have odd exceptions occurring on Windows platforms w.r.t. nsIFile.

Interestingly, content/base/test/test_bug414796.html uses the exact same mechanism for creating/removing files, but somehow doesn't throw an exception on nsIFile.Remove(). Very odd, indeed.

I guess for now don't worry about removing the files. I still like the new test code better, so it'd be great if you could attach a patch with the new code, but with a try/catch around the removals.

New version of test code, with a try/catch around the meddlesome nsIFile.remove() code. Passes Win unit tests on try.

This patch broke 307 redirects of POST requests. See bug 547239.

And as far as that goes, is there a reason this bug is still open?

No, this should definitely be marked fixed.

This patch also broke form POST when proxies are in use, or just being detected. See bug 541815, bug 547396, bug 547525, bug 548009 and https://support.mozilla.com/en-US/forum/1/570353

Documentation updated here: https://developer.mozilla.org/en/XMLHttpRequest#send()

I adjusted the docs to mention that Files can be sent starting with gecko 1.9.2