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)
henh, this reminds me of bug 56295 [tho' that one's Mac only]...
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.
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??
Created attachment 18448 [details] [diff] [review] fix to make the helper app dialog non modal (this is the way it used to be)
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.
I don't have anything called "N6Setup" in the directory. Jar said the 'salting' was important for security yesterday.
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).
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.
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.
(sorry, wasn't reading bugmail last night) sr=alecf
updating status. once you check in, can you go reopen that crasher bug?
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?).
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?
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).
I just checked the fix into the branch after receiving permission from PDT just now....Fixed on branch and tip.
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...
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].