604582 - downloaded file labeled as 'canceled' after closing the browser and restarting.

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[philippe (part-time)]

Attachment: screenshot

I have so far mostly seen this while saving patches locally from bugzilla.

STR:

1. save a patch locally (Cmd S)
2. Close the browser
3. restart the browser

AR: the just saved file is marked as 'canceled' in the download manager.
ER: Labeled 'completed in 00:00 (xx KB)'

Saving the patch(es) in bug 506345 have reproduced it always.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I've seen this, too, for a while, and thought I was going insane.  I couldn't reliably reproduce it when I talked to Ilya about it a while ago, but I don't think I was trying Bugzilla attachments then :P

Nick or Ilya, any suggestions on where to look at this (and/or stuff to log)?

I presume this doesn't happen in 2.0.x, but I haven't tested (it certainly didn't used to).

Comment 2

[Chris Peterson [:cpeterson]]

This problem happens when the server's HTTP response does not include a Content-Length header. When Camino saves the download manager state (in ~/Library/Application Support/Camino/downloads.plist), it includes the downloaded file size (from disk) and the most recent download progress (which is less than 100%). So when Camino restarts, it believes the file was not completely downloaded.

Apparently, bugzilla.mozilla.org does not include a Content-Length header for patch files.

I can reproduce this with Camino 2.1a1pre, 2.0.4, 1.6.11, 1.5.5, and 1.0.6!

The following debug message in Xcode's debugger console window pointed me to the problem:

> WARNING: nsWebBrowserPersist::OnDataAvailable() no content length header, pushing what we have: 'channelContentLength != -1', file /Users/chris/Code/mozilla/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp, line 907

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: download-completed-v1.patch

When the file download is complete, the download progress should match the downloaded file size.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Thanks for figuring this out, Chris!  (It seems to be all text files, since I can repro the "cancelled" state with the sample in bug 530026, too.)

The lack of content-length header getting sent for text files appears to be a regression in bmo (it didn't use to happen here, and it doesn't happen on landfill's 3.6 branch install), so I'll file a bug on that.

We should go ahead and protect ourselves from it for any other servers/future cases where it might appear, though.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Actually, curl is showing me content-length headers for the attachments (for their final "bugNNNN.b.m.o" URLs), but in Camino even when I load the attachment directly and Cmd-S (so that we already have/are at the final URL and there's/there-in-theory-should-be no redirect in the save process), I still see a cancelled notice on that completed download when I restart Camino.

So now I'm less sure it's a bmo bug (although given we can repro it all the way back to 1.0.6 and we never used to see it, it still seems likely, as well as that it only affects text file types) or if it's some sort of edge-case Gecko bug that bmo changes exposed.

Boris, do you have any thoughts here (see also comment 2 and comment 4); there's a UI issue that's a Camino bug regardless, but there's something somewhere else that caused our bug to appear, and I'd like to file that something else where appropriate)?

Comment 6

[Boris Zbarsky [:bzbarsky]]

You could try creating an HTTP log for the steps in comment 4 and seeing what it looks like, as a start....

Comment 7

[Chris Peterson [:cpeterson]]

When Gecko does not know the final Content-Length, it sends Camino downloads progress events that say "downloaded X bytes of a file of unknown size". But when the download is complete, I think Gecko sends us a "download completed" event without a progress event for the final download progress (or file size).

Maybe that's a Gecko optimization to avoid broadcasting "unnecessary" events, but it does seem like an awkward special case for apps embedding Gecko.

Camino's ProgressViewController.mm might be able to remove some special case code if we simulated a final progress event with final file size (updating download progress to 100%) before processing the "download completed" event.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: annotated nsHttp:3 log

I believe this HTTP log confirms that Bugzilla/bmo is, in fact, not sending content-length headers for text (patch) attachments.  (I had to turn everything off and scale the log level down to 3 to eliminate 100s of KB of noise; this logging seems to be equivalent to what the Live HTTP Headers extension shows in Firefox, and the headers appear to match what I see in a recent Fx 3.6.x nightly.)

So I'm not sure how curl was ever getting a content-length header; maybe curl doesn't request gzip, and Bugzilla/bmo only gives non-gzipped attachments a content-length header?

Landfill doesn't do gzip, either, so that does make gzip-on-text-attachments on bmo seem more likely to be the culprit…

Comment 9

[Boris Zbarsky [:bzbarsky]]

If it's doing streaming gzip on the server it may have no way to send a content-length header, since the final compressed length is not known when you start the response.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and that's using chunked transfer-encoding, which also means no content-length and suggests that streaming compression is what's happening.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

That means there's nothing bmo can do (other than choose to stop using chunked transfer-encoding and streaming compression) to "fix" the lack of content-length header, right?  If that's the case, I won't bother filing a bug against bmo ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> That means there's nothing bmo can do (other than choose to stop using chunked
> transfer-encoding and streaming compression) to "fix" the lack of
> content-length header, right? 

That's correct.

Comment 13

[Ilya Sherman]

Comment on attachment 484244 [details] [diff] [review]
download-completed-v1.patch

Looks great, thanks for the fix =)

r+=ilya

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 484244 [details] [diff] [review]
download-completed-v1.patch

sr=pink

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/2c7364a46d03

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on CAMINO_2_0_BRANCH in advance of 2.0.6.