Uploading File with FormData result in no data sent at all when running OOP

RESOLVED FIXED

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: gerard-majax, Assigned: jdm)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S][WebAPI:P2])

Attachments

(2 attachments, 4 obsolete attachments)

While attempting to write some image uploader for B2G, I wanted to upload an image using the FormData with File object, as explained on https://developer.mozilla.org/en-US/docs/DOM/XMLHttpRequest/FormData/Using_FormData_Objects#Sending_files_using_a_FormData_object.

The code looks like this, 'source' parameter is a File object [https://developer.mozilla.org/en-US/docs/DOM/File]:

function upload(source) {
  var url = "http://urlwheretoupload/";
  console.log("Uploading", source, "which is:", source.name, "(" + source.type + ")");

  var picture = new FormData();
  picture.append('email', "");
  picture.append('envoyer', "envoyer");
  picture.append('fichier', source);

  var xhr = new XMLHttpRequest({mozSystem: true});
  xhr.open("POST", url, true);
  xhr.onreadystatechange = function() {
    if (xhr.readyState == XMLHttpRequest.DONE) {
      if (xhr.status == 200) {
        if (xhr.responseText.match("Rappel des liens")) {
          setStatus("Uploaded successfully!");
        } else {
          setStatus("Error while uploading!");
        }
      } else {
        console.log("Err: state=" + xhr.readyState + "; status=" + xhr.status);
      }
      unlock();
    }
  };
  xhr.send(picture);
}

Using this code, and looking on the network at the POST request sent, I see no "Content-Type: multipart/form-data", nor anything related to "form-data". If I comment the line "picture.append('fichier', source);", then the POST request is correct.
If I build a Blob by hand like in the developer's page, and add it instead of 'source', then the request is correctly generated.
If I try to read the file myself with FileReader().readAsBinaryString(), then use FormData.append(), I only get about the 10 first bytes of the JPEG image.

Now, if I try to convert it to base64, I get this error:
E/GeckoConsole(12340): [JavaScript Error: "InvalidCharacterError: String contains an invalid character" {file: "app://image-uploader.gaiamobile.org/js/image-uploader.js" line: 36}]

Using the workaround described at https://developer.mozilla.org/en-US/docs/DOM/window.btoa#Unicode_Strings makes my base64 string working.
If I try with a simple text file (setup.sh from b2g), it does not work either.
Okay, I tried putting my application in the OOP blacklist, and now, upload works.
blocking-basecamp: --- → ?
Summary: Uploading File with FormData result in no data sent at all → Uploading File with FormData result in no data sent at all when running OOP
Anyone know what we do for <input type="file"> stuff in the child? Especially after we enable the sandbox we'll need to make sure this file picker lives in the parent process, not the child.
There's no file picker UI for b2g; we should be firing an activity (if that's too hard, punting).  Those can only get back blob/URIs, so we still shouldn't be dealing with file://.
blocking-basecamp: ? → +
I don't think iPhone can upload anything to this day. I don't think this blocks basecamp. Maybe kilimanjaro.
We were thinking about 3rd party apps like, say, a photo sharing app.  Isn't this how they'd want to upload photos?
As in comment 6, we should implement this using activity machinery.  However, I agree with Andreas that I wouldn't hold the release on this.
This bug isn't about <input type=file>. It's about uploading files using XHR. Please see comment 0. If we don't have that it'll basically be impossible to write apps that upload files at all.

