396870 - Add file in use uninstall support

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

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

Attachment: patch -rev1

Since there is no reliable or for that matter convoluted way to inform the user what has a file opened during an uninstall we should delete files on OS restart when a file is in use.

Comment 1

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

Comment on attachment 281633 [details] [diff] [review]
patch -rev1

>Index: toolkit/mozapps/installer/windows/nsis/common.nsh
>===================================================================
>...
>+!macro createSummaryINI
This is a pre install summary page to fix bug 252273

>+!macro ManualCloseAppPrompt
This is for when the user is the one using the app and it must be closed before launching the newly installed version or before attempting to uninstall by preventing the wizard from moving on to the next page.

>@@ -2793,17 +2898,26 @@
>       StrCmp "$R1" "File:" +1 end
>       StrCpy $R9 "$R9" "" 6
>       StrCpy $R0 "$R9" 1
> 
>       StrCpy $R1 "$INSTDIR$R9"
>       StrCmp "$R0" "\" +2 +1
>       StrCpy $R1 "$R9"
> 
>-      ${DeleteFile} "$R1"
>+      IfFileExists "$R1" +1 end
>+      Delete "$R1"
>+      IfErrors +1 end
>+      ClearErrors
>+      Rename "$R1" "$R1.moz-delete"
>+      IfErrors +1 +3
>+      Delete /REBOOTOK "$R1"
>+      GoTo end
>+
>+      Delete /REBOOTOK "$R1.moz-delete"
By renaming the file prior to deleting it with /REBOOTOK it prevents attempts to use the file after the uninstall completes.

>Index: browser/installer/windows/nsis/installer.nsi
>===================================================================
>...
>+!system 'echo ; > summary.ini'
New summary custom page

>@@ -176,18 +179,20 @@
> ; Start Menu Folder Page Configuration
> !define MUI_PAGE_CUSTOMFUNCTION_PRE preStartMenu
> !define MUI_STARTMENUPAGE_NODISABLE
> !define MUI_STARTMENUPAGE_REGISTRY_ROOT "HKLM"
> !define MUI_STARTMENUPAGE_REGISTRY_KEY "Software\Mozilla\${BrandFullNameInternal}\${AppVersion} (${AB_CD})\Main"
> !define MUI_STARTMENUPAGE_REGISTRY_VALUENAME "Start Menu Folder"
> !insertmacro MUI_PAGE_STARTMENU Application $StartMenuDir
> 
>+; Custom Summary Page
>+Page custom preSummary leaveSummary
>+
> ; Install Files Page
>-!define MUI_PAGE_CUSTOMFUNCTION_PRE preInstFiles
With this path these operations are performed before progressing to the installation page

>@@ -529,31 +534,31 @@
>   ClearErrors
>   ${If} $R6 ==  ""
>     ${Unless} ${FileExists} "$R1$R3\$R7"
>       ClearErrors
>       CreateDirectory "$R1$R3\$R7"
>       ${If} ${Errors}
>         ${LogMsg}  "** ERROR Creating Directory: $R1$R3\$R7 **"
>         StrCpy $0 "$R1$R3\$R7"
>-        ${WordReplace} "$(^FileError_NoIgnore)" "\r\n" "$\r$\n" "+*" $0
>+        StrCpy $0 "$(ERROR_CREATE_DIRECTORY)"
Better message

>         Quit
>       ${Else}
>         ${LogMsg}  "Created Directory: $R1$R3\$R7"
>       ${EndIf}
>     ${EndUnless}
>   ${Else}
>     ${Unless} ${FileExists} "$R1$R3"
>       ClearErrors
>       CreateDirectory "$R1$R3"
>       ${If} ${Errors}
>         ${LogMsg}  "** ERROR Creating Directory: $R1$R3 **"
>         StrCpy $0 "$R1$R3"
>-        ${WordReplace} "$(^FileError_NoIgnore)" "\r\n" "$\r$\n" "+*" $0
>+        StrCpy $0 "$(ERROR_CREATE_DIRECTORY)"
Same here

>@@ -585,17 +590,17 @@
>     ; helper.exe to be used with zip builds if we supply an uninstall.log.
>     ${WordReplace} "$R1$R3\$R7" "$INSTDIR" "" "+" $R3
>     ${LogUninstall} "File: $R3"
>   ${EndIf}
>   Push 0
> FunctionEnd
> 
> Function LaunchApp
>-  ${CloseApp} "true" $(WARN_APP_RUNNING_INSTALL)
>+  ${ManualCloseAppPrompt} "${WindowClass}" "$(WARN_MANUALLY_CLOSE_APP_LAUNCH)"
Ask the user to close the app

>@@ -672,17 +677,36 @@
> 
> Function preStartMenu
>   ${CheckCustomCommon}
>   ${If} $AddStartMenuSC != 1
>     Abort
>   ${EndIf}
> FunctionEnd
> 
>-Function preInstFiles
>+Function preSummary
New summary page

>+Function leaveSummary
>+  ; If there is a pending deletion from a previous uninstall don't allow
>+  ; installing until after the system has rebooted.
>+  IfFileExists "$INSTDIR\${FileMainEXE}.moz-delete" +1 +4
>+  MessageBox MB_YESNO "$(WARN_RESTART_REQUIRED_UNINSTALL)" IDNO +2
>+  Reboot
>+  Quit
If there is a pending uninstall ask the user to restart before trying to install again.

