If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

Camino Graveyard
Downloading
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: philippe (part-time), Assigned: cpeterson)

Tracking

1.9.2 Branch
x86
Mac OS X

Details

(Whiteboard: [camino-2.0.6])

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
Created attachment 483393 [details]
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.
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).
Keywords: regression
(Assignee)

Comment 2

7 years ago
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
(Assignee)

Updated

7 years ago
Assignee: nobody → mozilla.org
Status: NEW → ASSIGNED
(Assignee)

Comment 3

7 years ago
Created attachment 484244 [details] [diff] [review]
download-completed-v1.patch

When the file download is complete, the download progress should match the downloaded file size.
Attachment #484244 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #484244 - Flags: review? → review?(ishermandom+bugs)
(Assignee)

Updated

7 years ago
Keywords: regression
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.
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)?
You could try creating an HTTP log for the steps in comment 4 and seeing what it looks like, as a start....
(Assignee)

Comment 7

7 years ago
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.
Created attachment 484432 [details]
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…
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.
Oh, and that's using chunked transfer-encoding, which also means no content-length and suggests that streaming compression is what's happening.
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 ;)
> 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

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

Looks great, thanks for the fix =)

r+=ilya
Attachment #484244 - Flags: superreview?(mikepinkerton)
Attachment #484244 - Flags: review?(ishermandom+bugs)
Attachment #484244 - Flags: review+
Comment on attachment 484244 [details] [diff] [review]
download-completed-v1.patch

sr=pink
Attachment #484244 - Flags: superreview?(mikepinkerton) → superreview+
http://hg.mozilla.org/camino/rev/2c7364a46d03
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Flags: camino2.0.6?
Landed on CAMINO_2_0_BRANCH in advance of 2.0.6.
Flags: camino2.0.6? → camino2.0.6+
Whiteboard: [camino-2.0.6]
You need to log in before you can comment on or make changes to this bug.