I.e. you'll be able to write a camera app which takes pictures. But you won't be able upload those pictures to flickr.
(In reply to Jonas Sicking (:sicking) from comment #10)
> This bug isn't about <input type=file>. It's about uploading files using
> XHR. Please see comment 0. If we don't have that it'll basically be
> impossible to write apps that upload files at all.

Are you sure? The link he posted in comment 0 shows <input type="file"> as the source of the File object...
I never talked about form with <input type="file">, I posted the code triggering the issue in first and second comments, explicitely using XHR.
(In reply to Alexandre LISSY from comment #12)
> I never talked about form with <input type="file">, I posted the code
> triggering the issue in first and second comments, explicitely using XHR.

Right, I understand. The source of the File object matters though.

Where did you get that File object (the 'source' parameter) in the code you posted?
From here: https://github.com/lissyx/gaia/blob/image-uploader/apps/image-uploader/js/image-uploader.js#L247
The addImages() function is "inspired" a lot by the one available in "Share Receiver" example. The 'source' parameter you point is the "blob" variable in this function.
Josh will look into this one.
Assignee: nobody → josh
Whiteboard: [LOE:S]
Alexandre, the link in comment 14 no longer works. Is there another one you could provide?
Ok, so most of the form data is straight text, but one field is a blob returned from the device storage (see addImages; we end up calling the XHR upload code with the source parameter being an element of the files object). Time to figure out what the target.result attribute of the DeviceStorage success event is.
As I already stated:
 - only the blob show issue
 - either when I do the upload via FormData or trying to copy "by hand" by reading the file, it does not work.

In the second case, I get up to something like less than 20 bytes (on a 1.8MB picture).
Here's the problem: nsFSMultipartFormData ends up appending a RemoteInputStream representing the remote blob to the list of streams that make up the request body. When nsXMLHttpRequest::Send calls Available on the multiplex stream to figure out the total size of the upload request, RemoteInputStream::Available sees that it's being called on the main thread and returns NS_BASE_STREAM_CLOSED. This causes nsMultiplexInputStream::Available to abandon counting the rest of its streams, but nsXMLHttpRequest::Send doesn't care that Available returns non-NS_OK and takes the value of 0 as the total upload size.
I talked it over with Ben and we came up with the following plan:

It's bad that we're calling Available on the input stream on the main thread here. Technically we can make it work, but it just means that we in other cases will be doing synchronous main-thread IO.

What we should do is to change the nsIXHRSendable::GetSendInfo function to also produce a size. We then need to make the multipart encoder be able to calculate the size of the set of strings and blobs that it's concatenating together. Strings obviously by simply getting the length of the string, blobs by calling GetSize on the blob.

We also need to change the various GetRequestBody functions to also produce a size. This should be possible to do in all branches of GetRequestBody except when we're being passed an nsIInputStream. In that case we can hopefully make the XHR code simply leave the upload size as unknown (I see that we have both mUploadTotal and mUploadLengthComputable, so I suspect that's doable).

Josh, would you be able to do this? I realize that the XHR code is very messy with regards to progress events. In part because we still support old and outdated properties on the progress events. Hopefully we can rip all of that stuff out soon.
Yeah, I'll take lead.
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P2]
These patches make the image uploader work OOP for me. I'm running them through try now: https://tbpl.mozilla.org/?tree=Try&rev=e2152309248d