>Index: browser/installer/windows/nsis/uninstaller.nsi
>===================================================================
>...
> ; Uninstall Confirm Page
>+!define MUI_PAGE_CUSTOMFUNCTION_LEAVE un.leaveConfirm
> !insertmacro MUI_UNPAGE_CONFIRM
> 
> ; Remove Files Page
>-!define MUI_PAGE_CUSTOMFUNCTION_PRE un.preInstFiles
It is cleaner to perform the pre-uninstall checks prior to the installation page since we can just prevent moving to the next wizard page.

>@@ -174,16 +174,28 @@
> ################################################################################
> # Uninstall Sections
> 
> Section "Uninstall"
>   SetDetailsPrint textonly
>   DetailPrint $(STATUS_UNINSTALL_MAIN)
>   SetDetailsPrint none
> 
>+  ; Delete the app exe to prevent launching the app while we are uninstalling.
>+  ClearErrors
>+  ${DeleteFile} "$INSTDIR\${FileMainEXE}"
>+  ${If} ${Errors}
>+    ; If the user closed the application it can take several seconds for it to
>+    ; shut down completely. If the application is being used by another user we
>+    ; can still delete the files when the system is restarted. 
>+    Sleep 5000
>+    ${DeleteFile} "$INSTDIR\${FileMainEXE}"
>+    ClearErrors
>+  ${EndIf}
The comment says it... I do wish there were a better way but 5 seconds isn't that long.

>@@ -232,36 +244,58 @@
>     ${un.GetParent} "$R1" $R1
>     ${If} "$INSTDIR" == "$R1"
>       WriteRegStr HKLM "$0" "" "$R9"
>       ${un.GetParent} "$R9" $R1
>       WriteRegStr HKLM "$0" "Path" "$R1"
>     ${EndIf}
>   ${EndIf}
> 
>-  ; Remove directories and files we always control
>-  RmDir /r "$INSTDIR\updates"
>-  RmDir /r "$INSTDIR\defaults\shortcuts"
>-  RmDir /r "$INSTDIR\distribution"
>-  Delete "$INSTDIR\removed-files"
>+  ; Remove directories and files we always control before parsing the uninstall
>+  ; log so empty directories can be removed.
>+  ${If} ${FileExists} "$INSTDIR\updates"
>+    RmDir /r /REBOOTOK "$INSTDIR\updates"
>+  ${EndIf}
>+  ${If} ${FileExists} "$INSTDIR\defaults\shortcuts"
>+    RmDir /r /REBOOTOK "$INSTDIR\defaults\shortcuts"
>+  ${EndIf}
>+  ${If} ${FileExists} "$INSTDIR\distribution"
>+    RmDir /r /REBOOTOK "$INSTDIR\distribution"
>+  ${EndIf}
>+  ${If} ${FileExists} "$INSTDIR\removed-files"
>+    Delete /REBOOTOK "$INSTDIR\removed-files"
>+  ${EndIf}
Add reboot support and only when they exist

>-; When we add an optional action to the finish page the cancel button is
>-; enabled. This disables it and leaves the finish button as the only choice.
> Function un.preFinish
>-  !insertmacro MUI_INSTALLOPTIONS_WRITE "ioSpecial.ini" "settings" "cancelenabled" "0"
>-
>-  ; Setup the survey controls, functions, etc. except when the application has
>-  ; defined NO_UNINSTALL_SURVEY
>-  !ifdef NO_UNINSTALL_SURVEY
>+  ; Do not modify the finish page if there is a reboot pending
>+  ${Unless} ${RebootFlag}
This is needed so we don't disable the cancel button or remove fields when a reboot is required.

Comment 2

[(not reading, please use [e-mail] instead)]

Comment on attachment 281633 [details] [diff] [review]
patch -rev1

r=sspitzer, thanks for explaining the patch.

a question about the "sleep 5000".  Could we instead do some sort of loop where we sleep for a second, see if FindProc can find our process.  (Will FindProc find processes by another user?)

If we find the process and it's been 5 seconds, bail out.  If we can't find the process, we can avoid waiting the full 5 seconds.

Of course, let me know if you feel this is overkill (or won't work).

Comment 3

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

I considered using a loop for this but felt it was overkill... I'm going to continue to think about other ways of doing this but for now the sleep should suffice. btw: this is to only for when the current user is using firefox.exe and is not concerned with other users so using FindProc would find the other user's instance when we don't care about the other user's instance.

Comment 4

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

Comment on attachment 281633 [details] [diff] [review]
patch -rev1

Drivers, we currently just leave behind files in use on uninstall which this fixes by deleting them on OS restart. This also displays a new summary page so we can set the text on a button to Install when the next step is install (see bug 252273).

Comment 5

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

Checked in to trunk

Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.28; previous revision: 1.27
done
Checking in mozilla/browser/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.35; previous revision: 1.34
done
Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.12; previous revision: 1.11
done
Checking in mozilla/browser/locales/en-US/installer/custom.properties;
/cvsroot/mozilla/browser/locales/en-US/installer/custom.properties,v  <--  custom.properties
new revision: 1.10; previous revision: 1.9
done

Comment 6

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

Filed Bug 397119 to add more details to the summary page.