Closed Bug 797157 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Installer, defect, blocker)

17 Branch
All
Windows XP
defect
Not set
blocker

Tracking

()

VERIFIED FIXED
Firefox 18

People

(Reporter: jsmith, Assigned: robert.strong.bugs)

References

Details

(Whiteboard: [stub+])

Attachments

(2 files, 3 obsolete files)

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.
Yikes. This actually causes even worse behavior - when this scenario happens, nightly no longer shows up in add or remove programs.
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.
Whiteboard: [stub+]
I'm fine with that. I'll look at it tonight or tomorrow. Can you attach the install.log from when this happens? Thanks
Assignee: nobody → robert.bugzilla
Severity: normal → blocker
(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?
The installation directory though it might not be present.
Attached file Install log
This will block communicating the stub installer to Mozillians - do we have an ETA for this bug?
Attached patch patch rev1 (obsolete) — Splinter Review
This is based on the OnInstallUninstall macro
Attachment #668515 - Flags: review?(netzen)
Note that the TmpVal var is not defined and I have fixed this locally
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.
Attachment #668515 - Flags: review?(netzen) → review+
Attached patch patch rev2 (obsolete) — Splinter Review
Attachment #668515 - Attachment is obsolete: true
Attachment #668525 - Flags: review?(netzen)
Attached patch patch rev3 - remove the else (obsolete) — Splinter Review
Attachment #668525 - Attachment is obsolete: true
Attachment #668525 - Flags: review?(netzen)
Attachment #668528 - Flags: review?(netzen)
(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 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
Attachment #668528 - Flags: review?(netzen) → review+
Just a heads up that I found a bug in the patch that I have been investigating
Builds were started a while ago so we are going to go with that
what's the bug?
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.
Attached patch patch rev4Splinter Review
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
Attachment #668528 - Attachment is obsolete: true
Attachment #668591 - Flags: review+
(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?
https://hg.mozilla.org/mozilla-central/rev/a41de64fb322
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
QA Contact: jsmith
Whiteboard: [stub+] → [stub+], [qa verification failed]
(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.
(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.
Status: RESOLVED → VERIFIED
Whiteboard: [stub+], [qa verification failed] → [stub+],
Whiteboard: [stub+], → [stub+]
You need to log in before you can comment on or make changes to this bug.