797157 - Trying to pave-over install with the stub installer with nightly already running - warning comes up that nightly is already running, but installer closes and leaves behind "to_be_deleted" folder in C:\Program Files\Nightly

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Jason Smith [:jsmith]]

Steps:

1. Install Nightly using the stub installer using the x32 build
2. After Nightly successfully launches and is running, reinstall the same Nightly with the stub installer
3. Eventually, you'll get the "nightly is already running" prompt. Click okay on that.

Expected:

The "old" behavior I recall would be to repeatedly prompt until the user cancels the Nightly process or cancels the installer process. We should either follow the same behavior or follow something equivalently acceptable such as failing fast in the download phase. At the very minimum, we shouldn't result in leaving behind the download artifacts made in the "download" phase of the stub installer.

Actual:

Selecting okay closes the stub installer with no confirmation or clue to the user on what's happened. Additionally, checking the C:\Program Files\Nightly leaves behind a "to_be_deleted" folder behind. My likely hunch is that this is the resources we've downloaded in the download phase of the stub installer, but the abrupt ending of the install resulted in a lack of cleanup of resources we are not using.

Comment 1

[Jason Smith [:jsmith]]

Yikes. This actually causes even worse behavior - when this scenario happens, nightly no longer shows up in add or remove programs.

Comment 2

[Jason Smith [:jsmith]]

Not sure how we want to flag things that smell like blockers to ship, would stub+ work? This smells like something we would want to fix.

Comment 3

[Robert Strong (they/them - no direct email)]

I'm fine with that. I'll look at it tonight or tomorrow. Can you attach the install.log from when this happens? Thanks

Comment 4