Jonas, I ended up calling Available for the nsIInputStream case, since that made it equivalent to the existing code. I would be interested in your thoughts on the matter.
I think we should remove the call since it can cause synchronous IO. But I'm fine with doing that as a followup.
Attachment #658966 - Flags: review?(jonas)
Attachment #658968 - Flags: review?(bent.mozilla)
The try push looks good - all the failures were due to the e10s TCPSocket patch which was also part of the mix.
Try run for e2152309248d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e2152309248d
Results (out of 135 total builds):
    exception: 8
    success: 96
    warnings: 23
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-e2152309248d
(In reply to Jonas Sicking (:sicking) from comment #26)
> I think we should remove the call since it can cause synchronous IO. But I'm
> fine with doing that as a followup.

Bah. The point of the patch is to remove the main thread Available() call! Let's not do that in a followup.
Comment on attachment 658968 [details] [diff] [review]
Part 2: Allow serializing remote input streams by passing the actor reference over the wire and retrieving the original stream in the parent.

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

::: dom/ipc/Blob.cpp
@@ +52,5 @@
>    void
> +  Serialize(InputStreamParams& aParams)
> +  {
> +    nsCOMPtr<nsIRemoteBlob> remote = do_QueryInterface(mSourceBlob);
> +    MOZ_ASSERT(remote);

Nit: Newline after here please.

@@ +55,5 @@
> +    nsCOMPtr<nsIRemoteBlob> remote = do_QueryInterface(mSourceBlob);
> +    MOZ_ASSERT(remote);
> +    BlobChild* actor =
> +      static_cast<BlobChild*>(static_cast<PBlobChild*>(remote->GetPBlob()));
> +    MOZ_ASSERT(actor, "Null actor?");

Nit: Another newline here.

@@ +58,5 @@
> +      static_cast<BlobChild*>(static_cast<PBlobChild*>(remote->GetPBlob()));
> +    MOZ_ASSERT(actor, "Null actor?");
> +    RemoteInputStreamParams params;
> +    params.remoteBlobChild() = actor;
> +    aParams = params;

Nit: 'aParams = RemoteInputStreamParams(actor)' should work.

::: ipc/glue/InputStreamUtils.cpp
@@ +17,5 @@
>  #include "nsThreadUtils.h"
> +#include "nsIDOMFile.h"
> +#include "mozilla/dom/ipc/Blob.h"
> +#include "nsXULAppAPI.h"
> +#include "nsIXULRuntime.h"

Nit: Please alphabetize these.

@@ +22,5 @@
>  
>  using namespace mozilla::ipc;
> +using mozilla::dom::ipc::Blob;
> +using mozilla::dom::ipc::Parent;
> +using mozilla::dom::ipc::Child;

Nit: Please use 'mozilla::dom::BlobChild' and 'mozilla::dom::BlobParent' here. It makes things look much nicer by hiding the messy template details.

@@ +110,5 @@
> +    // When the input stream already exists in this process, all we need to do
> +    // is retrieve the original instead of sending any data over the wire.
> +    case InputStreamParams::TRemoteInputStreamParams: {
> +      nsCOMPtr<nsIDOMBlob> domBlob;
> +      const RemoteInputStreamParams& params = aParams.get_RemoteInputStreamParams();

Nit: I think this exceeds 80 chars

@@ +112,5 @@
> +    case InputStreamParams::TRemoteInputStreamParams: {
> +      nsCOMPtr<nsIDOMBlob> domBlob;
> +      const RemoteInputStreamParams& params = aParams.get_RemoteInputStreamParams();
> +
> +      if (XRE_GetProcessType() == GeckoProcessType_Default) {

This won't work when we have nested content processes, right? I think we should add a bool parameter to the RemoteInputStreamParams saying which kind we're sending. That way we'll know what type we have without guessing.

@@ +122,5 @@
> +        MOZ_ASSERT(blob, "Null actor?");
> +        domBlob = blob->GetBlob();
> +      }
> +
> +      MOZ_ASSERT(domBlob, "Invalid blob contents");

Nit: newline after this.

@@ +125,5 @@
> +
> +      MOZ_ASSERT(domBlob, "Invalid blob contents");
> +      nsCOMPtr<nsIInputStream> stream;
> +      domBlob->GetInternalStream(getter_AddRefs(stream));
> +      MOZ_ASSERT(stream, "Unexpected null stream");

We can't assert this here. 'domBlob' is not a blob that we control, it could be any kind of nsIDOMBlob thing. Please check that GetInternalStream succeeds and handle failure without asserting.

::: netwerk/protocol/ftp/PFTPChannel.ipdl
@@ +8,5 @@
>  include protocol PNecko;
>  include InputStreamParams;
>  include URIParams;
>  
> +include protocol PBlob;

Is this needed?

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +8,5 @@
>  include protocol PNecko;
>  include InputStreamParams;
>  include URIParams;
>  
> +include protocol PBlob;

Is this needed?

::: netwerk/protocol/websocket/PWebSocket.ipdl
@@ +9,5 @@
>  include protocol PBrowser;
>  include InputStreamParams;
>  include URIParams;
>  
> +include protocol PBlob;

Is this needed?
jdm, reverse-review ping ;).  Will you be able to address the comments here?
Yes. I've been on PTO in Poland this week, hence the silence.
Comment on attachment 658966 [details] [diff] [review]
Part 1: Make nsXMLHttpRequest aware of the length of uploads.

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

r=me with that fixed!

::: content/html/content/src/nsFormSubmission.cpp
@@ +517,5 @@
>      mPostDataStream->AppendStream(fileStream);
> +
> +    nsCOMPtr<nsIDOMFile> file = do_QueryInterface(aBlob);
> +    bool gotSize = false;
> +    if (file) {

Don't QI to nsIDOMFile. You can call GetSize on nsIDOMBlob directly. And if that produces an error, simply use NS_ENSURE_SUCCESS(rv, rv) since that should never happen.

That should remove the need to ever call Available further down.
Attachment #658966 - Flags: review?(jonas) → review+
Posted patch Part 2 interdiff (obsolete) — Splinter Review
Comments addressed. The PBlob protocol inclusion in unrelated necko protols is required due to some obscure IPDL bug I couldn't really figure out.
Attachment #662276 - Flags: review?(bent.mozilla)
Jonas, your suggestion does get rid of the Available in the nsFormSubmission code. However, there's still the case in the GetRequestBody nsIInputStream specialization, and getting rid of that is going be require a lot of surgery. In particular, SetExplicitUploadStream also tries to call Available if the content length isn't available, so that would need to move into a delayed runnable unless we're actually performing a sync XHR... I would really prefer to file a followup, as this patch eliminates the vast majority of calls to Available (I'm presuming that nsIInputStream isn't the common case for xhr.send, at least) on the main thread.
Oh, yes, I'm totally fine with getting rid of that Available call in a followup. My review comment wasn't in relation to getting rid of Available calls in general, but rather just getting rid of that one.
Comment on attachment 662276 [details] [diff] [review]
Part 2 interdiff

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

::: dom/ipc/Blob.cpp
@@ +47,2 @@
>    : mMonitor("RemoteInputStream.mMonitor"), mSourceBlob(aSourceBlob)
> +  , mOrigin(aOrigin)

Nit: put the , on the previous line

::: ipc/glue/InputStreamUtils.cpp
@@ +115,2 @@
>  
> +      domBlob = params.remoteBlobParent() ?

I really think we should be more explicit here by adding a boolean or ActorFlavor or something to the RemoteInputStreamParams struct.

@@ +119,5 @@
>  
>        MOZ_ASSERT(domBlob, "Invalid blob contents");
> +
> +      // If fetching the internal stream fails, we ignore it and return a
> +      // null stream.

Nit: Please at least warn.
(In reply to ben turner [:bent] from comment #37)
> ::: ipc/glue/InputStreamUtils.cpp
> @@ +115,2 @@
> >  
> > +      domBlob = params.remoteBlobParent() ?
> 
> I really think we should be more explicit here by adding a boolean or
> ActorFlavor or something to the RemoteInputStreamParams struct.

I don't see why. Chris suggested this method, and a boolean would be extraneous information - one of the methods will return non-null, and that is data we can use to behave correctly.
(In reply to Josh Matthews [:jdm] from comment #38)
> I don't see why. Chris suggested this method, and a boolean would be
> extraneous information - one of the methods will return non-null, and that
> is data we can use to behave correctly.

Ok, I just chatted with him, I thought that calling the "wrong" version would abort, but it doesn't. This should be fine.

I'll r+ a final patch with the interdiff rolled in.
Attachment #658968 - Attachment is obsolete: true
Attachment #658968 - Flags: review?(bent.mozilla)
Attachment #658966 - Attachment is obsolete: true
Attachment #662276 - Attachment is obsolete: true
Attachment #662276 - Flags: review?(bent.mozilla)
Comment on attachment 662714 [details] [diff] [review]
Part 2: Allow serializing remote input streams by passing the actor reference over the wire and retrieving the original stream in the parent.

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

r=me with nits addressed.

::: dom/ipc/Blob.cpp
@@ +70,5 @@
> +  Deserialize(const InputStreamParams& aParams)
> +  {
> +    // See InputStreamUtils.cpp to see how deserialization of a
> +    // RemoteInputStream is special-cased.
> +    MOZ_NOT_REACHED();

Nit: Please add a message here so we get something on stderr.

@@ +448,5 @@
>        MOZ_ASSERT(mActor);
>        MOZ_ASSERT(!mInputStream);
>        MOZ_ASSERT(!mDone);
>  
> +      nsRefPtr<RemoteInputStream> stream = new RemoteInputStream(mSourceBlob, ActorFlavor);

Nit: Please wrap so that this is under 80 chars.

::: ipc/glue/InputStreamUtils.cpp
@@ +123,5 @@
> +      // null stream.
> +      nsCOMPtr<nsIInputStream> stream;
> +      nsresult rv = domBlob->GetInternalStream(getter_AddRefs(stream));
> +      if (NS_FAILED(rv) || !stream) {
> +        NS_WARNING("Couldn't obtain a valid stream from the blob; returning null");

Nit: Please make this < 80 chars (could just lose the 'returning null' clause)

::: netwerk/protocol/websocket/PWebSocket.ipdl
@@ +9,5 @@
>  include protocol PBrowser;
>  include InputStreamParams;
>  include URIParams;
>  
> +include protocol PBlob;

Nit: Can you file an IPDL bug about this so we don't forget it? And add // FIXME: Bug xxxxxx comments to these.
Attachment #662714 - Flags: review?(bent.mozilla) → review+
Try run for 9dd236862f37 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9dd236862f37
Results (out of 114 total builds):
    success: 95
    warnings: 9
    failure: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-9dd236862f37
The failures were all infrastructure related and intermittent orange. This should be ready for pushing.
Keywords: checkin-needed
Attachment #662714 - Attachment is obsolete: true
Try run for 9dd236862f37 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9dd236862f37
Results (out of 159 total builds):
    exception: 1
    success: 136
    warnings: 10
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-9dd236862f37
Even with  ms2ger's followup in http://hg.mozilla.org/integration/mozilla-inbound/rev/9f119fd7b092 build still fail for me on OpenBSD with :


content/base/src/nsXMLHttpRequest.cpp:2485:36: error: cannot initialize a paramet
er of type 'uint64_t *' (aka 'unsigned long long *') with an lvalue of type 'uint64 *' (aka 'unsigned 
long *')
  nsresult rv = aStream->Available(aContentLength);
                                   ^~~~~~~~~~~~~~


content/base/src/nsXMLHttpRequest.cpp:2543:14: error: no matching function for ca
ll to 'GetRequestBody'
content/base/src/nsXMLHttpRequest.cpp:2552:14: error: no matching function for ca
ll to 'GetRequestBody'
content/base/src/nsXMLHttpRequest.cpp:2558:14: error: no matching function for ca
ll to 'GetRequestBody'
content/base/src/nsXMLHttpRequest.cpp:2611:10: error: no matching function for ca
ll to 'GetRequestBody'
content/base/src/nsXMLHttpRequest.cpp:2616:19: error: out-of-line definition of '
GetRequestBody' does not match any declaration in 'nsXMLHttpRequest'
content/base/src/nsXMLHttpRequest.cpp:2622:12: error: no matching function for ca
ll to 'GetRequestBody'
content/base/src/nsXMLHttpRequest.cpp:2630:14: error: no matching function for ca
ll to 'GetRequestBody
content/base/src/nsXMLHttpRequest.cpp:2639:14: error: no matching function for ca
ll to 'GetRequestBody'
content/base/src/nsXMLHttpRequest.cpp:2657:14: error: no matching function for ca
ll to 'GetRequestBody'


candidates are :

/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:2424:1: note: candidate function not viable
: no known conversion from 'uint64_t *' (aka 'unsigned long long *') to 'uint64 *' (aka 'unsigned long
 *') for 3rd argument
GetRequestBody(nsIDOMDocument* aDoc, nsIInputStream** aResult, uint64* aContentLength,
^
/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:2494:1: note: candidate function not viable
: no known conversion from 'nsCOMPtr<nsIDOMDocument>' to 'nsIXHRSendable *' for 1st argument
GetRequestBody(nsIXHRSendable* aSendable, nsIInputStream** aResult, uint64_t* aContentLength,
^
/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:2501:1: note: candidate function not viable
: no known conversion from 'nsCOMPtr<nsIDOMDocument>' to 'ArrayBuffer *' (aka 'TypedArray<uint8_t, JS_
GetArrayBufferData, JS_GetObjectAsArrayBuffer, JS_NewArrayBuffer> *') for 1st argument
GetRequestBody(ArrayBuffer* aArrayBuffer, nsIInputStream** aResult, uint64_t* aContentLength,
^
/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:2522:1: note: candidate function not viable
: no known conversion from 'nsCOMPtr<nsIDOMDocument>' to 'nsIVariant *' for 1st argument
GetRequestBody(nsIVariant* aBody, nsIInputStream** aResult, uint64_t* aContentLength,
^
/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:2467:1: note: candidate function not viable
: no known conversion from 'nsCOMPtr<nsIDOMDocument>' to 'const nsAString_internal' for 1st argument
GetRequestBody(const nsAString& aString, nsIInputStream** aResult, uint64* aContentLength,
^
/src/mozilla-central/content/base/src/nsXMLHttpRequest.cpp:2479:1: note: candidate function not viable
: no known conversion from 'nsCOMPtr<nsIDOMDocument>' to 'nsIInputStream *' for 1st argument
GetRequestBody(nsIInputStream* aStream, nsIInputStream** aResult, uint64* aContentLength,


i'll try to look deeper into it unless someone beats me to it. Do i need to file a followup ?
Disregard my last comment, after removing objdir and restarting build from scratch, m-c builds fine for me with the last commit from ms2ger.
You need to log in before you can comment on or make changes to this bug.