164917 - Download dialog can't be closed after saving page as HTML Complete

Status: VERIFIED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Chris Petersen]

Build: 2002-08-27-05
Platform: OS X 10.2
Expected Results: After download is completed (progress indicator is full),
dialog should close

What I got: The progress indication is full after transfer is completed, but the
dialog will remain open. Clicking cancel icon or red close button doesn't close
the dialog. When the dialog is in this state, the status and elapsed time will
continue to update even though the progress indicator is "full".

Steps to reproduce: 

1) Go to a url like mozilla.org.
2) Choose Save As from the File menu.
3) Specify a location and click Save.
4) Download dialog appears and completes the transfer (progress indicator).
5) D/L dialog remains open.

Comment 1

[Mike Pinkerton (not reading bugmail)]

-> brade

i think this regressed from the branch update

Comment 2

[sairuh (rarely reading bugmail)]

this is not a problem with the 8/26 verif bits, so the regression occurred btwn
then and today's bits. another thing i noticed when doing the "save as complete"
is that while the subsidiary files were saved, the main html page was not. also,
the "elapsed time" value keeps going (incrementing a second at a time).

Comment 3

[Chris Petersen]

Confirming Sarah's comment too.

Comment 4

[sairuh (rarely reading bugmail)]

actually, i take back (for the nonce) what i said about it regressing yesterday.
it might be older.

also, it seems that this occurs under more specific conditions: if "save as
complete" is the *first* downloading attempt during a chimera session, then
you'll encounter this bug.

however, if you download by another means beforehand in the same session ("save
as text", "save as source only", "download link target"), then "save as
complete" will eventually work. i'm trying to figure out *which* particular
download means will cause "save as complete" to succeed --it's currently appears
erratic. :-/

Comment 5

[sairuh (rarely reading bugmail)]

could this be related to bug 164924?

Comment 6

[sairuh (rarely reading bugmail)]

btw, chris cannot repro this on 0.40.

Comment 7

[Mike Pinkerton (not reading bugmail)]

this happened before 8/26, i saw it last week just never got around to reporting it.

Comment 8

[sairuh (rarely reading bugmail)]

yeah, it regressed sometime btwn the 8/23 (works) and 8/26 (broken) builds. will
narrow some more.

re comment 5 and comment 6: bug 164924 might not be related after all...

Comment 9

[sairuh (rarely reading bugmail)]

this became broken starting with the 2002.08.24.05 build.

Comment 10

[Tracy Walker [:tracy]]

this is worse than just the dialog hanging; upon quiting the app, the icon for
the saved page disappears, leaving just the folder for the associated page data.  

note that while the dialog is up, the status indicates downloading at a
progressively slower and slower rate. After 15 minutes of waiting for a 2.8KB
download to complete the speed has slowed to 3.0 bytes/sec.

was going to upgrade to a smoketest blocker, but sairah seems onto a workaround.
so just making critical because of the data loss.

Comment 11

[sairuh (rarely reading bugmail)]

fwiw, i tried some comparison testing with the 8/20 mach-o mozilla build.
unfortunately, saving didn't work at all, so no useful info there. "save as
complete", "save as text", "save only source" and "save link target" didn't save
anything.

Comment 12

[Simon Fraser [no longer active]]

0.5 blocker.

Comment 13

[Winnie Lam]

This bug results in dataloss, per comment #10. Agreed, this would be a 0.5
blocker.  What would the fix involve and when do you think we'd have it? 

Adding sdagley and ccarlen to cc:list since they've worked on downloading.

Comment 14

[Kathleen :Brade]

adding ccarlen and sdagley for winnie (see above comment)

Comment 15

[Steve Dagley]

The file deletion part sounds as if somehow the helper app/temp file download
code in uriloader is being invoked which is bad because the current version
deletes these files when the app exits.  I have a patch sitting in #151047 which
fixes that behavior but it wasn't landed when sfraser checked in the other parts
of that patch.  The real question is how did that code path get executed as it
shouldn't be as part of doing a Save As from the File menu

Comment 16

[Winnie Lam]

Steve, would the patch in bug 151047 fix the "download dialog can't be closed"
issue in this bug's original report too?

Should we check in the 151047 patch now then? 

Comment 17

[Steve Dagley]

No, 151047 as is makes no changes to the DL dialog behavior (one of the reasons
sfraser didn't want to land it) and it isn't code that should be executed via
the Save As from the File menu code path

Comment 18

[Simon Fraser [no longer active]]

This is happening because the nsIWebBrowserPersist flags that we get changed
*again*. The STATE_NETWORK bit isn't set on the last stop request, so we don't
know when the save is ending. Adam?

Comment 19

[Simon Fraser [no longer active]]

Here are the state changes we get now:

327681 start | request | network
65552  stop | request
65537  start | request
65552  stop | request
... (repeated for each image)

1  start
16 stop

65552 stop | request

The 'network' flag is missing on that last one.

Comment 20

[Steve Dagley]

Any chance cache changes have affected whether or not we'll always see network
flags?

Comment 21

[Simon Fraser [no longer active]]

The fix was simple:

Index: download/SaveHeaderSniffer.mm
===================================================================
RCS file: /cvsroot/mozilla/chimera/src/download/SaveHeaderSniffer.mm,v
retrieving revision 1.1
diff -u -r1.1 SaveHeaderSniffer.mm
--- download/SaveHeaderSniffer.mm	24 Jul 2002 05:53:06 -0000	1.1
+++ download/SaveHeaderSniffer.mm	28 Aug 2002 21:08:08 -0000
@@ -336,7 +336,9 @@
     flags |= nsIWebBrowserPersist::PERSIST_FLAGS_BYPASS_CACHE;
   else
     flags |= nsIWebBrowserPersist::PERSIST_FLAGS_FROM_CACHE;
-    
+  
+  webPersist->SetPersistFlags(flags);
+  
   if (sourceURI)
   {
     rv = webPersist->SaveURI(sourceURI, mPostData, destFile);

Comment 22

[Kathleen :Brade]

Attachment: fix in persist code

we should also fix this

Comment 23

[Simon Fraser [no longer active]]

Comment on attachment 97078 [details] [diff] [review]
fix in persist code

sr=sfraser for trunk and chimera branch

Comment 24

[Adam Lock]

Comment on attachment 97078 [details] [diff] [review]
fix in persist code

r=adamlock

Comment 25

[Chris Petersen]

Verified in the 2002-08-29-05 NB.