281648 - "Download Link Target" results in bad download for large MP3 file.

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Mike Pinkerton (not reading bugmail)]

from user feedback:

go to
http://www.boardgamegeek.com/geekjournal.php3?action=viewcomments&journalid=6383
and download either of the split MP3s (using Download Link Target)

expected:
- once its on the desktop, it should open fine

actual:
- doesn't open in iTunes, though it appears to have the correct size in the
finder. report says it works in firefox.

Comment 1

[Simon Fraser [no longer active]]

I think this depends on whether you use 'Download link target' or just click on
the MP3 link. 'Download link target' gives me a bad file, but clicking on the
link gives me a working file.

Comment 2

[Josh Aas]

upping the priority

Comment 3

[Simon Fraser [no longer active]]

Here are the HTTP headers for the download:
HTTP/1.1 200 OK
Date: Sat, 26 Feb 2005 22:33:19 GMT
Server: Apache/2.0.52 (Fedora)
Last-Modified: Wed, 02 Feb 2005 06:03:20 GMT
ETag: "3dc00d-21f5030-3ef1b01979a00"
Accept-Ranges: bytes
Vary: Accept-Encoding,User-Agent
Content-Encoding: gzip
Connection: close
Transfer-Encoding: chunked
Content-Type: audio/mp3


I think the stream is gzipped, but we're not unzipping on the way down.

Comment 4

[Simon Fraser [no longer active]]

Yup, I zcat even a partially downloaded file to get something playable.

Comment 5

[Simon Fraser [no longer active]]

Attachment: Patch: consult nsIExternalHelperAppService before setting PERSIST_FLAGS_NO_CONVERSION

This patch fixes the bug by only setting the PERSIST_FLAGS_NO_CONVERSION flag
if  nsIExternalHelperAppService::ApplyDecodingForExtension() says we should not
decode (it just looks at the file extension to see if its .zip, .gz etc). This
is what the JS in moz/ff does.

Also null out mPersist when we're done with it.

Comment 6

[Darin Fisher]

Comment on attachment 175670 [details] [diff] [review]
Patch: consult nsIExternalHelperAppService before setting PERSIST_FLAGS_NO_CONVERSION

This seems right to me, sr=darin

Comment 7

[Simon Fraser [no longer active]]

Maybe we should use PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION...

Comment 8

[Josh Aas]

darin - could you comment on smfr's suggestion?

Comment 9

[Darin Fisher]

(In reply to comment #7)
> Maybe we should use PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION...

can you explain why?  i'm open to whatever... just motivate the change :)

Comment 10

[Simon Fraser [no longer active]]

> can you explain why

Because someone mentioned that this functionality had been rolled into
nsWebBrowserPersist (based on that flag).

Comment 11

[Simon Fraser [no longer active]]

Attachment: Simpler patch

Comment 12

[Mike Pinkerton (not reading bugmail)]

this should also go on the 083 branch. it's been around for a while. 

Comment 13

[Mike Pinkerton (not reading bugmail)]

r=pink, but just curious if this will cause other unintended side effects to
suddenly start doing conversion on things.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 176098 [details] [diff] [review]
Simpler patch

sr=pink

also fixes the bug where you save the html at www.google.com and instead of
saving html we'd get garbage (the gzip'd contentes).

Comment 15

[Josh Aas]

landed on trunk

Comment 16

[Mike Pinkerton (not reading bugmail)]

which of these patches is ok for the 176branch? darin?

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

"simpler patch" won't compile on the 1.7 branch; that flag is trunk-only.

Comment 18

[Mike Pinkerton (not reading bugmail)]

that's what i figured. so is the previous patch ok for branch checkin?