1273279 - [FlyWeb] Review InternalResponse changes for landing in mozilla-central

Status: RESOLVED FIXED

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

Description

[Kannan Vijayan [:djvj]]

Attachment: flyweb-internal-response.patch

These are DOM changes related to tracking the response body size in InternalResponse.

These changes are included in the main DOM patchset as well (see bug 1272099), but they are split out in this bug too for specific review.

Comment 1

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

Note that there were some review comments already for this code in bug 1272099.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8753064 [details] [diff] [review]
flyweb-internal-response.patch

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

It would be nice to see some description of the motivation for this patch.  Right now its hard to understand why we need the size in some cases but can just ignore it otherwise.  Seems very arbitrary.

I had to give f-, though, for two main issues:

1) This treats zero length bodies as null bodies which has a content script observable change.
2) It ignores the possibility of chunk-encoding or variable length streams for bodies.  This is something we need to deal with very soon.  It would be nice to at least have a way of indicating this so we don't have to churn the API again.  This patch is baking in a fixed length requirement that wasn't there before (again with no rationale provided).

::: dom/fetch/Fetch.cpp
@@ +456,5 @@
>      return rv.StealNSResult();
>    }
>  
> +  if (aContentLength) {
> +    impl->GetInternalStream(aStream, rv);

I don't think this is right.  There is a difference between a null stream and an empty stream in the fetch spec.  If a blob is passed, then the body stream must not be considered null.  Instead a zero length nsIInputStream must be provided.

@@ +461,5 @@
> +    if (NS_WARN_IF(rv.Failed())) {
> +      return rv.StealNSResult();
> +    }
> +  }
> +  

nit: trailing white space

@@ +518,3 @@
>  
> +  return aContentLength ?
> +    NS_NewCStringInputStream(aStream, encoded) : NS_OK;

Again, a zero length nsIInputStream must be provided here to distinguish between:

  var r1 = new Request(url, { body: undefined })
  r1.text().then(function() {
    // r1.bodyUsed === false
  });

And

  var r2 = new Response(url, { body: '' })
  r2.text().then(function() {
    // r2.bodyUsed === true
  });

The bodyUsed logic is dependent on this here:

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Request.cpp#548

@@ +530,5 @@
>    aParams.Stringify(serialized);
>    aContentType = NS_LITERAL_CSTRING("application/x-www-form-urlencoded;charset=UTF-8");
> +  aContentLength = serialized.Length();
> +  return aContentLength ?
> +    NS_NewCStringInputStream(aStream, NS_ConvertUTF16toUTF8(serialized)) : NS_OK;

Again, a zero length stream should be provided.

::: dom/fetch/FetchDriver.cpp
@@ +487,5 @@
>    // step 5, "redirect status", step 11.
>  
>    bool foundOpaqueRedirect = false;
>  
> +  int64_t contentLength;

Lets just initialize this to zero.  Seems a bit safer in general.

@@ +490,5 @@
>  
> +  int64_t contentLength;
> +  rv = channel->GetContentLength(&contentLength);
> +  if (NS_FAILED(rv)) {
> +    contentLength = 0;

And then you can just make this MOZ_ASSERT_IF(NS_FAILED(rv), contentLength == 0).

@@ +521,5 @@
> +
> +    // Http-channels lie about their content-length sometimes
> +    ErrorResult res;
> +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {

You must call res.SuppressException() here.  If an error is thrown and nothing handles it ErrorResult will assert.

@@ +543,5 @@
>                                    result);
>        MOZ_ASSERT(!result.Failed());
>      }
>  
> +    if (contentLength > 0) {

This doesn't seem to be what the spec says.  Have you opened a spec issue?

@@ +571,5 @@
>      FailWithNetworkError();
>      // Cancel request.
>      return rv;
>    }
> +  response->SetBody(pipeInputStream, contentLength);

Why are you getting the contentLength out of the body after you might have set the Content-Length header?  Shouldn't you move the header setting code below this?

