491201 - It would be useful if XMLHttpRequest.send could take the data from nsIDOMFile parameter

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 2

[Arun Ranganathan]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Matin Movassate]

Attachment: Patch for XHR file send v1

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 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Matin Movassate]

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.

Comment 7

[Matin Movassate]

Attachment: Patch for XHR file send v2

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 8

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 9

[Matin Movassate]

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?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

web page could QueryInterface nsIDOMFile to nsIDOMFileInternal?

Comment 11

[Matin Movassate]

Attachment: Patch for XHR file send v3

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 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 395258 [details] [diff] [review]
Patch for XHR file send v3

r- for now per the IRC discussion

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[Matin Movassate]

Attachment: Patch for XHR file send v4

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 15

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 17

[Masatoshi Kimura [:emk]]

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

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 19

[Matin Movassate]

Attachment: Patch for XHR file send v5

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 20

[Christian :Biesinger (don't email me, ping me on IRC)]

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 21

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

In particular, note bug 343971.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, and GetInternalFile can be a two-liner:

  NS_IF_ADDREF(*aFile = mFile);
  return NS_OK;

Comment 25

[Matin Movassate]

Attachment: Patch for XHR file send v6

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 28

[Matin Movassate]

Attachment: Patch for XHR file send v7

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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

Comment 30

[Matin Movassate]

Attachment: Patch for XHR file send v8

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

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 32

[Matin Movassate]

Attachment: Patch for XHR file send v9

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 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 34

[Matin Movassate]

Attachment: Patch for XHR file send v10

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

Comment 35

[Matin Movassate]

Attachment: Patch for XHR file send v11

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

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 38

[Matin Movassate]

Attachment: Fix for CrossSiteXHR bug.

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 39

[Boris Zbarsky [:bzbarsky]]

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

Er, yes.

Comment 40

[Matin Movassate]

Attachment: Patch for XHR file send v12

Latest version of patch incorporating cross-site XHR fix.

Comment 41

[Matin Movassate]

Attachment: Candidate fix for nsIFile.Remove issue on Windows

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

Comment 42

[Matin Movassate]

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.

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 44

[Matin Movassate]

Attachment: Candidate fix for nsIFile.Remove issue on Windows, v2

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

Comment 45

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fc158dcd3472

Comment 46

[Boris Zbarsky [:bzbarsky]]

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

Comment 47

[Boris Zbarsky [:bzbarsky]]

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

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, this should definitely be marked fixed.

Comment 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[Eric Shepherd [:sheppy]]

Documentation updated here:

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

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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