1264181 - Cache API should block 206 responses

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

One part of fixing range response handling is preventing 206 responses from being stored in Cache API:

  https://github.com/slightlyoff/ServiceWorker/issues/703#issuecomment-175273497

There are still some questions about all of this, but blocking those now seems safe and reasonable.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

https://github.com/slightlyoff/ServiceWorker/issues/937

Comment 2

[Tom Tung [:tt, :ttung]]

I would like to take this bug.

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: Part 1: Cache API should block 206 response.

Hi Ben,

I implement this patch for blocking 206 response in add(), addAll() and put(). Could you help me to review this patch? Thanks!

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: Part 2: Add WPT tests for blocking 206 response.

Patch for WPT test. Could you help me to review this patch as well, Ben? Thanks!

Comment 5

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=605d42e7c75b86935f360dc3a0c77e6933f4354e&selectedJob=32803894

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8819192 [details] [diff] [review]
Part 1: Cache API should block 206 response.

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

r=me with comments addressed.

::: dom/cache/Cache.cpp
@@ +182,5 @@
> +      if (response->Status() == 206) {
> +        nsAutoString status;
> +        status.AppendInt(response->Status());
> +        ErrorResult rv;
> +        rv.ThrowTypeError<MSG_CACHE_INVALID_RESPONSE_STATUS>(status);

Can you just use MSG_CACHE_ADD_FAILED_RESPONSE instead?  It pretty much says the correct thing and also includes the request URL which is helpful for context.

@@ +418,5 @@
>  
> +  if (NS_WARN_IF(aResponse.Status() == 206)) {
> +    nsAutoString status;
> +    status.AppendInt(aResponse.Status());
> +    aRv.ThrowTypeError<MSG_CACHE_INVALID_RESPONSE_STATUS>(status);

Can you make a helper method like IsValidPutRequestMethod() that performs this check and throws the TypeError if necessary?  We could call it IsValidPutResponseStatus().

https://dxr.mozilla.org/mozilla-central/source/dom/cache/Cache.cpp#58

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8819193 [details] [diff] [review]
Part 2: Add WPT tests for blocking 206 response.

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

r=me with comments addressed.

::: testing/web-platform/tests/service-workers/cache-storage/script-tests/cache-add.js
@@ +82,5 @@
>    }, 'Cache.add with request with null body (not consumed)');
>  
>  cache_test(function(cache) {
>      return assert_promise_rejects(
> +      cache.add('../resources/fetch-status.php?status=206'),

This seems wrong.  We don't use .php files.  We should have a .py instead:

https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/service-workers/cache-storage/resources/fetch-status.py

So please change these to fetch-status.py?status=206.

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: Part 1: Cache API should block 206 response. r=bkelly.

(In reply to Ben Kelly [:bkelly] from comment #6)
Thanks for the reviews!

> @@ +418,5 @@
> >  
> > +  if (NS_WARN_IF(aResponse.Status() == 206)) {
> > +    nsAutoString status;
> > +    status.AppendInt(aResponse.Status());
> > +    aRv.ThrowTypeError<MSG_CACHE_INVALID_RESPONSE_STATUS>(status);
> 
> Can you make a helper method like IsValidPutRequestMethod() that performs
> this check and throws the TypeError if necessary?  We could call it
> IsValidPutResponseStatus().
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/cache/Cache.cpp#58

Sure, but is it only called by Cache::Put()? I used to think about it, but the helper function will be only used in Cache::Put(). Thus, I decided to remove it in previous patch(Part 1, Version 0). 

Could you help me to review this patch again? Thanks! I'm not sure my patch address your comment.

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Part 2: Add WPT tests for blocking 206 response. r=bkelly.

Address the comment!

(In reply to Ben Kelly [:bkelly] from comment #7)
> So please change these to fetch-status.py?status=206.

Thanks for pointing out using wrong file. I didn't notice that...

I also correct the other test which calls .php as well.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=031a4c9b63dc909e8bae3a8894db0d48ac56b047

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820112 [details] [diff] [review]
Part 1: Cache API should block 206 response. r=bkelly.

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

::: dom/cache/Cache.cpp
@@ +177,5 @@
>        // Do not allow the convenience methods .add()/.addAll() to store failed
> +      // or invalid responses.  A consequence of this is that these methods
> +      // cannot be used to store opaque or opaqueredirect responses since they
> +      // always expose a 0 status value.
> +      if (!response->Ok() || response->Status() == 206) {

Hmm, I'd prefer to keep all the status checking code in that IsValidPutResponseStatus() check.

Can you maybe change it to be:
  enum class PutStatusPolicy {
    Default,
    RequireOK
  };

  bool
  IsValidPut ResponseStatus(Response* aResponse, PutStatusPolicy aPolicy,
                            ErrorResult& aRv)
  {
    // Check !Ok() if aPolicy == PutStatusPolicy::RequireOK
  }

Then you can just replace this whole block with:

  if (!IsValidPutResponseStatus(response, PutStatusPolicy::RequireOK, rv)

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Part 1: Cache API should block 206 response.

(In reply to Ben Kelly [:bkelly] from comment #10)
Thanks for feedback! I address the comment. 

Could you help me to review it? Thanks!

Comment 12

[Tom Tung [:tt, :ttung]]

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a06f69252364d40f5dc1b8682af57ce6a78ad9c9&selectedJob=33175016

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8820529 [details] [diff] [review]
Part 1: Cache API should block 206 response.

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

Thanks!  Just a couple nits.  r=me

::: dom/cache/Cache.cpp
@@ +187,5 @@
> +      // or invalid responses.  A consequence of this is that these methods
> +      // cannot be used to store opaque or opaqueredirect responses since they
> +      // always expose a 0 status value.
> +      ErrorResult errorResult;
> +      if (NS_WARN_IF(!IsValidPutResponseStatus(*response,

Please remove this NS_WARN_IF().  Failed network requests are pretty common and we have the console message.

@@ +422,5 @@
>    if (NS_WARN_IF(!IsValidPutRequestMethod(aRequest, aRv))) {
>      return nullptr;
>    }
>  
> +  if (NS_WARN_IF(!IsValidPutResponseStatus(aResponse, PutStatusPolicy::Default,

Again, please remove the NS_WARN_IF().  We have the console message for this.

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Part 1: Cache API should block 206 response. r=bkelly.

(In reply to Ben Kelly [:bkelly] from comment #13)

Thanks for the review! Addressed the comment!

Comment 15

[Tom Tung [:tt, :ttung]]

Comment on attachment 8820113 [details] [diff] [review]
[Final] Part 2: Add WPT tests for blocking 206 response. r=bkelly.

Update the description to mark as "[Final]" since it has already gotten r+.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84fc5342b9b7
Part 1: Cache API should block 206 response. r=bkelly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c48b87e3ee4
Part 2: Add WPT tests for blocking 206 response. r=bkelly.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/84fc5342b9b7
https://hg.mozilla.org/mozilla-central/rev/9c48b87e3ee4