Closed Bug 1434553 Opened 6 years ago Closed 6 years ago

Content-Length is wrong for multipart/form-data POST repeated request

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
59 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- verified

People

(Reporter: cy6erGn0m, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(8 files, 42 obsolete files)

part 1 - nsIInputStreamLength and nsIAsyncInputStreamLength
4.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part 3 - InputStreamLengthHelper
13.20 KB, patch
Details | Diff | Splinter Review
part 4 - SlicedInputStream exposes nsIInputStreamLength
19.13 KB, patch
Details | Diff | Splinter Review
part 5 - nsBufferedInputStream
7.45 KB, patch
Details | Diff | Splinter Review
part 6 - nsMIMEInputStream
16.08 KB, patch
Details | Diff | Splinter Review
part 7 - nsIMultiplexInputStream
16.08 KB, patch
Details | Diff | Splinter Review
part 2 - IPCBlobInputStream exposes nsIInputStreamLength
25.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part 8 - docShell and necko
80.88 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
Attached file firefox-file-upload.txt.pcapng.gz (obsolete) — 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180128191456

Steps to reproduce:

Given a simple html file upload form

```
<html>
<head>
    <title>$Title$</title>
</head>
<body>
    <form method="post" action="/do" enctype="multipart/form-data">
        <p><input type="file" name="file1"></p>
        <p><input type="submit"></p>
    </form>
</body>
</html>
```

A a small text file `upload` (41 bytes length):
```
Let me try to upload a simple text file
```

I also have a server that does handle file uploads and responds with text "OK" (tried both Jetty and tomcat and also my hand-written handler)

1. Open an html file, select what file to upload 
2. press submit
3. I see "OK"
4. Stop server (to ensure all connections are closed and no http pipelining that could affect)
5. Start server again
6. Press F5 and confirm repeat send form 



Actual results:

a server is unable to parse the repeated request (neither Jetty nor Tomcat nor hand-written)

What I see is that `Content-Length` of the request is wrong. As far as I know Content-Length for multipart file upload should count all request body parts including preamble, boundaries with suffices and all CRLF, part bodies are headers. However it seems to be too small so server fails to handle it as multipart streams ends up unexpectedly. So at first time (from actual HTML form) Content-Length is fine but when repeating sending form it is not correct.

See attachment for network capture (done via Wireshark)
Component: Untriaged → HTML: Form Submission
Product: Firefox → Core
Priority: -- → P2
I did some local testing with the above, and comparing Firefox to Safari, the Content-Length: POSTed are indeed reported differently.

We claim to be sending a Content-Length of "154", while Safari claims Content-Length: "223". I was unable to check in Chrome. 

Farre, would this be better checked by the network folks?
Flags: needinfo?(afarre)
I did some more digging and found that it goes wrong in HttpBaseChannel::ExplicitSetUploadStream if a non-positive aContentLength is given, indicating that we should get the Content-Length from the saved input stream, which goes ahead and calls nsIInputStream::Available. This is also where mozregression finds the issue with the patches introduced in Bug 1398733.

Baku, do you know what's going on? It seems like nsIMIMEInputStream doesn't know its own length.

The regression range is:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2feeea8d65dba956ba15b1c6cb80053ad1c6337&tochange=726caa0c36bac0763e33ffd6b65b306ebb3a676b
Has Regression Range: --- → yes
Flags: needinfo?(afarre) → needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch contentLength.patch (obsolete) — — Splinter Review 
The issue here is related to nsIAsyncInputStream and how content length is managed by necko/docshell.

When a DOM Blob/File object is sent to the server in a multipart form submission, we take its inputStream, and, together with other form data, we create a nsIMIMEInputStream. This stream and its size are passed to necko.

This doesn't happen when docShell uses a nsSHEntry to reload a previous request. nsSHEntry knows the post data stream but not its size. Because content-length is not passed, Necko uses nsIInputStream::Available() to retrieve the size of the post data stream. But the stream is a nsIAsyncInputStream, and ::Available() returns just what is available, not the full size.

The result is that we send a wrong content length.
This patch makes nsISHEntry able to store the post data size.
Attachment #8961058 - Flags: review?(bzbarsky)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8961058 [details] [diff] [review]
contentLength.patch

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

::: docshell/base/nsDocShell.cpp
@@ +10065,5 @@
>        OnNewURI(aURI, nullptr, newURITriggeringPrincipal, newURIPrincipalToInherit,
>                 mLoadType, true, true, true);
>  
>        nsCOMPtr<nsIInputStream> postData;
> +      int64_t postDataLength;

= -1
Comment on attachment 8961058 [details] [diff] [review]
contentLength.patch

The file size on disk can change by the time the new POST happens, and then the Content-Length will be wrong, no?

We really do need to get the right length at the point when we are grabbing the file data, so when setting the stream on the new upload channel.
Attachment #8961058 - Flags: review?(bzbarsky) → review-
So is this a regression from bug 1398733?
Blocks: 1398733
Flags: needinfo?(amarchesini)
What we could try is to add a new thing on streams that asks for the data size (even if not all the data is available yet) and make sure all the streams we use for POST implement this new API...
Alternately we could have something on nsIMIMEInputStream that walks the child streams and does the right thing, plus APIs on the non-string streams we might put in there.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> So is this a regression from bug 1398733?

No. For underlying OS file, this is a regression from bug 1353629.
But note that this issue already exists for memory blobs with a size > 1mb. In this scenario we created a IPCStream inputStream, async. Without storing the size in nsSHEntry, we were triggering exactly the same problem.
Flags: needinfo?(amarchesini)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> What we could try is to add a new thing on streams that asks for the data
> size (even if not all the data is available yet) and make sure all the
> streams we use for POST implement this new API...

This is what I was thinking. But it must be async. And that means that LoadURI must be async as well when there is a nsSHEntry involved.

I would like to add this to nsIInputStream:

interface nsIInputStream {
  ...

  // This returns NS_BASE_STREAM_WOULD_BLOCK if the stream is async. Similar to what we do for read/readSegments.
  long long fullSize();

  // Here the callback system to know the fullSize if the previous method returns NS_BASE_STREAM_WOULD_BLOCK.
  void asyncWaitForFullSize(in nsIInputStreamAsyncFullSizeCallback aCallback);
};

interface nsIInputStreamAsyncFullSizeCallback : nsISupports {
  void onFullSizeReady(in nsIInputStream aStream);
};

It _must_ support NS_BASE_STREAM_WOULD_BLOCK because for IPCBlobInputStream, the operation must run, via IPC, on the parent process. Plus, it must return an unsigned value, because a nsIPipeInputStream doesn't know the fullsize and it must return -1.

Doing this, we must implement this method in nsIMIMEInputStream as well, where we call fullSize/asyncWaitForFullSize for each substream. Similar logic for nsIMultiplexInputStream.

Note that this is going to be a big task, because we have to implement this feature for each inputStream.
I need a feedback before continue.
Flags: needinfo?(bzbarsky)
to be more precise, I want to implement this:

interface nsIInputStream {
   ...
 
   // This returns NS_BASE_STREAM_WOULD_BLOCK if the stream is async. Similar to what we do for read/readSegments.
  long long fullSize();
}

interface nsIAsyncInputStream {
   // Here the callback system to know the fullSize if the previous method returns NS_BASE_STREAM_WOULD_BLOCK.
   void asyncWaitForFullSize(in nsIInputStreamAsyncFullSizeCallback aCallback);
};
 
interface nsIInputStreamAsyncFullSizeCallback : nsISupports {
  void onFullSizeReady(in nsIInputStream aStream);
};
Per IRC discussion, if we put the new methods on a new interface, and fall back to available() if that interface is not implemented on a stream, then we don't have to touch every stream.
Flags: needinfo?(bzbarsky)
Blocks: 1353629
No longer blocks: 1398733
bz, wondering if we want to land this patch as temporary fix. I'm not planning to work on the new interface soon.
Maybe the 'real' fix can be a follow up of this temporary fix?
Flags: needinfo?(bzbarsky)
I don't know that this fix necessarily makes things better.  It fixes some cases, but makes others worse (e.g. sending a too-long Content-Length can cause problems for the server).

We should really fix this the right way, esp. since this is a regression...
Flags: needinfo?(bzbarsky)
This first patch introduces 2 new interfaces.

nsIFullSizeInputStream exposes a fullSize() method.
nsIASyncFullSizeInputStream exposes asyncFullSizeWait() method.
Attachment #8961058 - Attachment is obsolete: true
Attachment #8962300 - Flags: review?(nfroyd)
Smaug, I would like you to review this patch because you know how IPCBlobInputStream workers.
Attachment #8962301 - Flags: review?(bugs)
This is an helper class that will be used to retrieve the size from any kind of stream.
Attachment #8962302 - Flags: review?(nfroyd)
SlicedInputStream is used in Blobs and FormData. It must supports nsIFullSizeInputStream
Attachment #8962303 - Flags: review?(nfroyd)
Attached patch part 5 - nsBufferedInputStream (obsolete) — — Splinter Review 
Also nsBufferedInputStream must support nsIFullSizeInputStream
Attachment #8962304 - Flags: review?(nfroyd)
Attached patch part 6 - nsMIMEInputStream (obsolete) — — Splinter Review 

        Attachment #8962305 -
        Flags: review?(nfroyd)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 7 - nsIMultiplexInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8962306 -
        Flags: review?(nfroyd)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 8 - docShell and necko (obsolete)
        — 
          — Splinter Review
      

    


  
Here the glue part.

        Attachment #8962307 -
        Flags: review?(honzab.moz)

        Attachment #8962307 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8962307 [details] [diff] [review]
part 8 - docShell and necko

amarchesini: please provide careful and detailed description of all the parts you have submitted with explanation how they work and why.  thanks. 

I can also see that some patches introducing changes to necko are not ran through necko peer review.  please do so next time.

        Attachment #8962307 -
        Flags: review?(honzab.moz) → review-

        Attachment #8962304 -
        Flags: review-

        Attachment #8962305 -
        Flags: review-

    
    

    
  
c24
Flags: needinfo?(amarchesini)

    
    

    
  
I though that the comments in this bug were enough. Here an explanation of the issue and of the patches:

Necko uses nsIInputStream::Available() if the length of the upload stream is not passed to SetUploadStream or ExplicitSetUploadStream.

Using nsIInputStream::Available() is wrong if the stream is nsIAsyncInputStream, because ::Available() doesn't return the length of the stream, but just what is available to read. And 0 is good value (that means that ::AsyncWait() has to be used).

We pass the length in any SetUploadStream/ExplicitSetUploadStream call everywhere in DOM.

But there is one important place where we don't pass the length of the stream: when the loading of a page is done using nsISHEntry. This happens when a page is reloaded. nsISHEntry keeps a reference of the nsIInputStream, but not its length.

My first approach was to keep the length together with the stream in nsISHEntry. But if the page is posting a file, this can change and we should send the latest version of that file.

The way I want to fix this issue is to introduce two new interfaces: nsIFullSizeInputStream and nsIAsyncFullSizeInputStream (part 1). These 2 interfaces introduce FullSize() and AsyncFullSizeWait() methods, sync and async approach.

IPCBlobInputStream must implement such interfaces (part 2). Just because IPCBlobInputStream is totally async, AsyncFullSizeWait() must be called. This can happen on any thread, any process, but the underlying OS File is handled on the parent process. Asynchronously the parent sends the length back to IPCBlobInputStream.

Blobs can be sliced and merged, so, I needed to implement nsIFullSizeInputStream and nsIAsyncFullSizeInputStream also here: SlicedInputStream - part 4, nsBufferedInputStream - part 5, MIME inputStream - part 6, Multiplex InputStream - part 7.

I also introduced an helper class able to extract the length of a stream using nsIFullSizeInputStream or nsIAsyncFullSizeInputStream or ::Available(), as fallback. part 3.

This helper class (FullSizeInputStreamHelper) is finally used by necko when SetUploadStream/ExplicitSetUploadStream is called. Just because the operation is async, AsyncOpen() must wait the result of the operation.

I already have an interesting follow up: SetUploadStream/ExplicitSetUploadStream should not receive the content-length at all. Using FullSizeInputStreamHelper is enough. But I want to see these first set of patches in inbound before submitting this extra cleanup.
Flags: needinfo?(amarchesini) → needinfo?(honzab.moz)

    
    

    
  
Comment on attachment 8962300 [details] [diff] [review]
part 1 - nsIFullSizeInputStream and nsIAsyncFullSizeInputStream

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

I wrote some comments below.  I think the documentation in the IDL file should talk about why you would have this and use it vs. Available(), too; WDYT?

::: xpcom/io/nsIFullSizeInputStream.idl
@@ +6,5 @@
> +
> +interface nsIEventTarget;
> +interface nsIFullSizeCallback;
> +
> +[scriptable, uuid(452d059f-9a9c-4434-8839-e10d1405647c)]

Do all of these interfaces need to be scriptable?

@@ +7,5 @@
> +interface nsIEventTarget;
> +interface nsIFullSizeCallback;
> +
> +[scriptable, uuid(452d059f-9a9c-4434-8839-e10d1405647c)]
> +interface nsIFullSizeInputStream : nsISupports

WDYT about calling this nsIStreamLength?

@@ +9,5 @@
> +
> +[scriptable, uuid(452d059f-9a9c-4434-8839-e10d1405647c)]
> +interface nsIFullSizeInputStream : nsISupports
> +{
> +  // Returns the full size of the stream if known. Otherwise it returns -1.

To be clear, this is the size of the input stream *now* or the total size of the input stream that can be read from it in the future?  For instance, consider a buffered input stream over a file: are we returning the size of the buffered data, or are we returning the size of the file?

@@ +24,5 @@
> +[scriptable, uuid(b63f9ecf-4668-44a3-93bd-72dbc65a6125)]
> +interface nsIAsyncFullSizeInputStream : nsISupports
> +{
> +  void asyncFullSizeWait(in nsIFullSizeCallback aCallback,
> +                         in nsIEventTarget aEventTarget);

This method needs documentation.  I am unsure what aEventTarget exists for; I assume it's an event target that will run aCallback at some future point, but I'm not really sure.

I have a slight preference for calling this asyncWait; compare nsIAsyncFileMetadata/nsIFileMetadataCallback.

@@ +38,5 @@
> +    /**
> +     * Called to inform what the full size of the stream is.
> +     *
> +     * @param aStream
> +     *        The stream whose asyncFullSizeWait method was called.

No documentation for aFullSize?

@@ +41,5 @@
> +     * @param aStream
> +     *        The stream whose asyncFullSizeWait method was called.
> +     */
> +    void onFullSizeReady(in nsIAsyncFullSizeInputStream aStream,
> +                         in long long aFullSize);

Is aFullSize in this method equivalent to the return value of nsIFullSizeInputStream, i.e. it can be -1?

        Attachment #8962300 -
        Flags: review?(nfroyd)

    
    

    
  


  

        Attachment #8962300 -
        Attachment is obsolete: true

        Attachment #8962691 -
        Flags: review?(nfroyd)

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #26)

Thanks for your very good summary, now I understand what's going on.
Flags: needinfo?(honzab.moz)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #29)
> (In reply to Andrea Marchesini [:baku] from comment #26)
> 
> Thanks for your very good summary, now I understand what's going on.

Interesting fact of what I'm doing here is that probably we can get rid of:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#607-659
and fix bug 1294446.

    
  

        Attachment #8962301 -
        Attachment is obsolete: true

        Attachment #8962301 -
        Flags: review?(bugs)

    
  

        Attachment #8962302 -
        Attachment is obsolete: true

        Attachment #8962302 -
        Flags: review?(nfroyd)

    
  

        Attachment #8962303 -
        Attachment is obsolete: true

        Attachment #8962303 -
        Flags: review?(nfroyd)

    
  

        Attachment #8962304 -
        Attachment is obsolete: true

        Attachment #8962304 -
        Flags: review?(nfroyd)

    
  

        Attachment #8962305 -
        Attachment is obsolete: true

        Attachment #8962305 -
        Flags: review?(nfroyd)

    
  

        Attachment #8962306 -
        Attachment is obsolete: true

        Attachment #8962306 -
        Flags: review?(nfroyd)

    
  

        Attachment #8962307 -
        Attachment is obsolete: true

        Attachment #8962307 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8962691 [details] [diff] [review]
part 1 - nsIInputStreamLength and nsIAsyncInputStreamLength

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

::: xpcom/io/nsIInputStreamLength.idl
@@ +18,5 @@
> +  // non-blocking. If this happens, you should use
> +  // nsIAsyncInputStreamLength::asyncWait().
> +
> +  // If the stream has already been used, length() returns
> +  // NS_ERROR_NOT_AVAILABLE.

this is somewhat unclear, did you mean "has already been read from?"

@@ +35,5 @@
> +  // know its length.
> +  //
> +  // If the stream implements nsIAsyncInputStreamLength, then the caller can
> +  // use this interface to request an asynchronous notification when the stream
> +  // length becomes known (via the AsyncWait method).

can you specify what has to happen when the length is already known?  I assume we just post the event (not call the callback!) immediately to the target thread?

@@ +63,5 @@
> +     * @param aLength
> +     *        The stream length. It can be -1 if the stream doesn't know its
> +     *        length. For instance, this can happen for a pipe inputStream.
> +     */
> +    void onLengthReady(in nsIAsyncInputStreamLength aStream,

I think onInputStreamLengthReady is a bit better name

    
  

        Attachment #8962691 -
        Attachment is obsolete: true

        Attachment #8962691 -
        Flags: review?(nfroyd)

    
    

    
  
Comment on attachment 8962691 [details] [diff] [review]
part 1 - nsIInputStreamLength and nsIAsyncInputStreamLength

Canceled all the review because of the renaming of the interfaces.

        Attachment #8962691 -
        Flags: review?(nfroyd)

    
  

        Attachment #8962691 -
        Attachment is obsolete: false

    
    

    
  


  

        Attachment #8962691 -
        Attachment is obsolete: true

        Attachment #8962691 -
        Flags: review?(nfroyd)

        Attachment #8962749 -
        Flags: review?(nfroyd)

        Attachment #8962749 -
        Flags: review?(honzab.moz)

    
    

    
  


  
I'm not going to ask for a review here because there is a leak. But it's important to have this patch here to have a complete view.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 3 - InputStreamLengthHelper (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963233 -
        Flags: review?(nfroyd)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 5 - nsBufferedInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963237 -
        Flags: review?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 6 - nsMIMEInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963238 -
        Flags: review?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 7 - nsIMultiplexInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963240 -
        Flags: review?(nfroyd)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 8 - docShell and necko (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963242 -
        Flags: review?(honzab.moz)

        Attachment #8963242 -
        Flags: review?(bugs)

    
    

    
  


  
This is the 'interesting' part. I want to have nsIInputStreamLength in necko everywhere instead having places where the length of the upload stream is passed as argument.

But there are places where the inputStream is a pipe. For this scenario I want to use this new class InputStreamLengthWrapper that makes any stream a nsIInputStreamLength.

        Attachment #8963244 -
        Flags: review?(nfroyd)

    
    

    
  


  

        Attachment #8963246 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8962749 [details] [diff] [review]
part 1 - nsIInputStreamLength and nsIAsyncInputStreamLength

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

Do we want to provide a reference to the InputStreamLengthHelper.h header in the IDL file, suggesting that you use that class instead?  (Maybe that reference should be added in part 3.)

::: xpcom/io/nsIInputStreamLength.idl
@@ +18,5 @@
> +  // non-blocking. If this happens, you should use
> +  // nsIAsyncInputStreamLength::asyncWait().
> +
> +  // If the stream has already been read (read()/readSegments()/close()/seek()
> +  // methods has been called), length() returns NS_ERROR_NOT_AVAILABLE.

To be clear here: *any* reading of the stream ensures that length() is going to throw?  Not just "reading" in the sense of consuming all the data, but any read of the data, no matter how small?

@@ +21,5 @@
> +  // If the stream has already been read (read()/readSegments()/close()/seek()
> +  // methods has been called), length() returns NS_ERROR_NOT_AVAILABLE.
> +
> +  // This is not an attribute because a stream can change its length. For
> +  // instance, if the stream is a file inputStream and the OS underlying file

Nit: I think "underlying OS file" is more idiomatic here.

@@ +22,5 @@
> +  // methods has been called), length() returns NS_ERROR_NOT_AVAILABLE.
> +
> +  // This is not an attribute because a stream can change its length. For
> +  // instance, if the stream is a file inputStream and the OS underlying file
> +  // changes, its length will change as well.

Nit: "the stream's length", so as to be precise about whose length we are discussing.

        Attachment #8962749 -
        Flags: review?(nfroyd) → review+

    
    

    
  
Comment on attachment 8963234 [details] [diff] [review]
part 4 - SlicedInputStream exposes nsIInputStreamLength

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

r=me, though I'd really like the testcase noted below.

::: xpcom/io/SlicedInputStream.cpp
@@ +121,5 @@
> +uint64_t
> +SlicedInputStream::AdjustRange(uint64_t aRange)
> +{
> +  // Let's remove extra length from the end.
> +  if (aRange + mCurPos > mStart + mLength) {

We are absolutely sure that none of these operations overflow, either here or below?  I guess we lifted these from Available(), so we *should* be OK...

Given the problem noted below, should we MOZ_ASSERT(aRange != UINT64_MAX) or similar?

@@ +604,5 @@
> +  if (!mAsyncWaitLengthCallback) {
> +    return NS_OK;
> +  }
> +
> +  aLength = (int64_t)AdjustRange((uint64_t)aLength);

We should check for -1 here before adjusting the length, yes?  Can we get a test for that?

        Attachment #8963234 -
        Flags: review?(nfroyd) → review+

    
    

    
  
Comment on attachment 8963233 [details] [diff] [review]
part 3 - InputStreamLengthHelper

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

I would like to see the documentation requested below.

::: xpcom/io/InputStreamLengthHelper.h
@@ +18,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  static void
> +  GetLength(nsIInputStream* aStream,
> +            const std::function<void(int64_t aLength)>& aCallback);

Documentation for this class or function would be useful.

@@ +31,5 @@
> +  Run() override;
> +
> +  NS_IMETHOD
> +  OnInputStreamLengthReady(nsIAsyncInputStreamLength* aStream,
> +                           int64_t aLength) override;

Isn't this just NS_DECL_NSIINPUTSTREAMLENGTHCALLBACK?

::: xpcom/tests/gtest/TestInputStreamLengthHelper.cpp
@@ +13,5 @@
> +#include "Helpers.h"
> +
> +using namespace mozilla;
> +
> +TEST(TestInputStreamLengthHelper, NonLengthStream) {

Tests! <3

        Attachment #8963233 -
        Flags: review?(nfroyd) → review+

    
    

    
  
Comment on attachment 8963244 [details] [diff] [review]
part 9 - InputStreamLengthWrapper

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

I think this works, but I haven't taken a look at how this class is actually used.

::: xpcom/io/InputStreamLengthWrapper.cpp
@@ +37,5 @@
> +  : mWeakCloneableInputStream(nullptr)
> +  , mWeakIPCSerializableInputStream(nullptr)
> +  , mWeakSeekableInputStream(nullptr)
> +  , mWeakAsyncInputStream(nullptr)
> +  , mLength(aLength)

Do we want to assert that this is not -1?

@@ +307,5 @@
> +NS_IMETHODIMP
> +InputStreamLengthWrapper::Length(int64_t* aLength)
> +{
> +  NS_ENSURE_STATE(mInputStream);
> +  *aLength = mLength;

Is this correct if the underlying stream supports nsIInputStreamLength?  AFAICT, mLength is only ever set if the stream is deserialized from over IPC...oh, or the constructor that provides a length is used.   I assume the constructor is going to be used liberally in the next patch(es)?

::: xpcom/io/InputStreamLengthWrapper.h
@@ +16,5 @@
> +#include "nsIInputStreamLength.h"
> +
> +namespace mozilla {
> +
> +// A wrapper keeps an inputStream together with its length.

So this is basically just a forwarding class?  You probably want to document what the behavior is around this class's implementation of nsIInputStreamLength.

@@ +36,5 @@
> +  NS_DECL_NSIINPUTSTREAMCALLBACK
> +  NS_DECL_NSIINPUTSTREAMLENGTH
> +
> +  InputStreamLengthWrapper(already_AddRefed<nsIInputStream> aInputStream,
> +                           int64_t aLength);

Documentation should probably make clear that this length is what will be used for nsIInputStreamLength, and the stream will not be consulted.

        Attachment #8963244 -
        Flags: review?(nfroyd) → review+

    
    

    
  
Comment on attachment 8963240 [details] [diff] [review]
part 7 - nsIMultiplexInputStream

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

I think this all makes sense.

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +1207,5 @@
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +  }
> +
> +  *aLength = length.value();

MOZ_ASSERT(length.isValid()), please.

@@ +1254,5 @@
> +
> +    // If we don't need to wait, let's inform the callback immediately.
> +    if (mPendingStreams.IsEmpty() || mNegativeSize) {
> +      RefPtr<nsMultiplexInputStream> parentStream = aParentStream;
> +      int64_t length = mNegativeSize ? -1 : mLength.value();

Are we guaranteed here that mLength is necessarily valid?

@@ +1293,5 @@
> +    mPendingStreams.RemoveElement(aStream);
> +
> +    if (aLength == -1) {
> +      mNegativeSize = true;
> +    } else { 

Nit: trailing whitespace.

@@ +1310,5 @@
> +    if (!mStream) {
> +      return NS_OK;
> +    }
> +
> +    // Let's notified the parent stream.

Nit: "Let's notify"?

::: xpcom/tests/gtest/Helpers.h
@@ +93,5 @@
>    nsCOMPtr<nsIInputStreamCallback> mCallback;
>    nsCOMPtr<nsIEventTarget> mCallbackEventTarget;
>  };
>  
> +class LengthInputStream final : public nsIInputStream

Some explanation of what this class is for--even for test code--would be good.

        Attachment #8963240 -
        Flags: review?(nfroyd) → review+

    
    

    
  
Comment on attachment 8963242 [details] [diff] [review]
part 8 - docShell and necko

ok, so the docshell/form part is backing out bug 1398733.
r+ for that. Didn't look at necko stuff

We obviously need some tests here to prevent future regressions.

        Attachment #8963242 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8962749 [details] [diff] [review]
part 1 - nsIInputStreamLength and nsIAsyncInputStreamLength

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

nit: the comments should be doxygen, but up to you to leave or update

::: xpcom/io/nsIInputStreamLength.idl
@@ +15,5 @@
> +  // bytes ready to be read from the stream.
> +
> +  // It could throw NS_BASE_STREAM_WOULD_BLOCK if the inputStream is
> +  // non-blocking. If this happens, you should use
> +  // nsIAsyncInputStreamLength::asyncWait().

hmm.. somehow I don't follow when this returns {OK,-1} and when {NS_BASE_STREAM_WOULD_BLOCK}.  should be explained in a bit more detail or the implementation should be made more consistent (choose only one of the possible results) if there is actually no difference for the consumer regarding handling.

@@ +37,5 @@
> +  // If the stream implements nsIAsyncInputStreamLength, then the caller can
> +  // use this interface to request an asynchronous notification when the stream
> +  // length becomes known (via the AsyncWait method).
> +  // If the length is already known, the aCallback will be still called
> +  // asynchronously.

does the 'throw after read' applies here as well as for the sync version? (=throw NS_ERROR_NOT_AVAILABLE after Read/ReadSegments/etc was called)

        Attachment #8962749 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8963237 [details] [diff] [review]
part 5 - nsBufferedInputStream

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

::: netwerk/base/nsBufferedStreams.cpp
@@ +784,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (mAsyncInputStreamLengthCallback && aCallback) {
> +      return NS_ERROR_FAILURE;

indent

@@ +808,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsCOMPtr<nsIInputStreamLengthCallback> callback;
> +  callback.swap(mAsyncInputStreamLengthCallback);

ok, you are correctly swapping to a local var, that's a must.  

but the whole access to mAsyncInputStreamLengthCallback must be synchronized.  apparently AsyncWait() and  OnInputStreamLengthReady() can be called on different threads.

we can fix this here or in bug 1451731 I just filed for the the mAsyncWaitCallback member

        Attachment #8963237 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8963238 [details] [diff] [review]
part 6 - nsMIMEInputStream

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

::: netwerk/base/nsMIMEInputStream.cpp
@@ +408,5 @@
> +    }
> +
> +    nsCOMPtr<nsIInputStreamLengthCallback> callback = mAsyncInputStreamLengthCallback;
> +    mAsyncInputStreamLengthCallback = nullptr;
> +    return callback->OnInputStreamLengthReady(this, aLength);

same issue, will update 1451731

        Attachment #8963238 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8963240 [details] [diff] [review]
part 7 - nsIMultiplexInputStream

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

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +1179,5 @@
> +      continue;
> +    }
> +
> +    if (rv == NS_ERROR_NOT_AVAILABLE) {
> +        return rv;

indent

@@ +1289,5 @@
> +                           int64_t aLength) override
> +  {
> +    MOZ_ASSERT(mPendingStreams.Contains(aStream));
> +
> +    mPendingStreams.RemoveElement(aStream);

this is racy.  you iterate on one thread in Proceed but this can be called on a different thread while you still iterate this array.  that will likely crash.

you should probably take a reference to the lock of the given nsMultiplexInputStream and use it here to sync, that would also sync access to this class's mStream member

r- for this.

@@ +1312,5 @@
> +    }
> +
> +    // Let's notified the parent stream.
> +    RefPtr<nsMultiplexInputStream> stream;
> +    stream.swap(mStream);

I think, access to this member is not fully atomic

@@ +1326,5 @@
> +
> +  CheckedInt64 mLength;
> +  bool mNegativeSize;
> +
> +  // Protected by mutex.

what mutex?  i don't see that

@@ +1368,5 @@
> +      do_QueryInterface(mStreams[i].mStream);
> +    if (!stream) {
> +      // Let's use available as fallback.
> +      uint64_t streamAvail = 0;
> +      nsresult rv = AvailableMaybeSeek(mStreams[i], &streamAvail);

I'm just wondering if this fallback to available is a good idea, in general.  if this new API has to reliably return what is the real length of the stream, then maybe we should rather fail here.

but this is up to you, just raising a concern.

        Attachment #8963240 -
        Flags: review-

    
    

    
  
Comment on attachment 8963242 [details] [diff] [review]
part 8 - docShell and necko

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

looks good, there is just one piece of code that is not clear to me.  to leave the discussion open I r-.  if explained and clear, I'll change to r+.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1074,5 @@
> +  RefPtr<HttpBaseChannel> self = this;
> +  InputStreamLengthHelper::GetLength(aStream,
> +    [self, aStreamHasHeaders](int64_t aLength) {
> +      self->mPendingInputStreamLengthOperation = false;
> +      self->ExplicitSetUploadStreamLength(aLength >= 0 ? aLength : 0,

so, when we get -1 (unknown or failure) we pass 0 as a result to the channel?  can you explain why?

it also seems that mReqContentLength can no longer keep -1 (should we make it unsigned?)

@@ +1101,5 @@
> +  nsAutoCString value;
> +  nsresult rv = GetRequestHeader(header, value);
> +  if (NS_SUCCEEDED(rv) && !value.IsEmpty()) {
> +    return NS_OK;
> +  }

this block is totally unclear to me.  if this is a re-post and a file being uploaded has been modified since, won't this just keep the wrong size while we still upload a different size?  that would break framing.

this really needs a very good explanation why we can't change the header.

r- for that.

@@ +1103,5 @@
> +  if (NS_SUCCEEDED(rv) && !value.IsEmpty()) {
> +    return NS_OK;
> +  }
> +
> +  // SetRequestHeader propagates headers to chrome if HttpChannelChild

I think only when called before we send asyncOpen up, which is probably the case ; anyway, would be good to MOZ_ASSERT(!mWasOpened) here if you need to enforce that

@@ +1121,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +HttpBaseChannel::MaybeWaitForLengthOperation(nsIStreamListener *aListener,

MaybeWaitForUploadStreamLength() ?

@@ +1122,5 @@
>  }
>  
> +bool
> +HttpBaseChannel::MaybeWaitForLengthOperation(nsIStreamListener *aListener,
> +                                               nsISupports *aContext)

indent

@@ +1123,5 @@
>  
> +bool
> +HttpBaseChannel::MaybeWaitForLengthOperation(nsIStreamListener *aListener,
> +                                               nsISupports *aContext)
> +{

please assert thread (I believe main thread is the thread this is expected to be called on?)

@@ +1135,5 @@
> +}
> +
> +void
> +HttpBaseChannel::MaybeResumeAsyncOpen()
> +{

please assert thread (I believe main thread is the thread this is expected to be called on?)

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +489,5 @@
>    CheckRedirectLimit(uint32_t aRedirectFlags) const;
>  
> +  bool
> +  MaybeWaitForLengthOperation(nsIStreamListener *aListener,
> +                                nsISupports *aContext);

indent

@@ +746,5 @@
> +
> +  nsCOMPtr<nsIStreamListener> mPendingAsyncOpenListener;
> +  nsCOMPtr<nsISupports> mPendingAsyncOpenContext;
> +
> +  bool mPendingInputStreamLengthOperation;

all these members need comments

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +569,2 @@
>  
> +    // Let's resolve the size of the stream.

please add a note that InputStreamLengthHelper::GetLength goes always async

        Attachment #8963242 -
        Flags: review?(honzab.moz) → review-

    
    

    
  
Comment on attachment 8963246 [details] [diff] [review]
part A - no contentLength param in explicit/setUploadStream

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

there are dom changes I can't review

but more generally, unless you have a very strong need to remove that argument, I'm against this patch.  when the size of the stream is known and could be simply set, with this patch you force to do a main thread loop (post and wait for result) on any upload stream.  this is something I'm strongly against from the performance point of view.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ -1067,5 @@
>    mUploadStream = aStream;
>  
> -  if (aContentLength >= 0) {
> -    return ExplicitSetUploadStreamLength(aContentLength, aStreamHasHeaders);
> -  }

hmmm... so when the size is know and could be set immediately we rather do a main thread loop to get it from the stream?

that really doesn't make me happy :(

        Attachment #8963246 -
        Flags: review?(honzab.moz) → feedback-
Depends on: 1451731

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 5 - nsBufferedInputStream (obsolete)
        — 
          — Splinter Review
      

    


  
Same patch built on top of 1451731.

        Attachment #8963237 -
        Attachment is obsolete: true

        Attachment #8966150 -
        Flags: review?(honzab.moz)

    
    

    
  
> I'm just wondering if this fallback to available is a good idea, in general.
> if this new API has to reliably return what is the real length of the
> stream, then maybe we should rather fail here.

This is kind of needed because not all the streams implement nsIAsyncInputStreamLength. For instance string and file streams do not implement that interface.

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #56)
> > I'm just wondering if this fallback to available is a good idea, in general.
> > if this new API has to reliably return what is the real length of the
> > stream, then maybe we should rather fail here.
> 
> This is kind of needed because not all the streams implement
> nsIAsyncInputStreamLength. For instance string and file streams do not
> implement that interface.

Makes sense.  Should we file followups for making them implement those interfaces or do you think the available() fallback is fine here?  It probably is because your new API provides the total-length information only while we have not read from the stream yet.  So for static-length streams (string/file) the result is identical for available() and your new API.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 7 - nsIMultiplexInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963240 -
        Attachment is obsolete: true

        Attachment #8966188 -
        Flags: review?(honzab.moz)

    
    

    
  
> so, when we get -1 (unknown or failure) we pass 0 as a result to the
> channel?  can you explain why?

We must set Content-Length somehow. Before, if ::Available() failed, we returned NS_ERROR_FAILURE.
Here, I forced the stream to be 0. But maybe I can abort the channel to have a similar behavior.

> it also seems that mReqContentLength can no longer keep -1 (should we make
> it unsigned?)

Already done.

> @@ +1101,5 @@
> > +  nsAutoCString value;
> > +  nsresult rv = GetRequestHeader(header, value);
> > +  if (NS_SUCCEEDED(rv) && !value.IsEmpty()) {
> > +    return NS_OK;
> > +  }

I don't remember exactly if it was a test or it was a chunk of code somewhere else (I need to comment out this block and run all to try again to tell you), but there was something like:

channel->SetUploadStream(a_Stream);
channel->SetRequestHeader("Content-Length", X);

Now, with my code, the callback is executed after SetRequestHeader, and it overwrites the X value. In order to avoid this, I added a check. I'll tell you more when I have the first try results.
Flags: needinfo?(honzab.moz)

    
    

    
  
Comment on attachment 8966150 [details] [diff] [review]
part 5 - nsBufferedInputStream

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

thanks

::: netwerk/base/nsBufferedStreams.cpp
@@ +798,5 @@
> +  {
> +    MutexAutoLock lock(mMutex);
> +
> +    if (mAsyncInputStreamLengthCallback && aCallback) {
> +        return NS_ERROR_FAILURE;

indention

        Attachment #8966150 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8966188 [details] [diff] [review]
part 7 - nsIMultiplexInputStream

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

LGTM, thanks

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +1322,5 @@
> +
> +    // Already notified.
> +    if (mStreamNotified) {
> +      return NS_OK;
> +    }

can this be moved higher?

@@ +1351,5 @@
> +nsMultiplexInputStream::AsyncWait(nsIInputStreamLengthCallback* aCallback,
> +                                  nsIEventTarget* aEventTarget)
> +{
> +  if (NS_WARN_IF(!aEventTarget)) {
> +    return NS_ERROR_FAILURE;

nit: I think NS_ERROR_NULL_POINTER would be better here?

::: xpcom/tests/gtest/Helpers.h
@@ +111,5 @@
> +  LengthInputStream(const nsACString& aBuffer,
> +                      bool aIsInputStreamLength,
> +                      bool aIsAsyncInputStreamLength,
> +                      nsresult aLengthRv = NS_OK,
> +                      bool aNegativeValue = false)

indention

        Attachment #8966188 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #59)
> > so, when we get -1 (unknown or failure) we pass 0 as a result to the
> > channel?  can you explain why?
> 
> We must set Content-Length somehow. Before, if ::Available() failed, we
> returned NS_ERROR_FAILURE.
> Here, I forced the stream to be 0. But maybe I can abort the channel to have
> a similar behavior.

I think the previous behavior was better.  I'd rather not send C-L: 0 and then potentially send some data (unpredictably, from a stream, potentially multiplexed, in a some weird state) after the request.  That could break framing on h1.1 conns.

> > @@ +1101,5 @@
> > > +  nsAutoCString value;
> > > +  nsresult rv = GetRequestHeader(header, value);
> > > +  if (NS_SUCCEEDED(rv) && !value.IsEmpty()) {
> > > +    return NS_OK;
> > > +  }
> 
> I don't remember exactly if it was a test or it was a chunk of code
> somewhere else (I need to comment out this block and run all to try again to
> tell you), but there was something like:
> 
> channel->SetUploadStream(a_Stream);
> channel->SetRequestHeader("Content-Length", X);
> 
> Now, with my code, the callback is executed after SetRequestHeader, and it
> overwrites the X value. In order to avoid this, I added a check. I'll tell
> you more when I have the first try results.

Aha.  OK, this is somewhat important, so let's what the reason is.

Anyway, this again shows that writing comments is a good thing to do ;)
Flags: needinfo?(honzab.moz)

    
  
Depends on: 1453015

    
    

    
  


  

        Attachment #8962749 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 3 - InputStreamLengthHelper (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963233 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #8963234 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 5 - nsBufferedInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8966150 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 6 - nsMIMEInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8963238 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 7 - nsIMultiplexInputStream (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  


  

        Attachment #8963244 -
        Attachment is obsolete: true

    
    

    
  


  
This bit is needed to make ServiceWorker tests to pass.
Note that I already made the change of supporting AsyncWait(callback1) + AsyncWait(callback2). There is also a gtest for it.

As discussed, I'm going to change all the other InputStreams in a separate bug.

        Attachment #8963246 -
        Attachment is obsolete: true

        Attachment #8966897 -
        Flags: review?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part B - docShell and necko (obsolete)
        — 
          — Splinter Review
      

    


  
Yes, I really would like to get rid of this extra parameter. Now that we have nsIInputStreamLength and nsIAsyncInputStreamLength, there are no needs to pass the stream length.

But I agree with you about the performance impact of the previous patch. For this reason, here, I introduce InputStreamLengthHelper::GetSyncLength() and renamed the previous method to GetAsyncLength().

The dom/docShell parts have been already reviewed by smaug.

        Attachment #8966899 -
        Flags: review?(honzab.moz)

    
    

    
  


  
This method is not used anymore. We can remove it.
But I want the same feature in InputStreamLengthHelper because I don't want to call ::Available() on the main-thread with blocking inputStream.

Now InputStreamLengthHelper returns false in GetSyncLength() if, calling ::Available() on main-thread, would run I/O operations. For GetAsyncLength(), AvailableEvent runnable is used as it was in nsStreamTransportService.

        Attachment #8966905 -
        Flags: review?(honzab.moz)

    
  

        Attachment #8963242 -
        Attachment is obsolete: true

    
    

    
  


  
Here a gtest for nsMIMEInputStream. Note that this is needed for 1453340. If you want to review (and land) bug 1453340 before this, I can move the gtest there.
Or I can file a separate bug just for these gtests.

        Attachment #8967013 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8966897 [details] [diff] [review]
part A - PartiallySeekableInputStream

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

::: netwerk/base/PartiallySeekableInputStream.cpp
@@ +413,5 @@
> +
> +    callback.swap(mAsyncInputStreamLengthCallback);
> +  }
> +
> +  MOZ_ASSERT(callback);

I think this is not necessary?

        Attachment #8966897 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8966899 [details] [diff] [review]
part B - docShell and necko

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

I reviewed only necko parts and the general helpers.  dom parts may need another peer review?  (not sure why those are part of this patch and not separated)

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2550,1 @@
>                                                    false);

another instance (see later comments)

::: netwerk/protocol/file/nsFileChannel.cpp
@@ +492,5 @@
>  
>    if ((mUploadStream = stream)) {
> +    // Make sure we know how much data we are uploading.
> +    uint64_t avail;
> +    nsresult rv = mUploadStream->Available(&avail);

I don't remember how this upload stream works in file channel, but doesn't this do an unnecessary I/O?  yet another instance...

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3624,5 @@
> +          RefPtr<InputStreamLengthWrapper> wrapper =
> +            new InputStreamLengthWrapper(uploadStream.forget(),
> +                                         nsCRT::atoll(clen.get()));
> +          uploadStream = wrapper;
> +        }

(didn't catch you on IRC)

it's not uncommon to have a long chain of redirects even for POST (sites are setting auth cookies that way.)  your patch will cause a wrapper over a wrapper over a wrapper etc just to carry this way an information that can be passed easily as an argument.

this unnecessary wrapping is the reason I don't like this patch in this form.

I think a good compromise would be to rather have two signatures of the (explicit-)set-upload-stream method: one as you propose here not taking the argument and doing the Length()/AsyncLength() call on the stream, and another one that still takes the argument (actually what we have now) when the length is known/unchanged to not force wrapping the stream this way.

other possibility could be to change the API specification for Length()/AsyncLength() to return always the same value, independent of the state of the stream - but that could become really hard to implement for all streams we have, so I may understand why you fail Length()/AsyncLength() after reading the stream and not renewing the length value even after seek(0).

also note that what we do in http transaction is that we just merge the request head and the upload stream (body) in a multiplexed stream and then send whatever amount the streams give us, actually sending what Available() gives at the moment.  if C-L and length of the stream are out of sync, we screw - that's a long standing possible race, no need to deal with it here.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +567,5 @@
> +    int64_t length;
> +    if (InputStreamLengthHelper::GetSyncLength(stream, &length)) {
> +      httpChannel->InternalSetUploadStreamLength(length >= 0 ? length : 0);
> +    } else {
> +      // Wait for the nputStreamLengthHelper::GetAsyncLength callback.

typo?

        Attachment #8966899 -
        Flags: review?(honzab.moz) → review-

    
    

    
  
Comment on attachment 8966905 [details] [diff] [review]
part C - Get rid of nsIStreamTransportService::InputAvailable

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

::: xpcom/io/InputStreamLengthHelper.cpp
@@ +24,5 @@
> +    , mStream(stream)
> +    , mCallback(aCallback)
> +    , mSize(-1)
> +  {
> +    mCallbackTarget = GetCurrentThreadSerialEventTarget();

you may want to assert NS_IsMainThread() here, otherwise when mCallbackTarget ends up being a non-main thread, you will indefinitely loop your Run()

@@ +32,5 @@
> +  Run() override
> +  {
> +    // ping
> +    if (!NS_IsMainThread()) {
> +      MOZ_ASSERT(!NS_IsMainThread());

not necessary?

        Attachment #8966905 -
        Flags: review?(honzab.moz) → review+

        Attachment #8967013 -
        Flags: review?(honzab.moz) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 8 - docShell and necko (obsolete)
        — 
          — Splinter Review
      

    


  
Here part 8. I marked it obsolete by mistake.

        Attachment #8967128 -
        Flags: review?(honzab.moz)

    
    

    
  


  
This patch is needed because otherwise FetchUtils is not able to call ReadSegment(). I don't want to wrap it with a nsBufferedInputStream just in FetchUtils, because maybe there are other places where we need this method.

        Attachment #8967148 -
        Flags: review?(nfroyd)

    
    

    
  


  
I'm not particularly happy of nsIFileInputStream inheriting nsIInputStream. Would be much nicer if nsIFileInputStream inherits directly from nsISupports as many other stream interfaces do.

So far, there are no issues because there is only 1 implementation of nsIFileInputStream. Maybe, we can file a bug to make to nsIFileInputStream inherit nsISupports as follow up.

        Attachment #8967291 -
        Flags: review?(nfroyd)

    
    

    
  


  

        Attachment #8963231 -
        Attachment is obsolete: true

        Attachment #8967312 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8967148 [details] [diff] [review]
part E - InputStreamLengthWrapper::ReadSegments

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

I think this change in isolation makes sense.

::: xpcom/io/InputStreamLengthWrapper.cpp
@@ +118,5 @@
>  }
>  
> +namespace {
> +
> +class MOZ_STACK_CLASS ReadSegmentsState final

There are probably a billion places where we can use a class like this.

@@ +134,5 @@
> +  void* mClosure;
> +};
> +
> +nsresult
> +ReadSegmentsCallback(nsIInputStream* aIn, void* aClosure,

Maybe make this a static method of ReadSegmentsState, as Callback(...)?  Just so if we did want to move this to a common header or something, people wouldn't have to keep writing the callback.

@@ +156,5 @@
> +  ReadSegmentsState state(this, aWriter, aClosure);
> +  nsresult rv = mInputStream->ReadSegments(ReadSegmentsCallback, &state, aCount,
> +                                           aResult);
> +  if (NS_SUCCEEDED(rv) && *aResult != 0) {
> +    mConsumed = true;

I think it'd be good to add some sort of documentation on `mConsumed`.  I was initially confused here that all we wanted was some data read, and we consider the stream "consumed", but apparently this is consistent with `Read()`.

Actually, judging by attachment 8966896 [details] [diff] [review], `mConsumed` is basically write-only...so do we need it at all?

        Attachment #8967148 -
        Flags: review?(nfroyd) → review+

    
    

    
  
Comment on attachment 8967291 [details] [diff] [review]
part F - InputStreamWrapper must expose nsIMultiplexInputStream, nsIStreamBufferAccess, and nsIFileInputStream interfaces to make webExtension upload happy.

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

I can't tell whether having InputStreamLengthWrapper implement all these sub-interfaces is a good thing.  I suppose the alternative, where you had:

template<typename StreamInterface>
class Wrapper
{
  // Do the base functionality of InputStreamLengthWrapper.

  // Expose methods for StreamInterface, probably through some traits class.
};

would probably fail, because there'd be something that expects multiple interfaces to be implemented by the wrapper.  You could extend Wrapper to be templated on multiple interfaces (and therefore magically implement them).  But you can't predict all the possible combinations that you're going to need, right?  And so you might as well implement everything?

r+, I guess, but I'm very skeptical of those MOZ_CRASHes, and I'd like to understand those before this bug is landed.

::: xpcom/io/InputStreamLengthWrapper.cpp
@@ +117,5 @@
>    }
> +
> +  nsCOMPtr<nsIMultiplexInputStream> multiplexInputStream =
> +    do_QueryInterface(mInputStream);
> +  if (multiplexInputStream &&

There's so much goo here (and I think we use this other pattern of holding weak pointers in other classes, too), perhaps we should come up with some common function/object for this?  Followup bug would be OK, I think; this bug is getting a little unwieldy.

@@ +232,5 @@
>    }
>  
>    nsCOMPtr<nsIInputStream> stream =
> +    static_cast<nsIAsyncInputStream*>(
> +      new InputStreamLengthWrapper(clonedStream.forget(), mLength));

Maybe this (and following .forget()) would be better as:

RefPtr<InputStreamLengthWrapper> stream = new ...;
*aResult = stream.forget().downcast<nsIAsyncInputStream>().take();

?  Not sure if that's really any better, but at least we wouldn't have to QI in there?

@@ +416,5 @@
> +NS_IMETHODIMP
> +InputStreamLengthWrapper::GetCount(uint32_t* aCount)
> +{
> +  NS_ENSURE_STATE(mWeakMultiplexInputStream);
> +  return mWeakMultiplexInputStream->GetCount(aCount);

It's really too bad that the NS_FORWARD_* macros can't be used for this sort of thing.

@@ +423,5 @@
> +NS_IMETHODIMP
> +InputStreamLengthWrapper::AppendStream(nsIInputStream* aStream)
> +{
> +  NS_ENSURE_STATE(mWeakMultiplexInputStream);
> +  MOZ_CRASH("nsIMultiplexInputStream::AppendStream is not supported because it would change the length.");

I feel like these MOZ_CRASHes are going to bite us at the worst times.  Is returning failure for these not an option, or were you just trying to root out problems with your patch set, or something else?

@@ +449,5 @@
> +
> +void
> +InputStreamLengthWrapper::PutBuffer(char* aBuffer, uint32_t aLength)
> +{
> +  MOZ_CRASH("nsIStreamBufferAccess::PutBuffer is not supported because it would change the length.");

It's unfortunate that this method is treated as infallible. =/

        Attachment #8967291 -
        Flags: review?(nfroyd) → review+

    
    

    
  


  
I hope to have covered all the possible scenarios with gtests.
It's also green on try, if we have enough tests for POST redirects.

        Attachment #8967434 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8967434 [details] [diff] [review]
part G - Make streams able to return their lengths after Seek(0)

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

::: netwerk/base/PartiallySeekableInputStream.cpp
@@ +392,5 @@
> +    MutexAutoLock lock(mMutex);
> +    if (mCachedLengthSet) {
> +      *aLength = mCachedLength;
> +      return NS_OK;
> +    }

why locked here...

@@ +404,5 @@
> +
> +  // Let's cache the length.
> +  {
> +    mCachedLength = *aLength;
> +    mCachedLengthSet = true;

...and not here?

if you make mCachedLengthSet atomic release/acquire (and follow the semantic), then I think you can go w/o a lock at all - but this is NOT a requirement, just a thought.

@@ +461,5 @@
> +  }
> +
> +  if (cachedLengthSet && callback) {
> +    RefPtr<AsyncWaitLengthRunnable> r =
> +      new AsyncWaitLengthRunnable(this, cachedLength);

isn't it better to notify the callback directly?  I would not care about the AsyncWait(null) call in between, if that was the reason you've done it this way.

@@ +462,5 @@
> +
> +  if (cachedLengthSet && callback) {
> +    RefPtr<AsyncWaitLengthRunnable> r =
> +      new AsyncWaitLengthRunnable(this, cachedLength);
> +    return aEventTarget->Dispatch(r.forget(), NS_DISPATCH_NORMAL);

aEventTarget can be legally null.

::: netwerk/test/gtest/TestBufferedInputStream.cpp
@@ +183,5 @@
> +  nsCOMPtr<nsISeekableStream> seekableStream = do_QueryInterface(bis);
> +  MOZ_ASSERT(seekableStream);
> +  ASSERT_EQ(NS_OK, seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, 0));
> +
> +  rv = qi->Length(&size);

can we change the underlying stream size to check it has been reflected here?

::: xpcom/io/SlicedInputStream.cpp
@@ +503,5 @@
> +      // If we rewind the stream, we want to rewind the underlying stream too.
> +      offset = aOffset;
> +      if (aOffset != 0) {
> +        offset += mStart;
> +      }

if mStart > 0 and aOffset == 0, won't we then fail on https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/xpcom/io/SlicedInputStream.cpp#484 ?

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +552,5 @@
>            if (NS_WARN_IF(NS_FAILED(rv))) {
>              return rv;
>            }
> +
> +          if (i == mCurrentStream) {

did you want `i == oldCurrentStream` ?

@@ +559,2 @@
>            continue;
>          } else {

nit: when you are here, I think you can remove this else (it's similar to 'else after return')

::: xpcom/tests/gtest/Helpers.h
@@ +96,5 @@
>    nsCOMPtr<nsIEventTarget> mCallbackEventTarget;
>  };
>  
>  // This class implements a simple nsIInputStreamLength stream.
>  class LengthInputStream : public nsIInputStream

nit: can be made final again?

::: xpcom/tests/gtest/TestMultiplexInputStream.cpp
@@ +592,5 @@
> +  ASSERT_EQ(NS_OK, seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, 0));
> +
> +  rv = qi->Length(&size);
> +  ASSERT_EQ(NS_OK, rv);
> +  ASSERT_EQ(buf.Length(), size);

a bit more extensive test for possible corner cases is IMO needed, the code in multiplex stream is quite complicated.

also would be good to: read(), change size of an inner stream, seek(0), check we get the new size

        Attachment #8967434 -
        Flags: review?(honzab.moz) → review-

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #85)
> I would not care about the
> AsyncWait(null) call in between, if that was the reason you've done it this
> way.

err... misplaced for AsyncWait for stream readiness, not length...  ignore that line please.

    
    

    
  
Comment on attachment 8967312 [details] [diff] [review]
part 2 - IPCBlobInputStream exposes nsIInputStreamLength

In the part 1
+   * This is not an attribute because a stream can change its length. For
+   * instance, if the stream is a file inputStream and the underlying OS file
+   * changes, its length will change as well.
+   */
+  long long length();
I don't quite understand the reasoning for not using attribute. Attribute values may change.



>+NS_IMETHODIMP
>+IPCBlobInputStream::AsyncWait(nsIInputStreamLengthCallback* aCallback,
>+                              nsIEventTarget* aEventTarget)
To make the code easier to follow, I'd use some other method name than AsyncWait which is already used by
nsIAsyncInputStream.
What about something like queryStreamLength?

Whenever mConsumed is set to true, we want to set 
mLengthCallback and mLengthCallbackEventTarget to null. Otherwise there is a high risk for leaks, I think.

I don't quite understand the setup around the two different asyncWaits. I think this code wants length to be
queried before data, but nothing really enforces that. We just start to throw if data is read before length reading.


>+IPCBlobInputStream::LengthReady(int64_t aLength)
>+{
>+  // We have been closed in the meantime.
>+  if (mState == eClosed || mConsumed) {


>+
>+  if (mLength < mActor->Size()) {
>+    aLength = XPCOM_MIN(aLength, (int64_t)mLength);
>+  }
This needs a comment.

>+// When the stream has been received from the parent, we inform the
>+// IPCBlobInputStream.
>+class LengthReadyRunnable final : public CancelableRunnable
>+{
>+public:
>+  LengthReadyRunnable(IPCBlobInputStream* aDestinationStream, int64_t aSize)
>+    : CancelableRunnable("dom::LengthReadyRunnable")
>+    , mDestinationStream(aDestinationStream)
>+    , mSize(aSize)
>+  {
>+    MOZ_ASSERT(mDestinationStream);
>+    // mCreatedStream can be null.
What mCreatedStream?

>   void
>+  LengthNeeded(IPCBlobInputStream* aStream,
>+                 nsIEventTarget* aEventTarget);
fix indentation
>+/* TODO
>+  RefPtr<LengthInputStreamHelper> helper =
>+    new LengthInputStreamHelper(stream);
>+
>+  helper->Run([self](length) {
>+    if (self->mContentManager || mPBackgroundManager) {
>+      Unused << self->SendLengthReady(length);
>+    }
>+  });
>+*/
What is this

>+
> nsIContentParent::DeallocPIPCBlobInputStreamParent(PIPCBlobInputStreamParent* aActor)
> {
>-  delete aActor;
>+  RefPtr<IPCBlobInputStreamParent> actor =
>+      dont_AddRef(static_cast<IPCBlobInputStreamParent*>(aActor));
fix indentation


>+++ b/dom/url/tests/mochitest.ini
>@@ -18,8 +18,9 @@ support-files =
> [test_worker_url.html]
> [test_worker_urlApi.html]
> [test_worker_url_exceptions.html]
> [test_worker_urlSearchParams.html]
> [test_unknown_url_origin.html]
> [test_bloburl_location.html]
> [test_worker_protocol.html]
> support-files = protocol_worker.js
>+[test_TEST.html]
What is this? Some debugging?
(but this bug needs tests)

BackgroundParentImpl::DeallocPIPCBlobInputStreamParent(PIPCBlobInputStreamParent* aActor)
> {
>   AssertIsInMainProcess();
>   AssertIsOnBackgroundThread();
>   MOZ_ASSERT(aActor);
> 
>-  delete aActor;
>+  RefPtr<mozilla::dom::IPCBlobInputStreamParent> actor =
>+      dont_AddRef(static_cast<mozilla::dom::IPCBlobInputStreamParent*>(aActor));
fix indentation

        Attachment #8967312 -
        Flags: review?(bugs) → review-

    
    

    
  
> +  long long length();

This follows the same logic of available().

> >+NS_IMETHODIMP
> >+IPCBlobInputStream::AsyncWait(nsIInputStreamLengthCallback* aCallback,
> >+                              nsIEventTarget* aEventTarget)
> To make the code easier to follow, I'd use some other method name than
> AsyncWait which is already used by
> nsIAsyncInputStream.
> What about something like queryStreamLength?

I don't really like the idea of changing this again. AsyncWait() is a common name of other similar operations:

nsIAsyncOutputStream,  nsIAsyncInputStream, nsIAsyncFileMetadata.

> I don't quite understand the setup around the two different asyncWaits. I
> think this code wants length to be
> queried before data, but nothing really enforces that. We just start to
> throw if data is read before length reading.

Right. The length is something that is available only before reading the stream.
This is documented in the interface.


> >+/* TODO
> >+  RefPtr<LengthInputStreamHelper> helper =
> >+    new LengthInputStreamHelper(stream);
> >+
> >+  helper->Run([self](length) {
> >+    if (self->mContentManager || mPBackgroundManager) {
> >+      Unused << self->SendLengthReady(length);
> >+    }
> >+  });
> >+*/
> What is this

See patch 3.

> >+[test_TEST.html]
> What is this? Some debugging?
> (but this bug needs tests)

Right. It has a lot of gtests for any stream implementation. Plus there are the existing necko tests.

I'll apply your comments as soon as possible.

    
    

    
  
I'd say nsIAsyncFileMetadata shouldn't have method called asyncWait.
And in nsIAsyncOutputStream and  nsIAsyncInputStream it makes a lot more sense.

    
    

    
  
In general method name should hint what it is going to do. asyncWait is super generic, and making same class to implement several methods named asyncWait with different argument types just decreases code readability.

    
    

    
  
(In reply to Olli Pettay [:smaug] (vacation Apr 15-20) from comment #90)
> In general method name should hint what it is going to do. asyncWait is
> super generic, and making same class to implement several methods named
> asyncWait with different argument types just decreases code readability.

First I somewhat did like it as it's a method of a specific interface, but then I caught myself once staring at "the other" asyncWait mistakenly before realizing the one I wanted was few lines away, so yes, it probably should have a different name.

    
    

    
  
Initially I submitted a patch with a method called: nsIAsyncFullSizeInputStream::asyncFullSizeWait(). The interface now is called nsIASyncInputStreamLength, so I guess the new method name could be:  asyncLengthWait() or asyncWaitForLength().

    
    

    
  
Comment on attachment 8967128 [details] [diff] [review]
part 8 - docShell and necko

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

Sorry for late answer.

I'm getting a bit lost in this chain of patches, so I'm sorry if I happen requesting a change to something you are removing or changing later.  maybe would be good to merge the patches or at least clearly describe (set) bug dependencies?

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1045,5 @@
>  
> +  nsresult rv = SetRequestMethod(aMethod);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // SetRequestHeader propagates headers to chrome if HttpChannelChild

unfinished comment

@@ +1069,5 @@
> +  if (aContentLength >= 0) {
> +    return ExplicitSetUploadStreamLength(aContentLength, aStreamHasHeaders);
> +  }
> +
> +  // Let's resolve the size of the stream.

assuming that one of the followup patches will turn this to `try sync, fallback async` path, right?

@@ +1152,5 @@
> +
> +  nsCOMPtr<nsISupports> context;
> +  context.swap(mPendingAsyncOpenContext);
> +
> +  AsyncOpen(listener, context);

r- for this bit.

If AsyncOpen fails, you MUST call AsyncAbort with the error so that the listener gets notified.  Of course only in case this is called after the first call to AsyncOpen has already returned to the caller with NS_OK.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +746,5 @@
> +
> +  // These 2 members are needed to continue the AsyncOpen() when
> +  // InputStreamLengthHelper::GetLength callback is executed.
> +  nsCOMPtr<nsIStreamListener> mPendingAsyncOpenListener;
> +  nsCOMPtr<nsISupports> mPendingAsyncOpenContext;

could you just use the existing mListener and mListenerContext members and here have a flag (to save some memory, since this is a heavily used class?)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +569,3 @@
>  
> +    // Let's resolve the size of the stream. The following operation is always
> +    // async.

assuming that this will be made `try sync, fallback to async` in one of the followup patches, yes?

        Attachment #8967128 -
        Flags: review?(honzab.moz) → review-

    
    

    
  


  
Re-uploading all the patches.

        Attachment #8947017 -
        Attachment is obsolete: true

        Attachment #8966188 -
        Attachment is obsolete: true

        Attachment #8966883 -
        Attachment is obsolete: true

        Attachment #8966884 -
        Attachment is obsolete: true

        Attachment #8966885 -
        Attachment is obsolete: true

        Attachment #8966893 -
        Attachment is obsolete: true

        Attachment #8966894 -
        Attachment is obsolete: true

        Attachment #8966895 -
        Attachment is obsolete: true

        Attachment #8966896 -
        Attachment is obsolete: true

        Attachment #8966897 -
        Attachment is obsolete: true

        Attachment #8966899 -
        Attachment is obsolete: true

        Attachment #8966905 -
        Attachment is obsolete: true

        Attachment #8967013 -
        Attachment is obsolete: true

        Attachment #8967128 -
        Attachment is obsolete: true

        Attachment #8967148 -
        Attachment is obsolete: true

        Attachment #8967291 -
        Attachment is obsolete: true

        Attachment #8967312 -
        Attachment is obsolete: true

        Attachment #8967434 -
        Attachment is obsolete: true

        Attachment #8973671 -
        Flags: review?(bugs)

    
    

    
  


  
This patch has been already reviewed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 8 - docShell and necko (obsolete)
        — 
          — Splinter Review
      

    


  
This patch is the last one here. All the rest can land in a separate bug.

        Attachment #8973678 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8973671 [details] [diff] [review]
part 1 - nsIInputStreamLength and nsIAsyncInputStreamLength

>+/**
>+ * Note: Instead of using these interfaces directly, consider to use
>+ * InputStreamLengthHelper class.
>+ */
>+
>+[uuid(452d059f-9a9c-4434-8839-e10d1405647c)]
>+interface nsIInputStreamLength : nsISupports
>+{
>+  /**
>+   * Returns the total length of the stream if known. Otherwise it returns -1.
>+   * This is different than calling available() which returns the number of
>+   * bytes ready to be read from the stream.
>+   * -1 is a valid value for a stream that doesn't know its length. For
>+   * instance, a pipe stream could return such value.
>+   *
>+   * It could throw NS_BASE_STREAM_WOULD_BLOCK if the inputStream is
>+   * non-blocking. If this happens, you should use
>+   * nsIAsyncInputStreamLength::asyncLengthWait().
>+   *
>+   * If the stream has already been read (read()/readSegments()/close()/seek()
>+   * methods has been called), length() returns NS_ERROR_NOT_AVAILABLE.
>+   *
>+   * This is not an attribute because a stream can change its length. For
>+   * instance, if the stream is a file inputStream and the underlying OS file
>+   * changes, its length will change as well.
Not a normal reasoning for not being an attribute. attribute values may change all the time.
Say, element.parentNode is a readonly attribute, yet its value may change.
So, just drop the comment. Keeping it as a method sounds still ok.
>+   * If the stream is non-blocking, nsIInputStreamLength::length() can return
>+   * NS_BASE_STREAM_WOULD_BLOCK. The caller must then wait for the stream to
>+   * know its length.
>+   *
>+   * If the stream implements nsIAsyncInputStreamLength, then the caller can
>+   * use this interface to request an asynchronous notification when the
>+   * stream's length becomes known (via the AsyncLengthWait method).
it is asyncLengthWait in the idl

>+   * If the length is already known, the aCallback will be still called
drop 'the' before aCallback

        Attachment #8973671 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8973672 [details] [diff] [review]
part 2 - IPCBlobInputStream exposes nsIInputStreamLength

>#
>+IPCBlobInputStream::AsyncLengthWait(nsIInputStreamLengthCallback* aCallback,
>+                                    nsIEventTarget* aEventTarget)
>+{
>+  MOZ_ASSERT(!!aCallback == !!aEventTarget);
Here you assert

>+
>+  MutexAutoLock lock(mMutex);
>+
>+  if (mState == eClosed) {
>+    return NS_BASE_STREAM_CLOSED;
>+  }
>+
>+  if (mConsumed) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  // If we have the callback, we must have the event target.
>+  if (NS_WARN_IF(!!aCallback != !!aEventTarget)) {
>+    return NS_ERROR_FAILURE;
>+  }
here you warn.
One or the other, not both.
I guess this if() is good enough.

>+IPCBlobInputStream::LengthReady(int64_t aLength)
>+{
>+  nsCOMPtr<nsIInputStreamLengthCallback> lengthCallback;
>+  nsCOMPtr<nsIEventTarget> lengthCallbackEventTarget;
>+
>+  {
>+    MutexAutoLock lock(mMutex);
>+
>+    // We have been closed in the meantime.
>+    if (mState == eClosed || mConsumed) {
>+      return;
>+    }
mConsumed check here isn't quite what the .idl documentation says, since the callback creator doesn't get
any error that the stream was read before the length callback was called. At least improve the documentation in .idl


>+// When the stream has been received from the parent, we inform the
>+// IPCBlobInputStream.
>+class LengthReadyRunnable final : public CancelableRunnable
>+{
>+public:
>+  LengthReadyRunnable(IPCBlobInputStream* aDestinationStream, int64_t aSize)
>+    : CancelableRunnable("dom::LengthReadyRunnable")
>+    , mDestinationStream(aDestinationStream)
>+    , mSize(aSize)
>+  {
>+    MOZ_ASSERT(mDestinationStream);
>+    // mCreatedStream can be null.
There is no mCreateStream in this class. I mentioned this already in previous review.


>+    if (mLength < mActor->Size()) {
>+      aLength = XPCOM_MIN(aLength, (int64_t)mLength);
>+    }
This still needs a comment.

>+  LengthNeeded(IPCBlobInputStream* aStream,
>+                 nsIEventTarget* aEventTarget);
This still needs indentation fix.


>   // This struct and the array are used for creating streams when needed.
>   struct PendingOperation
>   {
>     RefPtr<IPCBlobInputStream> mStream;
>     nsCOMPtr<nsIEventTarget> mEventTarget;
>+    enum {
Nit, { should be on its own line


>+/* TODO
>+  RefPtr<LengthInputStreamHelper> helper =
>+    new LengthInputStreamHelper(stream);
>+
>+  helper->Run([self](length) {
>+    if (self->mContentManager || mPBackgroundManager) {
>+      Unused << self->SendLengthReady(length);
>+    }
>+  });
>+*/
ok, looks like this patch isn't read.



>   nsresult rv;
>-  IPCBlobInputStreamParent* parentActor =
>+  RefPtr<IPCBlobInputStreamParent> parentActor =
>     IPCBlobInputStreamParent::Create(aInputStream, aSize, aChildID, &rv,
>                                      aManager);
>   if (!parentActor) {
>     return rv;
>   }
> 
>   if (!aManager->SendPIPCBlobInputStreamConstructor(parentActor,
>                                                     parentActor->ID(),
>                                                     parentActor->Size())) {
>     return NS_ERROR_FAILURE;
>   }
> 
>+  // We need manually to increase the reference for this actor because the
>+  // IPC allocator method is not triggered. The Release() is called by IPDL
>+  // when the actor is deleted.
>+  parentActor.get()->AddRef();
Why not just call parentActor.forget() ?



> nsIContentParent::DeallocPIPCBlobInputStreamParent(PIPCBlobInputStreamParent* aActor)
> {
>-  delete aActor;
>+  RefPtr<IPCBlobInputStreamParent> actor =
>+      dont_AddRef(static_cast<IPCBlobInputStreamParent*>(aActor));
2 spaces for indentation


> BackgroundParentImpl::DeallocPIPCBlobInputStreamParent(PIPCBlobInputStreamParent* aActor)
> {
>   AssertIsInMainProcess();
>   AssertIsOnBackgroundThread();
>   MOZ_ASSERT(aActor);
> 
>-  delete aActor;
>+  RefPtr<mozilla::dom::IPCBlobInputStreamParent> actor =
>+      dont_AddRef(static_cast<mozilla::dom::IPCBlobInputStreamParent*>(aActor));
2 spaces for indentation
(I would just NS_RELEASE)



Given that review comments were fixed, I assume wrong patch was uploaded.

        Attachment #8973672 -
        Flags: review?(bugs) → review-

    
    

    
  
Comment on attachment 8973678 [details] [diff] [review]
part 8 - docShell and necko

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1043,5 @@
>  
> +  nsresult rv = SetRequestMethod(aMethod);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // SetRequestHeader propagates headers to chrome if HttpChannelChild

please either remove or fix up this broken sentence.

@@ +1161,5 @@
> +
> +  nsCOMPtr<nsISupports> context;
> +  context.swap(mPendingAsyncOpenContext);
> +
> +  AsyncOpen(listener, context);

this was unaddressed, see comment 93.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +751,5 @@
> +  nsCOMPtr<nsISupports> mPendingAsyncOpenContext;
> +
> +  // This is set true if the channel is waiting for the
> +  // InputStreamLengthHelper::GetAsyncLength callback.
> +  bool mPendingInputStreamLengthOperation;

again, no response to my comments: could we use the existing members?  if not, please say why not.

        Attachment #8973678 -
        Flags: review?(honzab.moz) → review-

    
    

    
  
> >+/* TODO
> >+*/
> ok, looks like this patch isn't read.

This part is changed in the following patches where I introduce InputStreamLengthHelper class.

> > 
> >+  // We need manually to increase the reference for this actor because the
> >+  // IPC allocator method is not triggered. The Release() is called by IPDL
> >+  // when the actor is deleted.
> >+  parentActor.get()->AddRef();
> Why not just call parentActor.forget() ?

To follow the same pattern of IPCBlobInputStreamThread.

All the other comments applied.

    
    

    
  


  

        Attachment #8973672 -
        Attachment is obsolete: true

        Attachment #8975410 -
        Flags: review?(bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 8 - docShell and necko (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8973678 -
        Attachment is obsolete: true

        Attachment #8975419 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8975419 [details] [diff] [review]
part 8 - docShell and necko

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1151,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!mPendingInputStreamLengthOperation);
> +
> +  if (!mListener) {
> +    return;

I don't understand this bit - mListener is non-null during the whole processing time of a channel (between successfull AsyncOpen and call and call to listener's OnStopRequest) as well as when we are waiting for the length operation.  

Hence I'm not sure you can use this as a good condition to prevent repeated (unwanted) call to AsyncOpen when things are not pending.  Also let you know that we access mListener on multiple threads (OnDataAvailable can be called on worker threads).

It's good you reuse the existing members, but we probably need a new (best a bit) field to prevent MaybeResumeAsyncOpen trigger AsyncOpen more than once or somehow strongly ensure the logic doesn't misbehave (that this will not be called after we have already started the channel operation)


it seems to me that the condition for !mPendingInputStreamLengthOperation is not enough

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +428,5 @@
>    virtual void ReleaseListeners();
>  
> +  // Call AsyncAbort().
> +  virtual void
> +  DoAsyncAbort(nsresult aStatus) = 0;

what's exactly the reason for this method?  some build issues when using the plain AsyncAbort?  Please add a comment why you have to add this method.

    
    

    
  
please see comment 109
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 8975410 [details] [diff] [review]
part 2 - IPCBlobInputStream exposes nsIInputStreamLength

>+    if (mLength < mActor->Size()) {
>+      // If the remote stream must be sliced, we must return here the correct
>+      // value.
>+      aLength = XPCOM_MIN(aLength, (int64_t)mLength);
>+    }


Please document what mLength is in .h.
And "we must return here the correct value." isn't clear.
The comment should explain what aLength should point to - why min of aLength and mLength is the right value?

        Attachment #8975410 -
        Flags: review?(bugs) → review+

    
    

    
  
> I don't understand this bit - mListener is non-null during the whole
> processing time of a channel (between successfull AsyncOpen and call and
> call to listener's OnStopRequest) as well as when we are waiting for the
> length operation.

This method is called when GetAsyncLength() execs the callback, and, maybe AsyncOpen2() has not been called yet. In this case, mListener is null.

I see the problem of calling AsyncOpen() more than once. I think would be better to use mPendingAsyncOpenListener/Context as I was doing in the previous patches.

> what's exactly the reason for this method?  some build issues when using the
> plain AsyncAbort?  Please add a comment why you have to add this method.

AsyncAbort() is not part of the channel. It's part of HttpAsyncAborter class. nsHttpChannel and other classes inherit HttpAsyncAborter, but here we are in HttpBaseChannel, which doesn't have access to AsyncAbort().
Flags: needinfo?(amarchesini) → needinfo?(honzab.moz)

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #112)
> > I don't understand this bit - mListener is non-null during the whole
> > processing time of a channel (between successfull AsyncOpen and call and
> > call to listener's OnStopRequest) as well as when we are waiting for the
> > length operation.
> 
> This method is called when GetAsyncLength() execs the callback, and, maybe
> AsyncOpen2() has not been called yet. In this case, mListener is null.
> 
> I see the problem of calling AsyncOpen() more than once. I think would be
> better to use mPendingAsyncOpenListener/Context as I was doing in the
> previous patches.

What I'm trying to avoid is adding new heavy members to the http channel class unless really necessary.  here I have a feeling it's not necessary and we can do with the existing mListener/Context members.  to protect duplicate effects when you method is called unexpectedly more than once we just need to add a flag - a bit field at best - that will prevent it.  I would also add assert(false) when we hit it more than once, since that should probably not happen.

do you think it's doable?

> 
> > what's exactly the reason for this method?  some build issues when using the
> > plain AsyncAbort?  Please add a comment why you have to add this method.
> 
> AsyncAbort() is not part of the channel. It's part of HttpAsyncAborter
> class. nsHttpChannel and other classes inherit HttpAsyncAborter, but here we
> are in HttpBaseChannel, which doesn't have access to AsyncAbort().

aah.... yes, then it's probably the best way to work that around as you do.
Flags: needinfo?(honzab.moz) → needinfo?(amarchesini)

    
    

    
  


  

        Attachment #8975419 -
        Attachment is obsolete: true

        Attachment #8975419 -
        Flags: review?(honzab.moz)
Flags: needinfo?(amarchesini)

        Attachment #8976544 -
        Flags: review?(honzab.moz)

    
    

    
  
Comment on attachment 8976544 [details] [diff] [review]
part 8 - docShell and necko

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

thanks!

        Attachment #8976544 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2576bb7a8689
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 1 - IDL, r=mayhemer, r=froydnj, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0086783f1f56
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 2 - IPCBlobInputStream exposes nsIInputStreamLength, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b81e43d9ec
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 3 - InputStreamLengthHelper, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c1f8c0e321
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 4 - SlicedInputStream exposes nsIInputStreamLength, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/14019553d3de
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 5 - nsBufferedInputStream exposes nsIInputStreamLength, r=froydnj, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5e4a41b246
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 6 - nsMIMEInputStream exposes nsIInputStreamLength, r=mayhmer
https://hg.mozilla.org/integration/mozilla-inbound/rev/89acab76790a
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 7 - nsIMultiplexInputStream exposes nsIInputStreamLength, r=froydnj, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/791e32d77f0f
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 8 - PartiallySeekableInputStream exposes nsIInputStreamLength, r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f6b8d84e5d
Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 9 - necko and docShell, r=mayhemer, r=smaug

    
  
Blocks: 1463650

    
    

    
  
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0475a0ce8e3f
IPCBlobInputStreamParent reference must be increamented before calling any IPC method, r=me
Flags: qe-verify+
Flags: in-testsuite+

    
    

    
  
This like a scary thing to uplift. Calling this wontfix for 61.

          status-firefox61:
          affected → wontfix

    
    

    
  
Managed to reproduce using Firefox 59.0a1(20180113220355).

Have checked with Firefox 62.0b8 on Win_10x64, Ubuntu 16.04LTS, macOS 10.9 and can confirm the issue is no longer reproducible.
Status: RESOLVED → VERIFIED

          status-firefox62:
          fixed → verified
Flags: qe-verify+
Blocks: 1494176
No longer blocks: 1494176
Depends on: 1494176

    
  
Depends on: 1495363
Component: HTML: Form Submission → DOM: Core & HTML
Regressions: 1625749

    
    

    
  
Stopping by with the following observation in Firefox 79. I was first thinking it could be a regression but then failed to confirm.


Form defined as <form method="post" action="/~amk/8smer" enctype="multipart/form-data">

with no file upload, when submitted using Submit button sends proper header to the web server:


POST /~amk/8smer HTTP/1.1

Host: ver

User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:79.0) Gecko/20100101 Firefox/79.0

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8

Accept-Language: en-US,en;q=0.5

Accept-Encoding: gzip, deflate

Content-Type: multipart/form-data; boundary=---------------------------256117336139382807803966776657

Content-Length: 705

Origin: http://ver

Connection: keep-alive

Referer: http://ver/~amk/8smer

Cookie: __utma=242530053.219340367.1559423829.1559423829.1559435217.2

Upgrade-Insecure-Requests: 1


Refresh of the resulting page requests permission to repost data, but ends up with Bad Request. The http packet is lacking Content-Length as well as Content-Type and HTTP header is not terminated either:


POST /~amk/8smer HTTP/1.1

Host: ver

User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:79.0) Gecko/20100101 Firefox/79.0

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8

Accept-Language: en-US,en;q=0.5

Accept-Encoding: gzip, deflate

Referer: http://ver/~amk/8smer

Origin: http://ver

Connection: keep-alive

Cookie: __utma=242530053.219340367.1559423829.1559423829.1559435217.2

Upgrade-Insecure-Requests: 1

Cache-Control: max-age=0

-----------------------------30002942952810996801798892234

Content-Disposition: form-data; name="grid"


NORKA... etc (actual form data)


I am not sure whether to reopen this bug or report a new one. A problem is that I cannot reproduce this consistently.

Issue did not happen when I started to develop the script. Probably started after playing with the object inspector, not sure.

It does not happen in a newly created profile so I do not have sufficient data to submit a new bug.  Yet I wanted to leave some trace

in bugzilla in case someone else finds this bug like I did today.



    
    

    
  
I have witnessed the same issue.  Yesterday I spent hours trying to figure out why I was getting a bad request, although it was inconsistent at first.  In the end I was able to reproduce the issue by submitting a form that contained an md5 hash value for a particular field (proprietary session tracking) and this particular value consistently causes a bad request upon form re-submission.  As the previous submission noted, the http request is missing content-length among others. The request is clearly bad.  In fact, if a person uses the option to edit and resend, the request has the proper headers.  This appears to be a bug.  I can't reproduce the same problem with Chrome using the same set of data.



    
    

    
  
(In reply to mmkwall from comment #124)




I have witnessed the same issue.  Yesterday I spent hours trying to figure out why I was getting a bad request, although it was inconsistent at first.  In the end I was able to reproduce the issue by ... This appears to be a bug.  I can't reproduce the same problem with Chrome using the same set of data.




Thanks for the supportive comment. I did not happen to identify the trigger. If you have reproduction steps, I'd suggest to raise a new bug.



          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: