N6Setup.exe deleted after download

VERIFIED FIXED

Status

SeaMonkey
UI Design
P3
normal
VERIFIED FIXED
17 years ago
13 years ago

People

(Reporter: Warren Harris, Assigned: Scott MacGregor)

Tracking

({dataloss})

Trunk
x86
Windows NT
dataloss

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++] r=sspitzer, sr=alecf)

Attachments

(1 attachment)

(Reporter)

Description

17 years ago
Here's the scenario I encountered:

- went to sweetlou to download a new build
- saved N6Setup.exe to my temp directory (as I always do)
- noticed that there was already an N6Setup.exe there
- in file picker, said 'yes' to delete/overwrite the old one
- quit the browser after successful download
- when to the file browser (explorer) window to launch N6Setup.exe
- it was gone

Here's what I found out was happening:

- while the file picker dialog is up, the browser is actually downloading the
file to speed up the download process while I'm busy picking the destination
- what is supposed to happen is that after I pick a download location, the
pre-downloaded file is copied to my specified location
- I accidently pick the same location that it picked for the pre-downloaded file
- when the browser quits, it deletes all pre-downloaded files, inadvertently
deleting the file at my final destination

Possible solutions:

- could notice that the user's chosen location is the same as the pre-downloaded
location, and mark the file as not needing deleted on quit
  not good for several reasons:
    - I still see the pre-downloaded file and have to go through the Replace dialog
    - potential security problem if downloaded files get put in predictable
places with predictable names

- better solution would be to pre-download the file with an obscured name (like
we do in the cache directory)
    - could even clean up the file after copying to final user-specified
destination (rather than on quit)
(Reporter)

Updated

17 years ago
Keywords: dataloss, rtm
henh, this reminds me of bug 56295 [tho' that one's Mac only]...
(Assignee)

Comment 2

17 years ago
I think part of the problem is a regression caused by the fix for Bug #55627.
that's my bad because I super reviewed law's change and didn't catch this. That
bug makes the helper app dialog MODAL. This blocks the exthandler's ability to
process incoming OnDataAvailable events because the event queue isn't getting
any attention while the modal dialog is up. For large files this is really bad
because we never empty the stream while http is shoving more data into the stream.

Yes, the temporary file name should be salted but that bug ended up being
futured. I can dig up the bug number.

I was able to see Warren's behavior with the file not getting downloaded and
moved over properly occasionally. I couldn't get it to happen every time.

Removing the modality of the helper app dialog made this behavior go away.

Whiteboard: [rtm need info]
(Assignee)

Comment 3

