Closed
Bug 1288915
Opened 9 years ago
Closed 9 years ago
File Download Fails Using Service Worker and Gzip
Categories
(Core :: DOM: Service Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: chris, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Whiteboard: [specification][type:bug])
Attachments
(2 files)
1.13 KB,
patch
|
jdm
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.06 KB,
patch
|
jdm
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
What did you do?
================
1. https://sw-download-test.threatx.io/ in Firefox
2. Click #6 "Download (Problematic in Firefox)"
What happened?
==============
I receive error "[FILEPATH].part could not be saved, because the source file could not be read."
What should have happened?
==========================
#6 download should have saved file successfully like #1-5 work.
Is there anything else we should know?
======================================
The use case is streaming large (5-10MB) dynamic txt/json/csv files to users with Gzip on a ServiceWorker-enabled site. For better UX, I don't want to compress server-side and deliver .zip files to users. Works great in Chrome, but not in Firefox.
I posted the source and server configuration on Github. This problem also occurs on a private server that uses Python as a back end to stream a file with "Content-Disposition: attachment". Therefore, this should not be caused by PHP nor Python, unless certain HTTP headers are missing.
Also works locally using http://localhost, but I can't publish that test remotely.
Updated•9 years ago
|
Component: General → Untriaged
Product: Mozilla Developer Network → Firefox
Comment 1•9 years ago
|
||
I cannot open the URL because the hostname cannot be resolved.
is the page still available?
Flags: needinfo?(chris)
My apologies, I accidentally posted the wrong url. Please use the URL below and correct the description?
https://sw-download-test.anthum.com/
Comment 3•9 years ago
|
||
thanks :)
this seems to be a regression from bug 1261784.
Blocks: 1261784
Status: UNCONFIRMED → NEW
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox-esr45:
--- → unaffected
Component: Untriaged → DOM: Service Workers
Ever confirmed: true
Flags: needinfo?(chris)
Keywords: regression
Product: Firefox → Core
Assignee | ||
Comment 4•9 years ago
|
||
We are trying to run the Content-Disposition download app handler stuff from the fetch() initiated in the SW. This is wrong. We need to skip Content-Disposition handling just like XHR does.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
Assignee | ||
Updated•9 years ago
|
Blocks: ServiceWorkers-compat
Flags: needinfo?(bkelly)
Updated•9 years ago
|
Version: unspecified → 46 Branch
Assignee | ||
Comment 5•9 years ago
|
||
By the way, I don't buy that this is caused by bug 1261784. The failure may have been masked before that fix.
Comment 6•9 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #5)
> By the way, I don't buy that this is caused by bug 1261784. The failure may
> have been masked before that fix.
Sorry for misassumption.
and thank you for taking this :)
Thank you everyone for looking into this. Meanwhile, I'll use the following logic to bypass service workers on certain URLs
self.addEventListener('fetch', function(event) {
if (/exclude-uri-pattern/.test(event.request.url)) {
// don't use service worker for these URLs
} else {
// invoke service worker logic for all other URLs
event.respondWith(
..
...
....
);
}
});
Assignee | ||
Comment 8•9 years ago
|
||
This actually triggers an assertion in debug builds in e10s mode.
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #4)
> We are trying to run the Content-Disposition download app handler stuff from
> the fetch() initiated in the SW. This is wrong. We need to skip
> Content-Disposition handling just like XHR does.
This was incorrect. The Content-Disposition is being handled in the outer docshell initiated channel, but nsExternalAppHandler is overriding the channel's ApplyConversion flag here:
https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1600
Assignee | ||
Comment 10•9 years ago
|
||
This fixes the problem for me locally. I need to write a test.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 11•9 years ago
|
||
This test fails without the P1 and succeeds with P1 applied.
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8774839 [details] [diff] [review]
P1 Make nsExternalAppHandler respect existing channel ApplyConversion flag. r=jdm
A Response provided by a service worker is already decoded. The nsExternalAppHandler logic, however, can reset the ApplyConversion flag on the channel when trying to figure out if decoding should be applied for other reasons.
This patch makes nsExternalAppHandler short-circuit its logic if decoding is already disabled on the channel.
Attachment #8774839 -
Flags: review?(josh)
Assignee | ||
Updated•9 years ago
|
Attachment #8774851 -
Flags: review?(josh)
Updated•9 years ago
|
Attachment #8774839 -
Flags: review?(josh) → review+
Updated•9 years ago
|
Attachment #8774851 -
Flags: review?(josh) → review+
Comment 14•9 years ago
|
||
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d480d30a74c3
P1 Make nsExternalAppHandler respect existing channel ApplyConversion flag. r=jdm
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b2d8a0999e
P2 Verify that synthetic download works with a Content-Encoding header set. r=jdm
Comment 15•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d480d30a74c3
https://hg.mozilla.org/mozilla-central/rev/80b2d8a0999e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8774839 [details] [diff] [review]
P1 Make nsExternalAppHandler respect existing channel ApplyConversion flag. r=jdm
Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: This a bad compat bug that breaks sites using compressed downloads and service workers. These sites may be tested with chrome, but will break in firefox today. We should ship this as soon as possible.
[Describe test coverage new/current, TreeHerder]: This bug includes a change to an existing test to cover the compressed header case.
[Risks and why]: Minimial.
[String/UUID change made/needed]: None.
Attachment #8774839 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8774851 [details] [diff] [review]
P2 Verify that synthetic download works with a Content-Encoding header set. r=jdm
Comment 21.
Attachment #8774851 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•9 years ago
|
||
Comment 19•9 years ago
|
||
Comment on attachment 8774839 [details] [diff] [review]
P1 Make nsExternalAppHandler respect existing channel ApplyConversion flag. r=jdm
bad web compat, taking it.
Attachment #8774839 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8774851 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•9 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 21•9 years ago
|
||
Thank you all so much for fixing this. Have a great weekend!
Assignee | ||
Comment 22•9 years ago
|
||
Based on the fix I'm confident we always had this bug when service workers were in use.
No longer blocks: 1261784
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•