[Jason Smith [:jsmith]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #3)
> I'm fine with that. I'll look at it tonight or tomorrow. Can you attach the
> install.log from when this happens? Thanks

Where can find that log on XP?

Comment 5

[Robert Strong (they/them - no direct email)]

The installation directory though it might not be present.

Comment 6

[Jason Smith [:jsmith]]

Attachment: Install log

Comment 7

[Alex Keybl [:akeybl]]

This will block communicating the stub installer to Mozillians - do we have an ETA for this bug?

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

This is based on the OnInstallUninstall macro

Comment 9

[Robert Strong (they/them - no direct email)]

Note that the TmpVal var is not defined and I have fixed this locally

Comment 10

[Brian R. Bondy [:bbondy]]

Comment on attachment 668515 [details] [diff] [review]
patch rev1

Review of attachment 668515 [details] [diff] [review]:
-----------------------------------------------------------------

nit: In the uninstaller nsi please add an Rmdir /r call to delete this folder.

::: toolkit/mozapps/installer/windows/nsis/common.nsh
@@ +4213,5 @@
> +      StrCmp "$R1" "File:" +1 end
> +      StrCpy $R9 "$R9" "" 6    ; Copy string starting after the 6th char
> +      StrCpy $R0 "$R9" 1       ; Copy the first char
> +
> +      StrCmp "$R0" "\" +1 end  ; If this isn't a relative path goto end

Is it ever possible to have an absolute path here in the format of C:\...\file?

@@ +4232,5 @@
> +      Rename "$R1" "$R2"
> +      ${If} ${Errors}
> +        Delete /REBOOTOK "$R1"
> +      ${Else}
> +        Delete /REBOOTOK "$R2"

I think this else condition isn't needed because you successfully renamed the file already, and it exists inside instdir\to_be_deleted, so it should be picked up by the final RmDir /r /REBOOTOK "$INSTDIR\${TO_BE_DELETED}"

@@ +4238,5 @@
> +
> +      end:
> +      ClearErrors
> +
> +      Push 0

I'm assuming the code that calls this callback will pop off the error code after calling this function. I'm also assuming you always want 0 on the stack after this function call both when there are errors and when there are not.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch rev3 - remove the else

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to Brian R. Bondy [:bbondy] from comment #10)
> Comment on attachment 668515 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 668515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nit: In the uninstaller nsi please add an Rmdir /r call to delete this
> folder.
> 
> ::: toolkit/mozapps/installer/windows/nsis/common.nsh
> @@ +4213,5 @@
> > +      StrCmp "$R1" "File:" +1 end
> > +      StrCpy $R9 "$R9" "" 6    ; Copy string starting after the 6th char
> > +      StrCpy $R0 "$R9" 1       ; Copy the first char
> > +
> > +      StrCmp "$R0" "\" +1 end  ; If this isn't a relative path goto end
> 
> Is it ever possible to have an absolute path here in the format of
> C:\...\file?
It always uses relative paths

> @@ +4232,5 @@
> > +      Rename "$R1" "$R2"
> > +      ${If} ${Errors}
> > +        Delete /REBOOTOK "$R1"
> > +      ${Else}
> > +        Delete /REBOOTOK "$R2"
> 
> I think this else condition isn't needed because you successfully renamed
> the file already, and it exists inside instdir\to_be_deleted, so it should
> be picked up by the final RmDir /r /REBOOTOK "$INSTDIR\${TO_BE_DELETED}"
Done

> 
> @@ +4238,5 @@
> > +
> > +      end:
> > +      ClearErrors
> > +
> > +      Push 0
> 
> I'm assuming the code that calls this callback will pop off the error code
> after calling this function. I'm also assuming you always want 0 on the
> stack after this function call both when there are errors and when there are
> not.
Yes

Comment 14

[Brian R. Bondy [:bbondy]]

Comment on attachment 668528 [details] [diff] [review]
patch rev3 - remove the else

Review of attachment 668528 [details] [diff] [review]:
-----------------------------------------------------------------

If you want you can put an if exists around the removes but I don't think it'll matter other than it will set the error bit which usually gets cleared before being checked.

::: browser/installer/windows/nsis/stub.nsi
@@ -973,5 @@
>        Delete "$PLUGINSDIR\download.exe"
>        Delete "$PLUGINSDIR\${CONFIG_INI}"
> -      ${If} ${FileExists} "$INSTDIR\${TO_BE_DELETED}"
> -        RmDir /r /REBOOTOK "$INSTDIR\${TO_BE_DELETED}"
> -      ${EndIf}

Good catch

Comment 15

[Robert Strong (they/them - no direct email)]

Just a heads up that I found a bug in the patch that I have been investigating

Comment 16

[Robert Strong (they/them - no direct email)]

Builds were started a while ago so we are going to go with that

Comment 17

[Brian R. Bondy [:bbondy]]

what's the bug?

Comment 18

[Robert Strong (they/them - no direct email)]

Not sure yet. Even better, my system locked up and the patches reverted to a previous version locally. It might be a problem with my system.

Comment 19

[Robert Strong (they/them - no direct email)]

Attachment: patch rev4

Bah... forgot that SetOverWrite isn't set to On so the temp file has to be removed before moving the file in use.

Got r+ from bbondy over irc for this change

Comment 20

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/a41de64fb322

Comment 21

[Jason Smith [:jsmith]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #20)
> Pushed to mozilla-inbound
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a41de64fb322

I just grabbed the inbound build and tested this on my win 7 64-bit machine - http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1349528014/. 

This isn't entirely fixed, although the major "showstopping" problem looked like it was fixed. Specifically if I repeat the steps to reproduce in this bug, then:

- The to_be_deleted folder is still left behind in C:\Program Files (86)
- I'll now see nightly listed as a program that can be uninstalled. Uninstalling nightly appears to working as expected (all expected folders were removed, although the to_be_deleted folder that was left behind was also left behind by the uninstaller)

Any ideas?

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a41de64fb322

Comment 23

[Robert Strong (they/them - no direct email)]

(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #20)
> > Pushed to mozilla-inbound
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/a41de64fb322
> 
> I just grabbed the inbound build and tested this on my win 7 64-bit machine
> -
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> inbound-win32/1349528014/. 
> 
> This isn't entirely fixed, although the major "showstopping" problem looked
> like it was fixed. Specifically if I repeat the steps to reproduce in this
> bug, then:
> 
> - The to_be_deleted folder is still left behind in C:\Program Files (86)
> - I'll now see nightly listed as a program that can be uninstalled.
> Uninstalling nightly appears to working as expected (all expected folders
> were removed, although the to_be_deleted folder that was left behind was
> also left behind by the uninstaller)
> 
> Any ideas?
That is literally "to be deleted" in that it is set to be deleted on OS restart if we have user rights to delete it on OS restart. There is also code in the uninstaller and installer that attempt to delete this folder during uninstall and install.

Comment 24

[Jason Smith [:jsmith]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #23)
> (In reply to Jason Smith [:jsmith] from comment #21)
> > (In reply to Robert Strong [:rstrong] (do not email) from comment #20)
> > > Pushed to mozilla-inbound
> > > https://hg.mozilla.org/integration/mozilla-inbound/rev/a41de64fb322
> > 
> > I just grabbed the inbound build and tested this on my win 7 64-bit machine
> > -
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> > inbound-win32/1349528014/. 
> > 
> > This isn't entirely fixed, although the major "showstopping" problem looked
> > like it was fixed. Specifically if I repeat the steps to reproduce in this
> > bug, then:
> > 
> > - The to_be_deleted folder is still left behind in C:\Program Files (86)
> > - I'll now see nightly listed as a program that can be uninstalled.
> > Uninstalling nightly appears to working as expected (all expected folders
> > were removed, although the to_be_deleted folder that was left behind was
> > also left behind by the uninstaller)
> > 
> > Any ideas?
> That is literally "to be deleted" in that it is set to be deleted on OS
> restart if we have user rights to delete it on OS restart. There is also
> code in the uninstaller and installer that attempt to delete this folder
> during uninstall and install.

Ah, that makes sense. I just retested this on Win XP, saw the OS restart, restarted the OS, and saw the to_be_deleted folder disappear. Marking as verified.

Thanks for the clarification.