Open
Bug 1170795
Opened 9 years ago
Updated 2 years ago
Calling FetchEvent.respondWith() with a malformed synthesized redirected response causes us to hit the network
Categories
(Core :: DOM: Service Workers, defect, P3)
Core
DOM: Service Workers
Tracking
()
NEW
People
(Reporter: ehsan.akhgari, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
4.57 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Blocks: ServiceWorkers-v1
Comment 1•9 years ago
|
||
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 2•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
Backed out for e10s test_fetch_event.html failures. https://treeherder.mozilla.org/logviewer.html#?job_id=10430633&repo=mozilla-inbound
Comment 6•9 years ago
|
||
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.
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
(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)
Comment 9•9 years ago
|
||
(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.
Comment 10•9 years ago
|
||
(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)
Reporter | ||
Updated•9 years ago
|
Updated•9 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Blocks: ServiceWorkers-compat
Updated•8 years ago
|
Attachment #8614836 -
Attachment is obsolete: true
Comment 11•8 years ago
|
||
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.
Updated•8 years ago
|
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
Updated•8 years ago
|
Assignee: josh → nobody
Status: ASSIGNED → NEW
Comment 12•8 years ago
|
||
Oh, I missed that it was malformed! Sorry for raising the priority of this again. I agree it can wait.
Updated•7 years ago
|
No longer blocks: ServiceWorkers-postv1
Updated•6 years ago
|
Priority: -- → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•