536273 - e10s HTTP: get POSTs working

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

This ought to be a reasonably small and self-contained project.

The most obvious thing do start with is to start passing the request method type along in HttpChannelChild::AsyncOpen and set it in the chrome channel (i.e add a member variable, set it in SetRequestMethod, send it along in AsyncOpen, and then call SetRequestMethod on the chrome channel with it).

While doing this we should add a better xpcshell test for POSTs.  test_bug412945.js is the only existing test that uses POST, and it only sends a an empty POST.

Comment 1

[Jason Duell]

biesi pointed out to me that we need to be able to support POST-ing large files (there's a bug open to support >4 GB files, for instance).  So we need to be able to make e10s POST work so that 1) the total Content-length is sent to the parent process at the start; so that 2) the parent can start the POST before it has received all the data from the child, and continue uploading as the child sends the rest of the file data.

Comment 2

[Frederic Plourde (:fred23)]

Talked with Jduell via email... I'm going to take that bug. Should be fairly easy to do. I'll do it in two parts.
 1) make POST requests work
 2) tackle the "large file" issue... (possibly by filing a new bug)

Comment 3

[Frederic Plourde (:fred23)]

Attachment: POST_patch v.1

Very straightforward patch in accordance to what was proposed by jduell in the bug description above. 

I mark this one r? for review and will propose another patch to review later on for the ">4GB files" issue.

Comment 4

[Frederic Plourde (:fred23)]

Attachment: patch v.2

This patch makes POST requests work for this simple example : http://www.cs.unc.edu/~jbs/resources/perl/perl-cgi/programs/form1-POST.html

I haven't tested with other forms yet.

notes on this patch :
*********************

 - I have moved nsIUploadChannel support from nsHttpChannel to HttpBaseChannel because I need its original version from HttpChannelChild.

 - I assume uploadStreamData is never empty when uploadStream exists. In fact, in case uploadStream is null when calling AsyncOpen, I pass uploadStreamData="" across the process boundary to inform Content Process that uploadStream is null.

Comment 5

[[e-mail]]

Comment on attachment 439267 [details] [diff] [review]
patch v.2

jduell asked me to take a first pass at this.

Comment 6

[Jason Duell]

Comment on attachment 439267 [details] [diff] [review]
patch v.2

> class HttpChannelChild : public PHttpChannelChild
>                        , public HttpBaseChannel
>                        , public nsICachingChannel
>-                       , public nsIUploadChannel
>                        , public nsIUploadChannel2

We need to also handle nsIUploadChannel2.  Move that to HttpBaseChannel as well.  This adds some wrinkles to the patch, in that we can't just assume that we can call SetUpLoadStream in HttpChannelParent::RecvAsyncOpen, as the child might have set it with ExplicitSetUploadStream, which is subtly different.  I think the answer is going to be that we call neither function, and instead add some accessor methods that allow HttpChannelParent to set mUploadStream and mUploadStreamHasHeaders directly.

>+NS_IMETHODIMP
>+HttpBaseChannel::GetUploadStream(nsIInputStream **stream)
>+{
>+    NS_ENSURE_ARG_POINTER(stream);
>+    *stream = mUploadStream;
>+    NS_IF_ADDREF(*stream);
>+    return NS_OK;
>+}

Convert both Get/SetUploadStream to use 2-space indents, as per file's style.