::: dom/fetch/InternalResponse.h
@@ +205,4 @@
>    }
>  
>    void
> +  SetBody(nsIInputStream* aBody, uint64_t aBodySize)

Why must we know the length of the body up front?  In the near future we are going to need to support Response.body streams defined by js where we do not know the body size at this point.

I would really prefer not to add this restriction if we know we are going to be removing it immediately.

::: dom/fetch/Request.cpp
@@ +515,5 @@
>        const OwningArrayBufferOrArrayBufferViewOrBlobOrFormDataOrUSVStringOrURLSearchParams& bodyInit =
>          bodyInitNullable.Value();
>        nsCOMPtr<nsIInputStream> stream;
>        nsAutoCString contentType;
> +      uint64_t contentSize;

And yet, we don't do the same thing for Request.  This contentSize for the request body is just ignored instead of passing to request->SetBody().  Why?

Again, though, we probably will need to support chunked encoding uploads provided by a js stream in the near future.

::: dom/cache/TypeUtils.cpp
@@ +302,5 @@
>    }
>  
>    nsCOMPtr<nsIInputStream> stream = ReadStream::Create(aIn.body());
> +  uint64_t avail = 0;
> +  stream->Available(&avail);

No error checking?

::: dom/workers/ScriptLoader.cpp
@@ +656,5 @@
>  
>      // We synthesize the result code, but its never exposed to content.
>      RefPtr<mozilla::dom::InternalResponse> ir =
>        new mozilla::dom::InternalResponse(200, NS_LITERAL_CSTRING("OK"));
> +    ir->SetBody(loadInfo.mCacheReadStream, 0);

Again, I feel like we should think about chunked encoding.  It seems 0 is used here because the stream is a pipe.  But a 0 length stream and a chunked encoded stream are very different things.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

But thank you for working on this!  Sorry if my comments were brusque.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8753064 [details] [diff] [review]
flyweb-internal-response.patch

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

::: dom/fetch/FetchDriver.cpp
@@ +522,5 @@
> +    // Http-channels lie about their content-length sometimes
> +    ErrorResult res;
> +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
> +      contentLength = 0;

Also, zeroing out the contentLength if these headers are set seems a bit heavy handed.  If this is a fair thing to do for the entire platform, why isn't it done in necko?  And if its not fair for the whole platform why are we enforcing this for all fetch consumers?  It seems like something you could implement in your particular FlyWeb consumer.

Comment 5

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

> It would be nice to see some description of the motivation for this patch. 
> Right now its hard to understand why we need the size in some cases but can
> just ignore it otherwise.  Seems very arbitrary.

We would always want to know the size. However in some cases it's simply not possible to know the size, and so we also need to support "body has unknown size". I've tried to always get the size when possible, but if a http-response comes back without a ContentLength header then we have to flag the response as "body has unknown size".

Right now "body has unknown size" is indicated by returning 0 as the size, but based on comments below we probably need to flag this some other way. Maybe by setting the size to -1.

For resources stored in the cache, I couldn't see a way to get the size even in the cases where the size is actually known. So for now I always flag responses as "body has unknown size" when coming from the cache.


We use this information in the flyweb http server. When the body has a known size, we can send a Content-Length header in the response. When the body does not have a known size, we use chunked encoding to send the response. However this comes at a performance penalty both in the client and the server, which is why we want to send the Content-Length header where possible.

This code is landing in bug 1272099, which depends on this bug (marking it so).
In practice, the way we use this information is that when sending a response where the body 


> 1) This treats zero length bodies as null bodies which has a content script
> observable change.

Yeah, sounds like this needs to be fixed.

In general sounds like the patch as-is mixes up 0-sized bodies, unknown-sized bodies and null-bodies. We need to fix that.

