800112 - If I pave-over install Nightly while Nightly is running twice or more in a row, then nightly will always order a restart on every launch or uninstall

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Jason Smith [:jsmith]]

Steps:

1. With nightly already running, install Nightly with the stub installer
2. After installation completes, install Nightly again with the stub installer with Nightly already running
3. Close Nightly
4. Launch Nightly - restart required is seen
5. After restarting the machine, try launching nightly

Expected:

Nightly should launch with no errors.

Actual:

Nightly orders a restart required. This happens if you try launch Nightly anytime after these STR are executed or uninstall Nightly. The work-around for this issue is to pave-over install Nightly over the existing installation of Nightly without Nightly running.

Comment 1

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

Saw this as well late last night and I have a patch in progress. I think this should block and I'll hopefully have it fixed by tomorrow morning.

Comment 2

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

Attachment: patch rev1

The main changes are:
install progress bar is no longer indeterminate.
after the install starts it times out after 60 seconds and offers to download the full installer.
uses the install.log instead of the uninstall.log to determine if the install has finished since the install.log handle is closed later in the full installer.
focus is set to the install button on the intro and options page. Otherwise, unfocus and then focus of the ui would cause the Cancel button to be selected.

I've tested this out probably 50 or more times while tuning the UI changes and it worked every time.

Comment 3

[Brian R. Bondy [:bbondy]]

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

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

I have some questions, but they are not critical, so marking this as r+.
Do you suspect this is what happened with the .moz-upgrade files from the bug with the message box?

::: browser/installer/windows/nsis/stub.nsi
@@ +77,5 @@
> +; defined by InstallProgressFirstStep. This might not be enough when installing on a slow network drive so it will
> +; fallback to downloading the full installer if it reaches this number. The size
> +; of the install progress step increases when the full installer finishes
> +; instead of waiting the entire 60 seconds.
> +!define InstallProgresSteps 620

Maybe this should be 1220?

@@ +1028,5 @@
> +  ${EndIf}
> +
> +  SendMessage $ProgressbarInstall ${PBM_SETPOS} $InstallCounterStep 0
> +
> +  ${If} ${FileExists} "$INSTDIR\install.log"

Is there any case where an insatll.log will not be created? If so should we do any of the below steps in that case?  Or give some kind of prompt?

@@ +1041,5 @@
> +      ${NSD_KillTimer} CheckInstall
> +      ; Close the handle that prevents modification of the full installer
> +      System::Call 'kernel32::CloseHandle(i $HandleDownload)'
> +      Rename "$INSTDIR\install.tmp" "$INSTDIR\install.log"
> +      Delete "$INSTDIR\install.tmp"

If install.tmp is renamed to install.log why delete install.tmp after? Is this in case a rename fails but a delete succeeds?  Would having a left over install.tmp cause any problems when applying partial updates? If so maybe use /REBOOTOK?

@@ +1093,5 @@
> +  ${EndIf}
> +
> +  ${If} ${FileExists} "$INSTDIR\${FileMainEXE}.moz-upgrade"
> +    Delete "$INSTDIR\${FileMainEXE}"
> +    Rename "$INSTDIR\${FileMainEXE}.moz-upgrade" "$INSTDIR\${FileMainEXE}"

What if the first delete succeeds but the second rename fails?

Comment 4

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

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Comment on attachment 670707 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 670707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some questions, but they are not critical, so marking this as r+.
> Do you suspect this is what happened with the .moz-upgrade files from the
> bug with the message box?
> 
> ::: browser/installer/windows/nsis/stub.nsi
> @@ +77,5 @@
> > +; defined by InstallProgressFirstStep. This might not be enough when installing on a slow network drive so it will
> > +; fallback to downloading the full installer if it reaches this number. The size
> > +; of the install progress step increases when the full installer finishes
> > +; instead of waiting the entire 60 seconds.
> > +!define InstallProgresSteps 620
> 
> Maybe this should be 1220?
I am ok with changing that

> @@ +1028,5 @@
> > +  ${EndIf}
> > +
> > +  SendMessage $ProgressbarInstall ${PBM_SETPOS} $InstallCounterStep 0
> > +
> > +  ${If} ${FileExists} "$INSTDIR\install.log"
> 
> Is there any case where an insatll.log will not be created? If so should we
> do any of the below steps in that case?  Or give some kind of prompt?
It is always created and is one of the first things done. I've added a CloseHandle for the download to the fallback.

> @@ +1041,5 @@
> > +      ${NSD_KillTimer} CheckInstall
> > +      ; Close the handle that prevents modification of the full installer
> > +      System::Call 'kernel32::CloseHandle(i $HandleDownload)'
> > +      Rename "$INSTDIR\install.tmp" "$INSTDIR\install.log"
> > +      Delete "$INSTDIR\install.tmp"
> 
> If install.tmp is renamed to install.log why delete install.tmp after? Is
> this in case a rename fails but a delete succeeds?  Would having a left over
> install.tmp cause any problems when applying partial updates? If so maybe
> use /REBOOTOK?
Good catch. I changed it from CopyFiles to Rename and forgot to remove the Delete.

> @@ +1093,5 @@
> > +  ${EndIf}
> > +
> > +  ${If} ${FileExists} "$INSTDIR\${FileMainEXE}.moz-upgrade"
> > +    Delete "$INSTDIR\${FileMainEXE}"
> > +    Rename "$INSTDIR\${FileMainEXE}.moz-upgrade" "$INSTDIR\${FileMainEXE}"
> 
> What if the first delete succeeds but the second rename fails?
Then we are in a bad state but this case is very edgy and really should never happen.

Comment 5

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

Attachment: patch rev2 - comments addressed

Comment 6

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

Comment on attachment 670839 [details] [diff] [review]
patch rev2 - comments addressed

Preemptively requesting aurora for this

Comment 7

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

Pushed to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/90857937b601

Comment 8

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

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4c8d6775af7

Comment 9

[Jason Smith [:jsmith]]

Verified on 10/12 build.