>+NS_IMETHODIMP
>+HttpBaseChannel::SetUploadStream(nsIInputStream *stream,
>+                               const nsACString &contentType,
>+                               PRInt32 contentLength)
>+{
>+    // NOTE: for backwards compatibility and for compatibility with old style
>+    // plugins, |stream| may include headers, specifically Content-Type and
>+    // Content-Length headers.  in this case, |contentType| and |contentLength|
>+    // would be unspecified.  this is traditionally the case of a POST request,
>+    // and so we select POST as the request method if contentType and
>+    // contentLength are unspecified.
>+    
>+    if (stream) {
>+        if (!contentType.IsEmpty()) {
>+            if (contentLength < 0) {


Add comment here:

                  // Not really kosher to assume Available == total length of
                  // stream, but works for normal (not '/dev/zero') nonblocking
                  // streams.

It would be nice to throw in an NS_ASSERTION here that !stream->IsNonBlocking.  This code would set the wrong content-length for a non-blocking stream.

>+                stream->Available((PRUint32 *) &contentLength);
>+                if (contentLength < 0) {
>+                    NS_ERROR("unable to determine content length");
>+                    return NS_ERROR_FAILURE;
>+                }

>+            mRequestHead.SetHeader(nsHttp::Content_Length,
>+                                   nsPrintfCString("%d", contentLength));
>+            mRequestHead.SetHeader(nsHttp::Content_Type, contentType);

Both here and in ExplicitSetUploadStream(), use SetRequestHeader() instead of mRequestHead.SetHeader().  This will be slightly less efficient, but HttpChannelChild's version will automatically transport the headers to chrome for you (and will capture the order in which headers were set, in case the user fiddles with them before/after this call).


>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp
 
>+  // Prepare uploadStream for POST data

Move all of this code up to just after the NS_CheckPortSafety call.  We can't simply return an error code after we've done things like add to the load group, etc.

>+  nsCAutoString uploadStreamData;
>+  nsCAutoString contentType;
>+  PRUint32 contentLength;

Get rid of contentType/contentLength, both here and in the IPDL AsyncOpen function. 

>+  if (mUploadStream) {
>+    // Copy the upload stream

Add comment here:

      // This is a temporary measure until bug XXX is fixed:  we're doing
      // blocking read of a potentially arbitrarily large stream, so this isn't
      // performant/safe for large file uploads.

If you can ask cjones what the max size (if any) for IPDL args is, it'd be nice to fail with an error code if the uploadStreamData exceeeds that.

>+    PRUint32 bufsize;
>+    PRUint32 bytes;
>+    mUploadStream->Available(&bufsize);
>+    char* buf = (char*) nsMemory::Alloc(bufsize);
>+    mUploadStream->Read(buf, bufsize, &bytes);
>+    NS_ENSURE_TRUE(bytes == bufsize, NS_ERROR_UNEXPECTED);
>+    uploadStreamData.Assign(buf);
>+    nsMemory::Free(buf);

Any particular reason you need nsMemory::Alloc here, instead of (infallible) moz_xmalloc, or (better yet) a string of some sort?  You leak memory if NS_ENSURE_TRUE fails.


>+    // Set request headers
>+    if (!mUploadStreamHasHeaders) {
>+      nsCAutoString contentLengthStr("");
>+      mRequestHead.GetHeader(nsHttp::Content_Type, contentType);
>+      mRequestHead.GetHeader(nsHttp::Content_Length, contentLengthStr);
>+      contentLength = contentLengthStr.ToInteger(&rv, 10);
>+      NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
>+    } else {
>+      // This indicates that uploadStream contains the request headers
>+      contentType.Assign("");
>+    }
>+  }

This logic can all go away if we use SetRequestHeader() in the setter methods.

>   SendAsyncOpen(mSpec, charset, originalSpec, originalCharset, docSpec,
>                 docCharset, referrerSpec, referrerCharset, mLoadFlags,
>-                mRequestHeaders, mRequestHead.Method(), mPriority,
>-                mRedirectionLimit, mAllowPipelining,
>+                mRequestHeaders, mRequestHead.Method(), 
>+                uploadStreamData, contentType, contentLength,

instead of contentType/contentLength, send mUploadStreamHasHeaders (as a bool).

Complication:  I see that there's nothing in (Explicit)SetUploadStream that prevents one from passing in a stream with Content-length == 0 (is that even meaningful? Ask Jonas or bz).  If we can ban len==0, that'd be great, and then we'd be able to tell whether to set the upload stream in the chrome nsHttpChannel based on uploadStreamData.len > 0.   If 0-length streams are legal, then we'll need a different method.  In that case I'd recommend passing in an "int uploadStreamInfo" instead of mUploadStreamHasHeaders, and make it take one of three enum values: uploadStream_null, uploadStream_hasHeaders, and uploadStream_hasNoHeaders.

>diff --git a/netwerk/protocol/http/src/HttpChannelParent.cpp b/netwerk/protocol/http/src/HttpChannelParent.cpp

>@@ -148,6 +151,13 @@ HttpChannelParent::RecvAsyncOpen(const n
>   //  httpChan->SetNotificationCallbacks(this);
> 
>   httpChan->SetRequestMethod(nsDependentCString(requestMethod.get()));
>+
>+  nsCOMPtr<nsIInputStream> stream = nsnull;

The default constructor for nsCOMPtr sets it to null, and is more efficient.  

>+  if (uploadStreamData.Length() > 0) {
>+    NS_NewPostDataStream(getter_AddRefs(stream), false, uploadStreamData, 0);
>+  }
>+  httpChan->SetUploadStream(stream, contentType, contentLength);

We don't want to call SetUploadStream unconditionally here!  If SetUploadStream was never called in the child, we'd wind up setting the method to POST.  

Anyway, we won't call SetUploadStream in any case.  Instead check either "uploadStreamData.len > 0" (or, if 0-length streams are legal, "uploadStreamInfo != uploadStream_null"), and if true, set mUploadStream and mUploadStreamHasHeaders by adding (public but non-IPDL) accessor methods to nsHttpChannel (see SetRemoteChannel, for an example).

Comment 7

[Jason Duell]

Note: bug 423886 seems to imply that Content-length=0 needs to be supported for an uploadstream,

Comment 8

[Frederic Plourde (:fred23)]

Attachment: patch v.3

Patch corrected per Jduell's comments.

Jason: I built e10s okay with this new patch, but haven't tested the functionality... for some reason, the build was always shutting down on startup. I still post the patch here because I wanted you to be able to start reviewing it...  For now, I'll take a look at what's happenning with my build. 

thanks.

Comment 9

[Frederic Plourde (:fred23)]

OK, I've finally got a working build from tip and I verified that latest patch is still working for simple POST websites like : http://www.cs.unc.edu/~jbs/resources/perl/perl-cgi/programs/form1-POST.html

But, as Romaxa said, it's doing something weird when I press the "submit" button from www.gmail.com login : 

"Method Not Allowed
The request method ����������.��%��GET is inappropriate for the URL /accounts/ServiceLogin."

Comment 10

[Frederic Plourde (:fred23)]

what *seems* to be causing the problem with gmail submit form are the "hidden" typed input fields... Any idea?

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 444735 [details] [diff] [review]
patch v.3

I have tested this patch, and it seems to work fine...
At least my gmail started loading....

Comment 12

[Oleg Romashin (:romaxa)]

I was loading:
https://mail.google.com/mail/?ui=1&ov=1

but then gmail is fail to load with some JS errors... like JavaScript error: 
http://www.google.fi/accounts/SetSID?ui=1&view=page&name=browser&ver=1eic6yu9oa4y3, line 3: missing } in XML expression... and , line 12: is_browser_supported is not defined... 

###!!! ASSERTION: Stream is non-blocking. ContentLength might be wrong!: '!isNonBlocking', HttpBaseChannel.cpp, line 409

Comment 13

[Frederic Plourde (:fred23)]

(In reply to comment #12)
> I was loading:
> https://mail.google.com/mail/?ui=1&ov=1
> 
> but then gmail is fail to load with some JS errors... like JavaScript error: 
> http://www.google.fi/accounts/SetSID?ui=1&view=page&name=browser&ver=1eic6yu9oa4y3,
> line 3: missing } in XML expression... and , line 12: is_browser_supported is
> not defined... 

This should be investigated further. Seems like an actual bug.

> ###!!! ASSERTION: Stream is non-blocking. ContentLength might be wrong!:
> '!isNonBlocking', HttpBaseChannel.cpp, line 409

This assertion comes with my patch. It shows us that non-blocking streams are indeed used for regular POST requests (not so frequently, but this is one important case !). We may have to rework parts of the patch, but I'd need some more info on nsIUploadStream and whether or not we should be blocking on those streams or not.... Jason, any thoughts on that ?

Comment 14

[Jason Duell]

For now turn off the assertion, and see if things work without it.  m-c doesn't have the assert--I just wanted to check if we're indeed seeing non-blocking input streams here (and we are!)  

I'm still nervous about the logic of assuming total stream length from calling Available on a non-blocking stream, but hey, maybe it works for the input domain we see here.  I'd eventually like a better answer than that (bz, biesi?) but for now, let's just get things to work.

Comment 15

[Boris Zbarsky [:bzbarsky]]

What's the non-blocking stream in this case?

Comment 16

[Frederic Plourde (:fred23)]

(In reply to comment #15)
> What's the non-blocking stream in this case?

It's the nsIStringUploadStream created here : http://mxr.mozilla.org/projects-central/source/electrolysis/security/manager/ssl/src/nsNSSCallbacks.cpp#134

Comment 17

[Frederic Plourde (:fred23)]

Attachment: patch v.4

This is the same patch as last one, except it does not assert on non-blocking streams.

By the way, it *seems* to work okay. I can load gmail.com myself with it, but, as Romaxa stated in another post, there are a lot of other asserts popping up, not related to this bug. So after, like 25 "ignore asserts" clicks, I could login to gmail. So we might wanna push that now and see what could be patched for those other asserts later.

Comment 18

[Frederic Plourde (:fred23)]

Guys, I've investigated further all the asserts we're getting while loading gmail.com, and I'm worried about this one : 

NS_IMETHODIMP
nsChromeRegistryContent::GetSelectedLocale(const nsACString& aPackage,
                                           nsACString& aLocale)
{
  CONTENT_NOT_IMPLEMENTED();
}

we're still hitting this non-implemented part of ChromeRegistryContent. I'm not familiar with this code, any thoughts on that ? Thanks.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> It's the nsIStringUploadStream created here :

Ah, I see.  So the issue is that there are two subtly different properties a stream might have:

1)  Never blocks when called (in the sense of suspending the thread).
2)  Might throw NS_BASE_STREAM_WOULD_BLOCK in some cases.

IsNonBlocking tests property 1.  We care about not having property 2.  String streams have property 1 but not property 2.

Sounds like not having the assert is the right thing, unless we add another flag to nsIInputStream.

Comment 21

[Jason Duell]

Comment on attachment 445938 [details] [diff] [review]
patch v.4

I've made the changes in my review, since fred is out for a while.  Here are my
notes:

>diff --git a/netwerk/protocol/http/src/HttpBaseChannel.cpp b/netwerk/protocol/http/src/HttpBaseChannel.cpp
>+//-----------------------------------------------------------------------------
>+// HttpBaseChannel::nsIUploadChannel2
>+//-----------------------------------------------------------------------------
>+
>+NS_IMETHODIMP
>+HttpBaseChannel::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 && !aStreamHasHeaders) {
>+    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);
>+
>+  if (!aStreamHasHeaders) {
>+    mRequestHead.SetHeader(nsHttp::Content_Length,
>+      nsPrintfCString("%lld", aContentLength));
>+    mRequestHead.SetHeader(nsHttp::Content_Type, aContentType);
>+  }

We need to use SetRequestHeader instead of mRequestHead.SetHeader here for the same reasons as in SetUploadStream (it will propagate the headers to chrome for us if we're in the content process).

> 
>+typedef enum { eUploadStream_null = -1,
>+               eUploadStream_hasNoHeaders = 0,
>+               eUploadStream_hasHeaders = 1 } UploadStreamInfoType;


>+  PRUint32                          mUploadStreamHasHeaders     : 1;

Your enum has negative values, so I've changed mUploadStreamHasHeaders to a signed int32, to avoid compiler warnings and achieve higher karma.


>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp
>+
>+  if (mUploadStream) {
>+    // This is a temporary measure until bug 564553 is fixed:  we're doing
>+    // blocking read of a potentially arbitrarily large stream, so this isn't
>+    // performant/safe for large file uploads.
>+    PRUint32 bufsize;
>+    PRUint32 bytes;
>+    mUploadStream->Available(&bufsize);
>+    char* buf = static_cast<char*>(moz_xmalloc(bufsize));
>+    mUploadStream->Read(buf, bufsize, &bytes);
>+    NS_ENSURE_TRUE(bytes == bufsize, NS_ERROR_UNEXPECTED);
>+    uploadStreamData.Assign(buf);
>+    moz_free(buf);

NS_ENSURE_TRUE would still leak 'buf' here.  And it's not guaranteed by the API that Read() will return all the bytes we ask for.  Replaced with the same logic used in bug 569026 (refactored into a function in nsNetUtil.h).

>diff --git a/netwerk/protocol/http/src/HttpChannelParent.cpp b/netwerk/protocol/http/src/HttpChannelParent.cpp
>+
>+  if (uploadStreamInfo != eUploadStream_null) {
>+    nsCOMPtr<nsIInputStream> stream;
>+    NS_NewPostDataStream(getter_AddRefs(stream), false, uploadStreamData, 0);
>+    httpChan->SetUploadStream(stream);
>+    // We're casting uploadStreamInfo into PRBool here on purpose because
>+    // we know possible values are either 0 or 1. See uploadStreamInfoType.
>+    httpChan->SetUploadStreamHasHeaders((PRBool) uploadStreamInfo);
>+  }

NS_NewPostDataStream can fail.  Added error handling.

>diff --git a/netwerk/protocol/http/src/nsHttpChannel.h b/netwerk/protocol/http/src/nsHttpChannel.h
>+    void SetUploadStream(nsIInputStream *uploadStream) 
>+      { mUploadStream = uploadStream; }

You're overloading the SetUploadStream with a different return type, which causes the IDL function to be hidden.  Changed name to InternalSetUploadStream().

Comment 22

[Jason Duell]

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

Can login to Paypal, my bank.   Gmail, fidelity.com still broken, presumably for some other reason.  Amazon works in test-ipc.xul, but not fennec.  I suspect the POST/PUT logic is ok, but other factors are at work for the sites that aren't working.