> 2) It ignores the possibility of chunk-encoding or variable length streams
> for bodies.  This is something we need to deal with very soon.  It would be
> nice to at least have a way of indicating this so we don't have to churn the
> API again.  This patch is baking in a fixed length requirement that wasn't
> there before (again with no rationale provided).

The intent is that such bodies are flagged as "body has unknown size" as discussed below. This is done by setting the size to 0, but still return a non-null body object. But sounds like we need to flag this some other way so that we don't mix up 0-sized bodies with unknown-sized bodies.


> @@ +521,5 @@
> > +
> > +    // Http-channels lie about their content-length sometimes
> > +    ErrorResult res;
> > +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> > +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
> 
> You must call res.SuppressException() here.  If an error is thrown and
> nothing handles it ErrorResult will assert.

See bug 1272099 comment 5.

> ::: dom/fetch/InternalResponse.h
> @@ +205,4 @@
> >    }
> >  
> >    void
> > +  SetBody(nsIInputStream* aBody, uint64_t aBodySize)
> 
> Why must we know the length of the body up front?  In the near future we are
> going to need to support Response.body streams defined by js where we do not
> know the body size at this point.
> 
> I would really prefer not to add this restriction if we know we are going to
> be removing it immediately.

Bodies where the size is not known up front we should just treat it as unknown-sized bodies. I guess we could also enable setting the body size later, if we find out the size before we actually need it.

But even in that case we should set the size as unknown up front, and modify it later. So I think it still makes sense to keep the current signature. If we separate setting the body stream from setting the size I suspect we'll forget to set the size very often given how hard that is to detect (only a perf regression and only for flyweb).

> ::: dom/fetch/Request.cpp
> @@ +515,5 @@
> >        const OwningArrayBufferOrArrayBufferViewOrBlobOrFormDataOrUSVStringOrURLSearchParams& bodyInit =
> >          bodyInitNullable.Value();
> >        nsCOMPtr<nsIInputStream> stream;
> >        nsAutoCString contentType;
> > +      uint64_t contentSize;
> 
> And yet, we don't do the same thing for Request.  This contentSize for the
> request body is just ignored instead of passing to request->SetBody().  Why?

I don't understand this question. We currently have no need to track Request body size, so the patch does not add that.

> Again, though, we probably will need to support chunked encoding uploads
> provided by a js stream in the near future.
> 
> ::: dom/cache/TypeUtils.cpp
> @@ +302,5 @@
> >    }
> >  
> >    nsCOMPtr<nsIInputStream> stream = ReadStream::Create(aIn.body());
> > +  uint64_t avail = 0;
> > +  stream->Available(&avail);
> 
> No error checking?

Yeah, if we're using some other way to indicate "unknown size" than 0, we probably need to add error checking here.

Comment 6

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

> ::: dom/fetch/FetchDriver.cpp
> @@ +522,5 @@
> > +    // Http-channels lie about their content-length sometimes
> > +    ErrorResult res;
> > +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> > +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
> > +      contentLength = 0;
> 
> Also, zeroing out the contentLength if these headers are set seems a bit
> heavy handed.  If this is a fair thing to do for the entire platform, why
> isn't it done in necko?  And if its not fair for the whole platform why are
> we enforcing this for all fetch consumers?  It seems like something you
> could implement in your particular FlyWeb consumer.

When either of those headers are set, the size of the body stream is unknown. It is possible that we indeed should fix this at a necko level, but that felt like a more risky change. There's a lot of places that get the nsIChannel.contentLength, including addons, so modifying all of them seems very risky.

Note that the Content-Length header, which is what httpchannel.contentLength currently returns, can be meaningfull even when the above to headers are set. For example when creating a download bar.

Comment 7

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

So, to clarify. When the "Content-Encoding" or "Transfer-Encoding" headers are set, HTTP simply doesn't have a way to signal how many bytes the resulting body has. This is not something that's specific to FlyWeb, but is inherent in all currently existing versions of HTTP.