17 years ago
Warren, do you have a file called N6Setup (note the lack of extension) in your
temp directory instead of N6Setup.exe? I bet that's what happened to you. When
you change the directory location a file is being saved to, we don't save the
extension of the file anymore (a "feature" of nsIFilePicker??
(Assignee)

Comment 4

17 years ago
Created attachment 18448 [details] [diff] [review]
fix to make the helper app dialog non modal (this is the way it used to be)
(Assignee)

Comment 5

17 years ago
I believe warren's problem was because when he changed the name of the
directory, the file picker dropped the extension on the file (a separate bug?).
So the file was really there it was just called "N6Setup" instead of N6Setup.exe.

However, in debugging this I discovered the regression caused by making the
helper app dialog modal in Bug #55627. We don't process any of the ODA events
coming into the UI thread where the exthandler is. This will cause failures to
download really large files if you leave the modal dialog up for a while because
we'll back up necko. I'll post test cases shortly. I believe backing out that
modal dialog change is worthy of an rtm fix even at this late date.

Salting the file name was futured a while ago, but I'd be willing to revisit it
if PDT felt it was important. 
Status: NEW → ASSIGNED
Target Milestone: --- → M19
(Reporter)

Comment 6

17 years ago
I don't have anything called "N6Setup" in the directory.

Jar said the 'salting' was important for security yesterday.

Comment 7

17 years ago
I'm surprised that the modal dialog blocks the downloading of the data.  Why is
that?

Does that mean we have the same risk if the user causes some other modal dialog
to appear (e.g., presses Ctrl-L)?

It seems that a modal dialog doesn't stop *all* network traffic to cease.  For
example, if I open a modal dialog from one browser window, I can still use other
browser windows.  I did notice that if I press Ctrl-L while a page is loading,
that the page never completes loading (the throbber never turns off, even after
the dialog is dismissed).

One other note; making that dialog non-modal does reopen bug 55627, at least to
some extent.  We also checked in a null-pointer check that might avoid some of
the crashes that ensue (but it isn't clear that that will fix all of them).
(Assignee)

Comment 8

17 years ago
I couldn't find the bug I had for salting the name of the temp file so I filed a
new one. Please see 58774 to continue that thread. I posted a fix to see if PDT
is interested in taking it this late in the game for rtm.

Warren, can you apply my patch in this bug and see if that made your problems go
away with N6Setup.exe not being there when you were done?

With this patch, things are behaving well again for me. 
(Assignee)

Comment 9

17 years ago
I've been able to confirm, if I leave the modal helper app dialog up long enough
(where we end up backing up the netwerk socket because we aren't reading
anything out of the pipe) then we end up not storing any content at all.

Backing out our change to make this dialog modal fixes this problem.

Law or sspitzer, any chance one of you guys could review taking out the modal
tag tonight?

alecf, a super review?

I'm hoping to get this on the rtm+ list tonight so it has a chance with the
limbo 3 list tomorrow!

PDT: with regards to risk analysis. This dialog used to be non modal since it's
inception. A week or so ago, we made it modal in order to fix some crashes
covered in Bug #55627 where we'd weren't real happy if you dismissed the
underlying browser window before dismissing the helper app dialog that is
parented from that browser window. So by making this change, the only risk we
are opening ourselves up to is the possibilty of crashing again when you do
destroy the underlying window before dismissing the helper app dialog. I'd
rather have that trade off than not have this work.

When we re-open 55627 by checking this in, we can take a look at adding some
protection against the crash. 

Comment 10

17 years ago
(sorry, wasn't reading bugmail last night)
sr=alecf
updating status.

once you check in, can you go reopen that crasher bug?
Whiteboard: [rtm need info] → [rtm+] r=sspitzer, sr=alecf

Comment 12

17 years ago
I agree that re-exposing us to the risk of crash when closing the browser window
is better than silently losing data (which will likely occur with greater
frequency).

Any comment on the fact that opening any other modal dialog has similar effect?
 I talked to danm about that and he explained that we block any ongoing network
I/O but permit new requests that originate while the modal dialog is up to complete.

I think this is broken, but there's nothing we can do about it for rtm.
Long-term, I think we need a better architecture for handling these situations.
 There are all kinds of things broken as it stands; e.g., bringing up a dialog
in one browser window can kill any ongoing operations (fetching a web page,
downloading a file, reading mail?, sending mail?, loading chrome?).
(Assignee)

Comment 13

17 years ago
Hey Bill, thanks for talking to danm about the event processing questions with a
modal dialog. I feel better now understanding why we are seeing the behavior
that we are. Can I take it from your comments that I have a r=law?

Updated

17 years ago
Blocks: 57354

Comment 14

17 years ago
Yes on the r=law.

It turns out that this fix will also have the nice side-effect of fixing bug
57354.  In case that helps us decide.

I did note one thing, however.  The file picker dialog one gets when you choose
"Save to Disk" is modal.  I suspect that this will expose us to the same
problem.  I do not know if there is a way to make the file picker dialog
non-modal.  That dialog is a system dialog on Mac/Win and that might be
different (and maybe better).
(Assignee)

Comment 15

17 years ago
I just checked the fix into the branch after receiving permission from PDT just
now....Fixed on branch and tip.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [rtm+] r=sspitzer, sr=alecf → [rtm++] r=sspitzer, sr=alecf
hokay, i repeated warren's original step to repro the problem --and after i quit
the browser, N6Setup.exe was happily still there in the dirview. i tested this
using 2000.11.03.09-n6 comm branch bits on winNT. just as a doublecheck, i tried
this out with the comm branch bits [same spin time] on linux and mac: same
results.

oddly enough, i still got a dialog asking me whether i wanted overwrite an
existing version of the file --even though i previously checked to see that it
didn't already exist (and deleted the file before i tried testing this issue, if
it did exist). i don't understand why i should be encountering this --i imagine
it is due to the background downloading. still, some users might find it
confusing to be asked to replace the file, if they think they hadn't downloaded
it before...
Keywords: vtrunk
vrfy fixed using opt comm *trunk* bits 2000.11.13.08 on linux, winnt and mac. in
fact, i no longer see the confusing "do you want to replace" dialog when the
file doesn't exist [which is good].
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.