470972 - Always load and unload the UAC plugin to accommodate SeaMonkey

Status: RESOLVED WONTFIX

(Firefox :: Installer, enhancement)

Description

[Frank Wein [:mcsmurf]]

After talking to a NSIS developer a bit, it looks like the ElevateUAC and the UnloadUAC macros at http://hg.mozilla.org/mozilla-central/annotate/3fdd6aaa25f1/toolkit/mozapps/installer/windows/nsis/common.nsh#l5042 need a clean-up.
1. In the UnloadUAC macro, this comment
; If the command line contains /UAC then we need to unload the UAC plugin
is wrong: The UAC plugin is also present in the non-elevated process, so we need to unload it every time, not just in the elevated process
2. In the ElevateUAC macro there is no need to check if we run on Vista or if we are admin already (at least I think so...). What about when we run the installer as limited user on WinXP? Guess Windows will ask to run the installer as admin and/or we can use the plugin to run the second, elevated process as admin.
Also we do not seem to check the return code of RunElevated? Ok, we do some write tests in the installer if we can write to HKLM, but maybe we can/should instead check if RunElevated succeeded with $0==0? Thoughts on this?

Comment 1

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

(In reply to comment #0)
> After talking to a NSIS developer a bit, it looks like the ElevateUAC and the
> UnloadUAC macros at
> http://hg.mozilla.org/mozilla-central/annotate/3fdd6aaa25f1/toolkit/mozapps/installer/windows/nsis/common.nsh#l5042
> need a clean-up.
> 1. In the UnloadUAC macro, this comment
> ; If the command line contains /UAC then we need to unload the UAC plugin
> is wrong: The UAC plugin is also present in the non-elevated process, so we
> need to unload it every time, not just in the elevated process
Though the comment may not be correct I'm not convinced anything needs to be done. At least in the case of the Firefox installer the plugin is not in use and hence would not be deleted when the installer completes or is canceled.

> 2. In the ElevateUAC macro there is no need to check if we run on Vista or if
> we are admin already (at least I think so...). What about when we run the
> installer as limited user on WinXP? Guess Windows will ask to run the installer
> as admin and/or we can use the plugin to run the second, elevated process as
> admin.
If you wanted to use it outside of the common macros provided by common.nsh then you would likely need to update all of the installers and common.nsh accordingly or use your own code outside of common.nsh to accomplish this. I personally am not too keen on launching the installer on XP and using the custom run as dialog provided by the UAC plugin since that is not normal behavior of installers in general on XP.

> Also we do not seem to check the return code of RunElevated? Ok, we do some
> write tests in the installer if we can write to HKLM, but maybe we can/should
> instead check if RunElevated succeeded with $0==0? Thoughts on this?
It appears to work just fine the way it is but if it fixed a problem I would be all for such a change. Having said that, I might be fine with such a change on trunk.

Comment 2

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

(In reply to comment #1)
> (In reply to comment #0)
> > After talking to a NSIS developer a bit, it looks like the ElevateUAC and the
> > UnloadUAC macros at
> > http://hg.mozilla.org/mozilla-central/annotate/3fdd6aaa25f1/toolkit/mozapps/installer/windows/nsis/common.nsh#l5042
> > need a clean-up.
> > 1. In the UnloadUAC macro, this comment
> > ; If the command line contains /UAC then we need to unload the UAC plugin
> > is wrong: The UAC plugin is also present in the non-elevated process, so we
> > need to unload it every time, not just in the elevated process
> Though the comment may not be correct I'm not convinced anything needs to be
> done. At least in the case of the Firefox installer the plugin is not in use
> and hence would not be deleted when the installer completes or is canceled.
In case it isn't clear the reason it needs to be unloaded is to prevent it from being in use during the cancel / finish cases since the temp directory where the plugin resides won't be deleted. From my own testing it is deleted so I don't believe the code to unload the UAC plugin is broken.

Comment 3

[Frank Wein [:mcsmurf]]

Why do we check if we are admin? Was the special logic in that macro required in that time for admin vs. non-admin? The UAC plugin should normally handle all this fine.

Comment 4

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

We currently only display the UAC for users that are a member of the administrators group with the intent that only these users will be able to elevate to administrator along with the default Vista users being a member of the administrators group. This should be changed per bug 404541 to always display the UAC prompt and if the request to elevate is canceled then the install would be performed with the whatever rights the user has which would typically be in HKCU and a user writable location of the file system. When bug 404541 is fixed the check for admin would not be performed.

Comment 5

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

Hey Frank, at this point is this bug for anything besides what is covered in bug 404541?

Comment 6

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

Changing summary since the only app that needs this is SeaMonkey because they chose to always load the UAC plugin and hence the common macro for unloading doesn't unload the plugin. I am fairly certain no other apps besides SeaMonkey are affected by this.

Comment 7

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

No one has stepped up to change the current behavior that works for all other apps besides SeaMonkey so I am resolving this as wontfix. If SeaMonkey wants to do things differently than other apps it is quite easy to just use custom code instead of the common code in common.nsh.