However the "Content-Length" header can in at least some situation carry meaning. Just not the meaning of "how many bytes does the resulting body contain". For example, when only "Transfer-Encoding" is set, it means "number of bytes that will be transferred over the wire", which is meaningful value when drawing a progress bar. I believe it's also the value that should be exposed by the XHR progress events.

So I think there's a significant risk that changing the value of httpchannel.contentLength will break existing code.

The correct solution here would be to expose separate values for "size of resulting body" and "number of bytes that will be transferred across the wire". Hopefully once fetch() grows progress notifications we'll expose both.

Until then I think it makes sense to track "size of resulting body" along with the body, since that's the value that we have a usecase for.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> For resources stored in the cache, I couldn't see a way to get the size even
> in the cases where the size is actually known. So for now I always flag
> responses as "body has unknown size" when coming from the cache.

If you are talking about dom cache API we could definitely expose this.  We would just need to track the number of bytes written to disk before compression and store it in the sqlite entry for the response.

Related: https://github.com/slightlyoff/ServiceWorker/issues/587

Note, cache API is a bit tricky with size because the headers remain intact, but the body data is returned after the data has been decoded.  So a response with gzip encoding will usually have a content-length matching the compressed size, but the response body data will be the uncompressed size.

Also, related: https://github.com/whatwg/fetch/issues/67

Comment 9

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

I think getting the body size for entries from the cache sounds like something we should do as a followup. But having an explicit comment indicating that sounds like a good idea.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8753064 [details] [diff] [review]
flyweb-internal-response.patch

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

bkelly did a perfect review of this patch. I remove my review request for now.

::: dom/fetch/Fetch.cpp
@@ +550,1 @@
>    } else if (aBodyInit.IsArrayBufferView()) {

no else after return.

@@ +580,4 @@
>  
>    if (aBodyInit.IsArrayBuffer()) {
>      const ArrayBuffer& buf = aBodyInit.GetAsArrayBuffer();
> +    return ExtractFromArrayBuffer(buf, aStream, aContentLength);

ditto.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Incidentally, chrome just sent an intent-to-ship for the js provided ReadableStream in Response objects:

https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/0S-jPuCy8ws

I'm actively working on our ReadableStream stuff over in bug 1128959 and its dependencies.

Comment 12

[Kannan Vijayan [:djvj]]

bkelly: see comment responses below.  Jonas is working on e10s stuff so I'm handling review comments.

>::: dom/fetch/Fetch.cpp
>@@ +456,5 @@
>>      return rv.StealNSResult();
>>    }
>>
>> +  if (aContentLength) {
>> +    impl->GetInternalStream(aStream, rv);
>
>I don't think this is right.  There is a difference between a null stream and an empty stream in the fetch spec.  If a blob is passed, then the body stream must not be considered null.  Instead a zero length nsIInputStream must be provided.

Fixed.  We always do GetInternalStream now, within ExtractFromBlob.

>@@ +521,5 @@
>> +
>> +    // Http-channels lie about their content-length sometimes
>> +    ErrorResult res;
>> +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
>> +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
>
>You must call res.SuppressException() here.  If an error is thrown and nothing handles it ErrorResult will assert.

Fixing instead by asserting !res.Failed();
Renamed |res| to |result|.

>@@ +543,5 @@
>>                                    result);
>>        MOZ_ASSERT(!result.Failed());
>>      }
>>
>> +    if (contentLength > 0) {
>
>This doesn't seem to be what the spec says.  Have you opened a spec issue?
>
>
>@@ +571,5 @@
>>      FailWithNetworkError();
>>      // Cancel request.
>>      return rv;
>>    }
>> +  response->SetBody(pipeInputStream, contentLength);
>
>Why are you getting the contentLength out of the body after you might have set the Content-Length header?  Shouldn't you move the header setting code below this?

Fixed by setting contentLength to -1 when it's not known.  I don't understand the issue in the second comment.  We call SetBody with the same contentLenth that add as the "Content-Length" header prior.  These two values should be in sync.

