File Download Fails Using Service Worker and Gzip

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: chris, Assigned: bkelly)

Tracking

(Blocks 1 bug)

46 Branch
mozilla50
All
Other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 wontfix, firefox49 fixed, firefox-esr45 disabled, firefox50 fixed)

Details

(Whiteboard: [specification][type:bug])

Attachments

(2 attachments)

Reporter

Description

3 years ago
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.
Component: General → Untriaged
Product: Mozilla Developer Network → Firefox
I cannot open the URL because the hostname cannot be resolved.
is the page still available?
Flags: needinfo?(chris)
Reporter

Comment 2

3 years ago
My apologies, I accidentally posted the wrong url. Please use the URL below and correct the description?

https://sw-download-test.anthum.com/
thanks :)

this seems to be a regression from bug 1261784.
Blocks: 1261784
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Service Workers
Ever confirmed: true
Flags: needinfo?(chris)
Keywords: regression
Product: Firefox → Core
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)
Flags: needinfo?(bkelly)
Version: unspecified → 46 Branch
By the way, I don't buy that this is caused by bug 1261784.  The failure may have been masked before that fix.
(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 :)
Reporter

Comment 7

3 years ago
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(
      ..
      ...
      ....
    );
  }
});
This actually triggers an assertion in debug builds in e10s mode.
(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
This fixes the problem for me locally.  I need to write a test.
This test fails without the P1 and succeeds with P1 applied.
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)
Attachment #8774851 - Flags: review?(josh)
Attachment #8774839 - Flags: review?(josh) → review+
Attachment #8774851 - Flags: review?(josh) → review+

Comment 14

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d480d30a74c3
https://hg.mozilla.org/mozilla-central/rev/80b2d8a0999e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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?
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?
(In reply to Ben Kelly [:bkelly] from comment #17)
> Comment 21.

I mean comment 16.
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+
Attachment #8774851 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter

Comment 21

3 years ago
Thank you all so much for fixing this.  Have a great weekend!
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.