190484 - After GRE install, user is asked to restart computer, ending rest of install

Status: VERIFIED FIXED

(Core Graveyard :: Installer: GRE, defect)

Description

[Grace Bush]

Steps to reproduce:
1. Download trunk build 2003012404
2. Run the installer- any setup type

actual results: After GRE install, a dialog is presented to user notifying that
files are in use and asking to restart computer now or restart computer later. 
If user chooses to restart now, the rest of installation is lost. If user
chooses to not restart, the installation completes as expected.

expected results:  User is not asked to restart the computer

Comment 1

[R.K.Aa.]

*** Bug 192829 has been marked as a duplicate of this bug. ***

Comment 2

[Boris Zbarsky [:bzbarsky]]

This seems to be the most negative-reaction-provoking thing about 1.3b so far
(admittedly, the data set thus far is small).

Comment 3

[Asa Dotzler [:asa]]

Agreed. Blocking 1.3 final.

Comment 4

[Troy Miller]

Just adding my voice to the data set.  Actually, with my distrust of Windows and
knowing that Mozilla had never previously required a system restart, I seriously
suspected that I had gotten a virus-infected version of the installer.

Comment 5

[James Ross]

*** Bug 193440 has been marked as a duplicate of this bug. ***

Comment 6

[Robert Accettura [:raccettura]]

Yep, Asa got it right, this is definately a blocker.  I also thought I had a bad
download.  Even went back to redownload, with the same effect.

Comment 7

[Sean Su]

found the problem.  working on a fix now.

Comment 8

[Boris Zbarsky [:bzbarsky]]

*** Bug 193503 has been marked as a duplicate of this bug. ***

Comment 9

[Robert Accettura [:raccettura]]

should note I didn't get any notice to restart when I uninstalled Mozilla before
installing.  When installing and a version is already installed, it prompts me,
then appears to cut off.

Comment 10

[Sean Su]

Attachment: patch v1.0

this patch will now prevent the GRE installer from throwing up the Reboot
dialog.  Instead, it will log it and have the Mozilla installer throw up the
Reboot dialog.

seeking reviews now.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 114856 [details] [diff] [review]
patch v1.0

This patch doesn't address the question of *why* the GRE install thinks it
needs a reboot. I assume it's because the GRE install is run before the Mozilla
installer gets around to checking whether Mozilla is already running and
shutting it down. *That* needs to get fixed; the install shouldn't require a
reboot unless we're actually installing a system library, which we've taken
pains not to do so far and should be extremely rare -- not every install.

(To those who get this prompt, just ignore it. After the install finishes the
xpicleanup util will replace all the DLL's correctly without having to reboot)


>-$verPartial       = "5.0.0.";
>+$verPartial       = "1.3.0.";
> $ver              = $verPartial . GetVersion($DEPTH);
>-$verGre           = "1.3b.0.0";
>+$verGre           = $verParial . "0";

$verPartial ?
.......^

>+      CleanupXpcomFile();
>             if(NeedReboot())
>             {
>-              CleanupXpcomFile();
>+        LogExitStatus("Reboot");
>+        if(sgProduct.mode == NORMAL)

Tab issues?


> /* Function to copy strings safely.
>  *   returns the amount of memory required (including NULL byte) if there's not enough
>  *   else, it returns 0 for success.
>  */
> int MozCopyStr(LPSTR szSrc, LPSTR szDest, DWORD dwDestBufSize)
> {
>-  DWORD length = lstrlen(szSrc) + 1;
>+  DWORD length;
>+
>+  if(!szSrc || !szDest)
>+    return(WIZ_ERROR_UNDEFINED);

Is this such a good idea? some calling code will now think they need to
recall this function with a 1024-byte buffer. On the other hand I can't
find any places where the MozCopyStr return value is interpreted as a
length and only 5 places that use it at all. I assume this was added
because you found a case that passed a null. With this patch you won't know
when that happens, but it's got to be an error. Would an assertion help?

>+  /* clean up previous exis status log file */

"exit"

>+BOOL GreInstallerNeedsReboot()
>+{
>+  BOOL greReboot = FALSE;
>+
>+  /* if this setup is not installing GRE *and* the GRE Setup has been run, then
>+   * check for GRE setup's exit value, if one exists */

the comment confused me at first, because the Mozilla installer *does*
install the GRE (unless it's up to date and not forced). Maybe

   if this is not the GRE install itself *and* the GRE Setup has been run, then
   ...

sr=dveditz on this patch if you fix the above minor issues, but I'd rather fix
the real problem which is the bogus need for a reboot.

Comment 12

[Sean Su]

Attachment: patch v1.1

> This patch doesn't address the question of *why* the GRE install thinks it
>  needs a reboot. I assume it's because the GRE install is run before the
>  Mozilla installer gets around to checking whether Mozilla is already
>  running and shutting it down. *That* needs to get fixed; the install
>  shouldn't require a reboot unless we're actually installing a system
>  library, which we've taken pains not to do so far and should be extremely
>  rare -- not every install.

This patch definitely does not address that problem.  Requiring a reboot right
now is probably due to a combination of these bugs:
  bug 193173 - Installer needs to detect running apps using GRE
  bug 190816 - Running process causes Installer to hang until process is
	       shut down via task manager
  bug 151532 - Delay system-tray icon until it is ready to accept
	       mouse-clicks

> $verPartial ?
> .......^

typo fixed.

> Tab issues?

not really.  I forgot to mention that the last patch was diffed with a '-w'. 
this one is not.

> I assume this was added because you found a case that passed a null. With
> this patch you won't know when that happens, but it's got to be an error.
> Would an assertion help?

yes, I did run into a case where it crashed.  It was due to the order of how
things are initialized as setup starts up.  Code changed to use assertions
instead.

> "exit"

typo fixed.

> the comment confused me at first, because the Mozilla installer *does*
> install the GRE (unless it's up to date and not forced). Maybe
>
>   if this is not the GRE install itself *and* the GRE Setup has been run,
then
>   ...

changed.

Comment 13

[Samir Gehani]

Comment on attachment 114856 [details] [diff] [review]
patch v1.0

r=sgehani

Caveat for review:
Please file a bug to make WinSpawn() return the child process's exit code so we
can simply detect the child installer reboot requirement.  Thanks.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 114872 [details] [diff] [review]
patch v1.1

a=asa (on behalf of drivers) for checkin to 1.3.

Comment 15

[Sean Su]

patch checked in.

Comment 16

[Charles Rosendahl]

20030225 and 20030227 mozilla nightlies.
Verified - No reboot messages being generated directly from the GRE installer
itself.
 

Comment 17

[Jo Hermans]

*** Bug 195699 has been marked as a duplicate of this bug. ***

Comment 18

[TheGreatOne]

um? is the release fixed or do i have to download that .dif file... what do i 
do with the .dif after i download it?

Comment 19

[TheGreatOne]

I Really do need help on how to fix this bug. IE isnt saving my passwords 
correctly and it sucks and its slow :(

Comment 20

[TheGreatOne]

My browser is totally mest up and i cant downgrade and i never heard of a 
damn .dif file before...

Comment 21

[Boris Zbarsky [:bzbarsky]]

This was checked in on the branch, so it should be fixed in the release.  Follow
up in bug 197503