>::: dom/fetch/InternalResponse.h
>@@ +205,4 @@
>>    }
>>
>>    void
>> +  SetBody(nsIInputStream* aBody, uint64_t aBodySize)
>
>Why must we know the length of the body up front?  In the near future we are going to need to support Response.body streams defined by js where we do not know the body size at this point.
>
>I would really prefer not to add this restriction if we know we are going to be removing it immediately.

Changed this whole file to keep track of a |int64_t| body size, which is allowed to be -1 to indicate unknown size.

>::: dom/fetch/Request.cpp
>@@ +515,5 @@
>>        const OwningArrayBufferOrArrayBufferViewOrBlobOrFormDataOrUSVStringOrURLSearchParams& bodyInit =
>>          bodyInitNullable.Value();
>>        nsCOMPtr<nsIInputStream> stream;
>>        nsAutoCString contentType;
>> +      uint64_t contentSize;
>
>And yet, we don't do the same thing for Request.  This contentSize for the request body is just ignored instead of passing to request->SetBody().  Why?
>
>Again, though, we probably will need to support chunked encoding uploads provided by a js stream in the near future.

Jonas responded in comment 4 to this.

>::: dom/cache/TypeUtils.cpp
>@@ +302,5 @@
>>    }
>>
>>    nsCOMPtr<nsIInputStream> stream = ReadStream::Create(aIn.body());
>> +  uint64_t avail = 0;
>> +  stream->Available(&avail);
>
>No error checking?

Fixed to use int64_t avail, and set to -1 (unkown size) on error.

>::: dom/workers/ScriptLoader.cpp
>@@ +656,5 @@
>>
>>      // We synthesize the result code, but its never exposed to content.
>>      RefPtr<mozilla::dom::InternalResponse> ir =
>>        new mozilla::dom::InternalResponse(200, NS_LITERAL_CSTRING("OK"));
>> +    ir->SetBody(loadInfo.mCacheReadStream, 0);
>
> Again, I feel like we should think about chunked encoding.  It seems 0 is used here because the stream is a pipe.  But a 0 length stream and a chunked encoded stream are very different things.

Setting to -1 here to indicate unknown size.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Can you add constant like InternalResponse::UNKNOWN_SIZE for the -1 magic number?

I'm still unsure about the special handling around transfer-encoding and content-encoding headers.

Comment 14

[Kannan Vijayan [:djvj]]

Attachment: flyweb-internal-response-updates.patch

Delta patch with updates to the internal-response handling.

Comment 15

[Kannan Vijayan [:djvj]]

Landed the first set of changes in response to review:
http://hg.mozilla.org/projects/larch/rev/3f6da60bb3dd

Comment 16

[Kannan Vijayan [:djvj]]

Attachment: flyweb-internal-response.patch

Updated "full-diff" of InternalResponse changes between moz-central and flyweb branch.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8754502 [details] [diff] [review]
flyweb-internal-response-updates.patch

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

I'm just going to review the full patch again.  Thanks.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8754920 [details] [diff] [review]
flyweb-internal-response.patch

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

This looks much better, thanks!  I still think there are some things that need to be addressed, though.  Sorry I missed the issue with ReadStream::Available() in my previous review.

::: dom/cache/TypeUtils.cpp
@@ +304,5 @@
>    nsCOMPtr<nsIInputStream> stream = ReadStream::Create(aIn.body());
> +  uint64_t avail = 0;
> +  nsresult rv = stream->Available(&avail);
> +
> +  int64_t bodySize = NS_SUCCEEDED(rv) ? avail : -1;

Sorry, but a ReadStream does not have its full length known immediately.  It has to read the stream from its source (disk) and decompress it to get the real length.  So for anything longer than 32KB or so you will not get the right answer here.

I think for now you must specify that its an unknown length or implement storing the real length as meta data in the dom/cache sqlite database.

