Calling FetchEvent.respondWith() with a malformed synthesized redirected response causes us to hit the network

NEW
Unassigned

Status

()

defect
P3
normal
4 years ago
2 years ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

If a service worker does something like:

self.onfetch = (e) => {
  e.respondWith(new Response("foo", {status: 308}));
};

We incorrectly end up hitting the network.

Josh said he would fix this.
This fixes a hole where a synthesized cache entry is ignored if it has a redirection HTTP status but does not include a corresponding header. Since we never want to revalidate the cache entry, this seems like the easiest way to ignore the problem.
Attachment #8614836 - Flags: review?(honzab.moz)
Comment on attachment 8614836 [details] [diff] [review]
Ignore redirect errors for synthesized cache entries

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

r=honzab

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2961,5 @@
>      // Do not return 304 responses from the cache, and also do not return
>      // any other non-redirect 3xx responses from the cache (see bug 759043).
>      NS_ENSURE_TRUE((mCachedResponseHead->Status() / 100 != 3) ||
> +                   isCachedRedirect ||
> +                   mInterceptCache == INTERCEPTED, NS_ERROR_ABORT);

update also the comment before this ENSURE.
Attachment #8614836 - Flags: review?(honzab.moz) → review+
This passes half of the test in e10s - the status is correct, but the response body isn't. Apparently the cache believes we're still writing to the entry if we don't close the output stream before trying to use the entry.
Honza, when I first made the changes in bug 1137287, you suggested that sending the body to the parent isn't necessary (comment 14). It looks to me like it is. Do you have a better solution?
Flags: needinfo?(honzab.moz)
(In reply to Josh Matthews [:jdm] from comment #6)
> Created attachment 8615593 [details] [diff] [review]
> Ignore redirect errors for synthesized cache entries
> 
> This passes half of the test in e10s - the status is correct, but the
> response body isn't. 

It's empty or wrong?

> Apparently the cache believes we're still writing to
> the entry if we don't close the output stream before trying to use the entry.

Yes, that is the design.  Until you close the output reading pumping the input will never finish.

(In reply to Josh Matthews [:jdm] from comment #7)
> Honza, when I first made the changes in bug 1137287, you suggested that
> sending the body to the parent isn't necessary (comment 14). 

If you only need it for the headers/response check then I still believe you don't need the body.

> It looks to me
> like it is. Do you have a better solution?

I first need to understand the reason you would want to send the body up and down.  I need more info (not much provided in the bug).  Feel free to catch me on IRC too.
Flags: needinfo?(honzab.moz) → needinfo?(josh)
(In reply to Honza Bambas (:mayhemer) from comment #8)
> Yes, that is the design.  Until you close the output reading pumping the
> input will never finish.

I.e. any input stream that has reached EOF returns WOULD_BLOCK.
(In reply to Honza Bambas (:mayhemer) from comment #8)
> (In reply to Josh Matthews [:jdm] from comment #6)
> > Created attachment 8615593 [details] [diff] [review]
> > Ignore redirect errors for synthesized cache entries
> > 
> > This passes half of the test in e10s - the status is correct, but the
> > response body isn't. 
> 
> It's empty or wrong?

It's empty. Since we create a cache entry in the parent with no body, the channel in the child receives IPC messages with no body.

> (In reply to Josh Matthews [:jdm] from comment #7)
> > Honza, when I first made the changes in bug 1137287, you suggested that
> > sending the body to the parent isn't necessary (comment 14). 
> 
> If you only need it for the headers/response check then I still believe you
> don't need the body.

When I write an SJS that returns a 308 with no additional headers, and contains a non-empty response body, an XHR to that SJS will have a 308 status attribute and a corresponding non-empty responseText. When a service worker intercepts an XHR request and responds with a Response object with a 308 status and a non-empty body, the behaviour should be the same. Right now we send the headers and status code to the parent for processing when the synthesized response is not 200 or 404; this goes through the regular IPC channel behaviour which tells the XHR in the child that there is an empty body. Does that make sense?
Flags: needinfo?(josh)
Blocks: ServiceWorkers-postv1
No longer blocks: ServiceWorkers-v1
Status: NEW → ASSIGNED
Attachment #8614836 - Attachment is obsolete: true
This is edge-casey enough that I don't believe it's worth addressing before we fix bug 1231222, since it will incur additional technical debt in order to do so.
Summary: Calling FetchEvent.respondWith() with a synthesized redirected response causes us to hit the network → Calling FetchEvent.respondWith() with a malformed synthesized redirected response causes us to hit the network
Assignee: josh → nobody
Status: ASSIGNED → NEW
Oh, I missed that it was malformed!  Sorry for raising the priority of this again.  I agree it can wait.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.