1288915 - File Download Fails Using Service Worker and Gzip

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[chris]

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.

Comment 1

[Tooru Fujisawa [:arai]]

I cannot open the URL because the hostname cannot be resolved.
is the page still available?

Comment 2

[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

[Tooru Fujisawa [:arai]]

thanks :)

this seems to be a regression from bug 1261784.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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

[Tooru Fujisawa [:arai]]

(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 :)

Comment 7

[chris]

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(
      ..
      ...
      ....
    );
  }
});

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

This actually triggers an assertion in debug builds in e10s mode.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make nsExternalAppHandler respect existing channel ApplyConversion flag. r=jdm

This fixes the problem for me locally.  I need to write a test.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Verify that synthetic download works with a Content-Encoding header set. r=jdm

This test fails without the P1 and succeeds with P1 applied.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d480d30a74c3
https://hg.mozilla.org/mozilla-central/rev/80b2d8a0999e

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8774851 [details] [diff] [review]
P2 Verify that synthetic download works with a Content-Encoding header set. r=jdm

Comment 21.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #17)
> Comment 21.

I mean comment 16.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8774839 [details] [diff] [review]
P1 Make nsExternalAppHandler respect existing channel ApplyConversion flag. r=jdm

bad web compat, taking it.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bfb16f4837b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef0e06a0e51a

Comment 21

[chris]

Thank you all so much for fixing this.  Have a great weekend!

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Based on the fix I'm confident we always had this bug when service workers were in use.