::: dom/fetch/Fetch.cpp
@@ +416,5 @@
>  {
>    aBuffer.ComputeLengthAndData();
> +  aContentLength = aBuffer.Length();
> +  if (!aContentLength) {
> +    return NS_OK;

This makes a zero-length array buffer act like a null stream.  Again, this is incorrect.

@@ +433,5 @@
>  {
>    aBuffer.ComputeLengthAndData();
> +  aContentLength = aBuffer.Length();
> +  if (!aContentLength) {
> +    return NS_OK;

Again, you can't treat 0-length as a null stream.

@@ +539,3 @@
>  {
>    MOZ_ASSERT(aStream);
> +  MOZ_ASSERT(!*aStream);

nit: Do we really need this assert?  We generally just rely on getter_AddRef() zero'ing the pointer for us automatically.  Right?

::: dom/fetch/FetchDriver.cpp
@@ +516,5 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        NS_WARNING("Failed to visit all headers.");
>      }
> +
> +    // Http-channels lie about their content-length sometimes

Please improve this comment to describe why we don't trust content-length in this situation.  Also please provide an NS_WARNING().

::: dom/fetch/InternalResponse.h
@@ +212,5 @@
>      }
>      // A request's body may not be reset once set.
>      MOZ_ASSERT(!mBody);
>      mBody = aBody;
> +    mBodySize = aBodySize;

Can we add a MOZ_ASSERT(mBodySize == UNKNOWN_SIZE || mBodySize >= 0) here?

@@ +282,5 @@
>    const uint16_t mStatus;
>    const nsCString mStatusText;
>    RefPtr<InternalHeaders> mHeaders;
>    nsCOMPtr<nsIInputStream> mBody;
> +  int64_t mBodySize;

Where do we document the meaning of -1?  Also, please make a constant for UNKNOWN_SIZE instead of using the -1 literal everywhere.

::: dom/fetch/Response.cpp
@@ +206,5 @@
>      }
>  
>      nsCOMPtr<nsIInputStream> bodyStream;
>      nsCString contentType;
> +    uint64_t bodySize;

Please initialize.

@@ +212,5 @@
> +                                    getter_AddRefs(bodyStream),
> +                                    contentType,
> +                                    bodySize);
> +    internalResponse->SetBody(bodyStream, bodySize);
> +    if (NS_WARN_IF(aRv.Failed())) {

This error checking should take place before the SetBody() call.

Comment 19

[Kannan Vijayan [:djvj]]

Pushed fixes relating to review comments: https://hg.mozilla.org/projects/larch/rev/2bd28b597d70

Comment 20

[Kannan Vijayan [:djvj]]

Attachment: flyweb-internal-response.patch

Updated amalgamated patch for internal-response changes.  Addresses all comments by bkelly in comment 18.

Forwarding r+ from baku, and asking for f? again from bkelly.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8756519 [details] [diff] [review]
flyweb-internal-response.patch

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

r=me with comments addressed.  Thanks!  Sorry for the delays reviewing.

::: dom/fetch/InternalResponse.h
@@ +195,5 @@
>      if (Type() == ResponseType::Opaque ||
>          Type() == ResponseType::Opaqueredirect) {
>        *aStream = nullptr;
> +      if (aBodySize) {
> +        *aBodySize = 0;

I think this should be UNKNOWN_BODY_SIZE instead of 0.  If a stream body is never set and we return nullptr in the unfiltered case we return a default of UNKNOWN_BODY_SIZE.  So I think we should do the same here.

@@ +212,4 @@
>      }
>      // A request's body may not be reset once set.
>      MOZ_ASSERT(!mBody);
> +    MOZ_ASSERT(mBodySize == UNKNOWN_SIZE || mBodySize >= 0);

Can you also MOZ_ASSERT(aBody) here.  I don't think there is any case where we should be setting a nullptr body stream.

@@ +285,5 @@
>    RefPtr<InternalHeaders> mHeaders;
>    nsCOMPtr<nsIInputStream> mBody;
> +  int64_t mBodySize;
> +public:
> +  static const int64_t UNKNOWN_SIZE = -1;

Thanks for adding this!  But please name it UNKNOWN_BODY_SIZE since its about the body.  Also, I think a comment indicating that we have this constant for cases where we don't know the body stream length up front, like chunked encoding, would be helpful.

::: dom/fetch/Response.cpp
@@ +207,5 @@
>  
>      nsCOMPtr<nsIInputStream> bodyStream;
>      nsCString contentType;
> +    uint64_t bodySize = 0;
> +    aRv = ExtractByteStreamFromBody(aBody.Value(),

It would be nice if this took a int64_t for aBodySize so we could initialize to UNKNOWN_BODY_SIZE.  I guess its unnecessary now, though, since we only support fixed length streams in the Response::Constructor().

Comment 22

[Kannan Vijayan [:djvj]]

(In reply to Ben Kelly [:bkelly] from comment #21)
> ::: dom/fetch/Response.cpp
> @@ +207,5 @@
> >  
> >      nsCOMPtr<nsIInputStream> bodyStream;
> >      nsCString contentType;
> > +    uint64_t bodySize = 0;
> > +    aRv = ExtractByteStreamFromBody(aBody.Value(),
> 
> It would be nice if this took a int64_t for aBodySize so we could initialize
> to UNKNOWN_BODY_SIZE.  I guess its unnecessary now, though, since we only
> support fixed length streams in the Response::Constructor().

Looking at the implementation of ExtractByteStreamFromBody, all of its uses currently seem to explicitly return known unsigned lengths.  It's probably better to fix this when it's possible for the lengths to be UNKNOWN than ahead of time.  Leaving it uint64_t for now at least documents the fact that ExtractByteStreamFromBody's implementation is never expected to return an UNKNOWN length.

All other changes have been made.  Thanks for your time!  Really appreciate the thorough feedback.

Comment 23

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

Comment on attachment 8756519 [details] [diff] [review]
flyweb-internal-response.patch

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

::: dom/fetch/FetchDriver.cpp
@@ +522,5 @@
> +    ErrorResult result;
> +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), result) ||
> +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), result)) {
> +      NS_WARNING("Cannot know response Content-Length due to presence of Content-Encoding "
> +                 "or Transfer-Encoding headers.");

I'm not sure that this warning is appropriate. It's generally much better to set those headers than to try to avoid them. I.e. it's much better to content-encoding:gzip than to not compress a response, and it's much better to use transfer-encoding:chunked than to wait for the whole response before sending it.

Web developers won't see this warning, so it's not a big deal though.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #23)
> I'm not sure that this warning is appropriate. It's generally much better to
> set those headers than to try to avoid them. I.e. it's much better to
> content-encoding:gzip than to not compress a response, and it's much better
> to use transfer-encoding:chunked than to wait for the whole response before
> sending it.

Would it make sense to limit the warning only if we're overriding the size provided via a content-length header?

Comment 25

[Kannan Vijayan [:djvj]]

(In reply to Ben Kelly [:bkelly] from comment #21)
> @@ +212,4 @@
> >      }
> >      // A request's body may not be reset once set.
> >      MOZ_ASSERT(!mBody);
> > +    MOZ_ASSERT(mBodySize == UNKNOWN_SIZE || mBodySize >= 0);
> 
> Can you also MOZ_ASSERT(aBody) here.  I don't think there is any case where
> we should be setting a nullptr body stream.

This ASSERT is actually failing..  see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da2fcd096be6&selectedJob=21585425

Stack:
10:45:22     INFO -  Assertion failure: aBody, at /builds/slave/try-lx-d-000000000000000000000/build/src/obj-firefox/dist/include/mozilla/dom/InternalResponse.h:210
10:45:49     INFO -  #01: mozilla::dom::InternalResponse::SetBody [dom/fetch/InternalResponse.h:210]
10:45:49     INFO -  #02: mozilla::dom::cache::TypeUtils::ToResponse [dom/cache/TypeUtils.cpp:286]
10:45:49     INFO -  #03: mozilla::dom::cache::CacheOpChild::HandleResponseList [xpcom/glue/nsTArray.h:361]
10:45:49     INFO -  #04: mozilla::dom::cache::CacheOpChild::Recv__delete__ [dom/cache/CacheOpChild.cpp:120]
10:45:49     INFO -  #05: mozilla::dom::cache::PCacheOpChild::OnMessageReceived [obj-firefox/ipc/ipdl/PCacheOpChild.cpp:161]
10:45:49     INFO -  #06: mozilla::ipc::PBackgroundChild::OnMessageReceived [obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1890]
10:45:49     INFO -  #07: mozilla::ipc::MessageChannel::DispatchAsyncMessage [ipc/glue/MessageChannel.h:553]
10:45:49     INFO -  #08: mozilla::ipc::MessageChannel::DispatchMessage [ipc/glue/MessageChannel.cpp:1593]
10:45:49     INFO -  #09: mozilla::ipc::MessageChannel::OnMaybeDequeueOne [ipc/glue/MessageChannel.cpp:1562]
10:45:49     INFO -  #10: nsRunnableMethodImpl<bool (mozilla::ipc::MessageChannel::*)(), false, true>::Run [xpcom/glue/nsThreadUtils.h:707]
10:45:49     INFO -  #11: mozilla::ipc::MessageChannel::DequeueTask::Run [ipc/glue/MessageChannel.h:477]
10:45:49     INFO -  #12: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1025]
10:45:49     INFO -  #13: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:290]
10:45:49     INFO -  #14: mozilla::dom::workers::WorkerPrivate::DoRunLoop [dom/workers/WorkerPrivate.cpp:4653]
10:45:49     INFO -  #15: WorkerThreadPrimaryRunnable::Run [dom/workers/RuntimeService.cpp:2673]
10:45:49     INFO -  #16: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1025]
10:45:49     INFO -  #17: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:290]
10:45:49     INFO -  #18: mozilla::ipc::MessagePumpForNonMainThreads::Run [ipc/glue/MessagePump.cpp:382]
10:45:49     INFO -  #19: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:235]
10:45:49     INFO -  #20: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:493]
10:45:49     INFO -  #21: nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:467]
10:45:49     INFO -  #22: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:219]
10:45:49     INFO -  #23: libpthread.so.0 + 0x6d4c

The issue seems to be that ReadStream::Create (https://dxr.mozilla.org/mozilla-central/source/dom/cache/ReadStream.cpp#436) can return nullptr, and its usage in TypeUtils::ToResponse (https://dxr.mozilla.org/mozilla-central/source/dom/cache/TypeUtils.cpp#283) passes the return value directly into SetBody without checking.

Not sure if this is an existing error caught by the new assert, or the new assert is over-strict.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kannan Vijayan [:djvj] from comment #25)
> Not sure if this is an existing error caught by the new assert, or the new
> assert is over-strict.

I'd be happy with either of these:

1) check body != nullptr before that SetBody()
2) make the assert MOZ_ASSERT_IF(!body, mBodySize == UNKNOWN_BODY_SIZE)

My main concern was that we didn't let a null body get set with a "known" body size.  That seemed bogus to me.

Comment 27

[Kannan Vijayan [:djvj]]

Attachment: flyweb-internal-response-updates.patch

New updates patch.  The M2 crash from the ASSERT on aBody is fixed with this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d37d55f0dbb

Forwarding r+ to it.

Comment 28

[Kannan Vijayan [:djvj]]

http://hg.mozilla.org/projects/larch/rev/39dcfacc4b5b

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9020ac239e2a
Changes in preparation for FlyWeb landing.  Change InternalResponse handling to track body size. r=baku f=bkelly

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9020ac239e2a