98201 - cancel button does not respond after other events fail (pause/resume or fail to create dir etc)

Status: VERIFIED FIXED

(SeaMonkey :: Installer, defect)

Description

[Grace Bush]

Steps to reproduce:
Obtain Mozilla (0.9.3) or trunk build of N6 (20010904)
During download press [Cancel]

expected results: Installer ends

actual results: nothing - button does not respond to mouse click, need to ctrl-c
to quit

Comment 1

[K'Trina Medina]

Nominating nsbeta1

Comment 2

[Curt Patrick (gone)]

Assigning to myself

Comment 3

[André Dahlqvist]

This must have been fixed, because pressing cancel shut down the net-installer
both on 0.9.4 and the current nightly builds.

Grace Bush, please test with a recent build and try to reproduce it. If you
can't , please mark this WORKSFORME.

Comment 4

[Grace Bush]

I can reproduce on 10/5 mozilla build-
press pause/resume (see bug 98200) while pause is enable and waiting for resume, 
cancel does nothing.

cancel does work if resume is enabled- still a problem though

Comment 5

[Curt Patrick (gone)]

It seems that one must click Pause and then click Resume at least once to get
into the state that triggers this bug.  Having done so clicking Cancel (while in
the Resumed state) not only does NOT cancel but it also disables the download
and there is no obvious way to get it going again without starting all over. 
This is a more serious problem than it appeared to be when Grace and I looked it
over together.  We observed that you could still cancel out simply by clicking
Pause again first, but we did not note that the download was permanently blocked.

Grace, can you verify that you are seeing this same behavior.

Comment 6

[Grace Bush]

yes, see first comment - had to ctrl-c to get out of installer

Comment 7

[Syd Logan]

taking

Comment 8

[Grace Bush]

re summarize to handle other times cancel fails

Comment 9

[Syd Logan]

Attachment: disable and enable cancel button appropriately

The handler for the cancel button expects the download to be in Pause mode,
which explains the behaviour we are seeing. I've changed the enabling and
disabling of the cancel button to correspond to this expectation, it will be
enabled only in pause mode, and disabled otherwise.

Comment 10

[Samir Gehani]

Comment on attachment 67225 [details] [diff] [review]
disable and enable cancel button appropriately

<http://lxr.mozilla.org/seamonkey/source/xpinstall/wizard/unix/src2/nsInstallDl
g.cpp#164>

Actually, you should be able to cancel a download while it's going on rather
than having to pause first and then cancel.  We explicitly hook up a callback
for download cancellation.

Comment 11

[Syd Logan]

You should, but you can't, because you explicitly check to see if the install is
paused else you ignore the click, which is why the user sees the problem. Do you
have a better patch, or some explaination for why that check for PAUSE state is
there?

Comment 12

[Syd Logan]

It appears that Cancel is being acted upon in two cases during download.

First, the user finds themselves in the Next button callback and the global
bDLCancel flag is set, and we call gtk_main_exit() in response. The Next button
callback is invoked if the user hits the Next button, so there must be some
condition other than clicking the Cancel key that sets bDLCancel. But there
isn't -- the only place that bDLCancel is set is nsInstallDlg::DLCancel(), which
is the callback function for the cancel button. So, in this scenario, you have
to hit Cancel, then Next, for a cancel to occur? Seems weird.

The other scenario is that nsInstallDlg::DLCancel() calls gtk_main_exit() if the
bDLPause flag is set. bDLPause is set in nsInstallDlg::DLPause(), which is the
callback associated with hitting the Pause button in the UI, and nowhere else.
In this scenario, then, it seems you have to click on the Pause button, then
Cancel, in order for the cancel to occur.

So, what is the purpose of doing anything other than exiting the app when the
user hits cancel? My original patch assumed there was some importance to pausing
the download, precisely, that by pausing the download we would be able to pick
it up again successfully again the next time the installer was run (e.g.,
something happens in the pause button handler that is useful for resuming the
install where the download left off the next time the install was kicked). So,
users are forced to pause before cancelling seems good. However, if it doesn't
matter (and it doesn't, all that function does is twiddle the sensitivity of the
pause and resume buttons, and set the global bDLPause flag), we might as well
let the process exit. In unix, we can be assured that memory will be freed,
process resources like fds and network connections will get cleaned up -- this
isn't MacOS or Winblows we are dealing with here. So, why not just exit in the
cancel button if there is nothing we need to save for a successful resume to
occur the next time the user runs the installer?

Comment 13

[Syd Logan]

OK, I see how this thing works. The problem is PerformInstaller(), when called
from the resume function, doesn't check its' return value, and so if the user
canceled after a resume, it is never looked at. Doh. Patch on its way.

I also noticed that the callback in the UI code (download progress callback)
must be entered in order for the cancelled state to be acknowledged. This only
happens every 14K. Which sucks on dialup at 56K, is probably unusable on 14K or
28K connection. We should do it off of a timer of somekind. If the progress
function was just about updating the progress bar, I guess the current way is
ok. But we also do other stuff there that should appear to happen realtime. 10
or 15 seconds to wait for a response to Cancel or Pause clicking is not really
ideal.

Comment 14

[Syd Logan]

Attachment: New patch, make sure we deal with return value from "PerformInstall()"

Comment 15

[Sean Su]

Comment on attachment 68765 [details] [diff] [review]
New patch, make sure we deal with return value from "PerformInstall()"

r=ssu

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 68765 [details] [diff] [review]
New patch, make sure we deal with return value from "PerformInstall()"

sr=dveditz

Comment 17

[Syd Logan]

Fixed

Comment 18

[K'Trina Medina]

Verified on comm linux build 2002-02-18-06-trunk and mozilla 0.9.8