368587 - avoid the second UAC prompt for helper.exe on software update by launching it directly from the elevated updater.exe process

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

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

avoid the second UAC prompt (for firefoxHelper.exe) on software update by launching it directly from the elevated updater.exe process

see bug #368353 for details about firefoxHelper.exe.

after robert lands all his code for vista support to trunk/MOZILLA_1_8_BRANCH, here's what will happen on software update on windows (all platforms):

1) firefox.exe downloads the .mar
2) firefox.exe will launch updater.exe (and ask for elevated privs on vista), and will exit firefox.exe
3) updater.exe will apply the mar and start up firefox.exe (but will not elevate firefox.exe)
4) firefox.exe will execute the code in nsUpdateService.js which will execute the code in nsPostUpdateWin.js (which will do some work on the log files), which will in turn call the code which will launch firefoxHelper.exe (and ask for elevated privs on vista, see bug 354226 for more on this step.)  firefoxHelper.exe will finish the work on the log files, and modify the registry.

as you can see, steps 2 and 4 will result in UAC prompts.

we could elimate the second one, and remove nsPostUpdateWin.js if we ported code that works on the logs from nsPostUpdateWin.js to the installer code (remember, firefoxHelper.exe is an NSIS based application)

if we did that, we could then have this:

1) firefox.exe downloads the .mar
2) firefox.exe will launch updater.exe (and ask for elevated privs on vista), and will exit firefox.exe
3) updater.exe will apply the mar, launch firefoxHelper.exe, and start up firefox.exe (but will not elevate firefox.exe)

we'd remove nsPostUpdateWin.js, and we'd remove the code in nsUpdateService.js to launch it.  We still need to be able to launch firefoxHelper.exe (and elevate it) from inside firefox.exe, as we need it when we set the application as default.  but, we could also simplify the code that launches firefoxHelper.exe, as we only need the "firefoxHelper.exe /fixReg" scenario (we could remove the "firefoxHelper.exe /postupdate /uninstalllog=%s" scenario.)

this doesn;t block 2.0.0.2, but it something that both robert and I would like to fix.

note to mscott:  all this goes for tbird as well, with thunderbirdHelper.exe.

Comment 1

[Marcia Knous [:marcia]]

nominating just to get this on the radar. jay and triage team can move it to wanted for next release if they deem it appropriate.

Comment 2

[Jay Patel [:jay]]

seth:  you said that this doesn't block 2.0.0.2, but is this something you guys are trying to get in for this release?   if we can fit this in, it will be nice to have, but considering everything else happening right now, we can push this out to the next release.

Comment 3

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

in my opinion, this shouldn't block 2.0.0.2.

it is something we'd to fix in a future release, to improve the user experience and simplify the code.  (or, to be fair, move code from firefox to the installer)

Comment 4

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

A little explanation... to accomplish this we are going to move the update and old xpinstall uninstall log parsing for adding new entries for the uninstaller into the NSIS code. The NSIS code is much less efficient at parsing these logs and there is the possibility of there being numerous old xpinstall uninstall logs - I had an install that had over 100 for example. Since the NSIS code can take minutes to accomplish this I would like to hold off on this until 3.0 and at that time only parse a couple of the old xpinstall uninstall logs and the update log. The end result would be that some people that upgrade via software update from 1.5.0.x to 3.0 might not have all of the app's files removed.

Comment 5

[Daniel Veditz [:dveditz]]

If it didn't block 2.0.0.2 it probably shouldn't block any future point releases. If you make this better we'll consider approving a trunk-tested patch, though.

Comment 6

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

Attachment: work in progress rev1

Comment 7

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

still to do:
1. update the uninstall.log with the current update and old xpinstall logs.
2. audit the PostUpdate actions performed by NSIS (e.g. that we can check reg write failure and update HKCU accordingly when we have a manifest - see bug 370457 except with execution level asInvoker, etc.).
3. verify the log used to display update history is located in the app dir at the end of the update so it is available to all users of the system.

Comment 8

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

(In reply to comment #7)
> still to do:
>...
> 3. verify the log used to display update history is located in the app dir at
> the end of the update so it is available to all users of the system.
We use the one located in the user's profile. Submitted bug 388200 to cover this separately.

Comment 9

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

Attachment: NSIS patch wip rev1

Comment 10

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

Attachment: NSIS patch wip rev2 -w

Comment 11

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

Attachment: patch -w rev1

Comment 12

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

Attachment: patch

Comment 13

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

Comment on attachment 273020 [details] [diff] [review]
patch -w rev1

Found a build problem with this patch... will re-submit after I get that worked out

Comment 14

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

Attachment: patch -w rev2

The previous patches had a couple of extra spaces in a Makefile.in... other than that this is the same.

Comment 15

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

Attachment: patch rev2

Comment 16

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

Comment on attachment 273045 [details] [diff] [review]
patch -w rev2

ignore the change to shared.nsh... I've added that to the patch in bug 388932

Comment 17

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

One thing (e.g. oops) that came to mind this weekend is that the new updater.ini is the one I should be reading... besides that I believe the process flow is correct.

Comment 18

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

Comment on attachment 273045 [details] [diff] [review]
patch -w rev2

r=sspitzer

can you log a spin off bug about the code we should remove once bug 386760 is fixed?

Comment 19

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

Comment on attachment 273045 [details] [diff] [review]
patch -w rev2

clearing review request, I believe robert is working on a new version that addresses the ini file location issue.

Comment 20

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

Attachment: patch -w rev3

Benjamin, this has a change from attachment 273045 [details] [diff] [review] which sspitzer r+'d earlier in that this uses the updated updater.ini in the app dir. I'm unsure if this change will adversely affect xulrunner apps and whether this is the best way to append the updater_append.ini to updater.ini.

Comment 21

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

Attachment: patch rev3

Comment 22

[Mike Connor [:mconnor]]

Missing the freeze, moving out.

Comment 23

[Benjamin Smedberg]

Comment on attachment 273752 [details] [diff] [review]
patch -w rev3

I don't know how this would affect any XR apps. The only one I know has update working is Songbird, so we should ask bent.

Comment 24

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

Ben, heads up about changes to updater.exe and how we will make the post update changes on Win32. I think this should be flexible enough for your needs and if it isn't please file a new bug and cc me.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks Rob. Sounds like this should work well.

Comment 26

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

Landed on Trunk with a verbal a=mconnor

Checking in mozilla/toolkit/mozapps/update/src/Makefile.in;
/cvsroot/mozilla/toolkit/mozapps/update/src/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in mozilla/toolkit/mozapps/update/src/nsPostUpdateWin.js;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsPostUpdateWin.js,v  <--  nsPostUpdateWin.js
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/toolkit/mozapps/update/src/updater/updater.cpp;
/cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v  <--  updater.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in mozilla/browser/locales/Makefile.in;
/cvsroot/mozilla/browser/locales/Makefile.in,v  <--  Makefile.in
new revision: 1.52; previous revision: 1.51
done
RCS file: /cvsroot/mozilla/browser/installer/windows/nsis/updater_append.ini,v
done
Checking in mozilla/browser/installer/windows/nsis/updater_append.ini;
/cvsroot/mozilla/browser/installer/windows/nsis/updater_append.ini,v  <--  updater_append.ini
initial revision: 1.1
done
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.18; previous revision: 1.17
done

Comment 27

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

Note: you will still get the second UAC prompt until you receive your second Software Update after this patch has landed. From that point forward you should only get one UAC prompt. I'll provide steps for verifying this is fixed later.

Comment 28

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

Attachment: supplemental bustage fix - checked in

Comment 29

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

To verify (must be running Vista):

1. Install the previous nightly build using the installer.
2. Copy the uninstall.log from the appdir's uninstall directory to your desktop.
3. Open the uninstall.log and remove the following:
   File: \freebl3.chk
   File: \softokn3.chk
4. Copy the uninstall.log you just modified to the appdir's uninstall directory and replace the existing uninstall.log.
4. Open regedit and navigate to
   HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion\Uninstall\Minefield (3.0a7pre)
5. Change the value of Comments to Minefield bogus
6. Run Firefox, check for updates, install the update, and click restart.
7. You should receive a UAC prompt to elevate updater.exe - click allow.

After the previous steps have been completed the following should be observed.
1. A UAC prompt for helper.exe was NOT displayed.
2. The uninstall.log in the appdir's uninstall directory should have the following added:
   File: \freebl3.chk
   File: \softokn3.chk
3. The value of Comments under:
   HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion\Uninstall\Minefield (3.0a7pre)
   should be Minefield

Comment 30

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

Got my numbering wrong above but I'm sure you get the jist of it! ;)

Comment 31

[Carsten Book [:Tomcat]]

verified fixed on trunk with  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/200707310505 Minefield/3.0a7pre as the old build and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/200708020404 Minefield/3.0a7pre as the patched build.

I followed the steps to reproduce from rstrong and can confirm that :
- no second UAC prompt for helper.exe was displayed during update (only the one for updater.exe)

- File: \freebl3.chk and File: \softokn3.chk was added to the uninstall.log after update (was deleted on the uninstall.log of 200707310505)

and HKEY_LOCAL_MACHINE\Software\Microsoft\Windows\CurrentVersion\Uninstall\Minefield (3.0a7pre) was renamed after the Update to Minefield (after renaming in 200707310505 to Minefield Bogus)

-> changing status to verified fixed

Comment 32

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

Tomcat, thanks and you rock!

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 273752 [details] [diff] [review]
patch -w rev3

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 34

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

Checked in to MOZILLA_1_8_BRANCH

note: I decided not to bother checking in the change to mozilla/toolkit/mozapps/update/src/Makefile.in since it is just a correctness fix not necessary to fix this bug.

Checking in mozilla/toolkit/mozapps/update/src/nsPostUpdateWin.js;
/cvsroot/mozilla/toolkit/mozapps/update/src/nsPostUpdateWin.js,v  <--  nsPostUpdateWin.js
new revision: 1.2.2.10; previous revision: 1.2.2.9
done
Checking in mozilla/toolkit/mozapps/update/src/updater/updater.cpp;
/cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v  <--  updater.cpp
new revision: 1.11.4.14; previous revision: 1.11.4.13
done
Checking in mozilla/browser/locales/Makefile.in;
/cvsroot/mozilla/browser/locales/Makefile.in,v  <--  Makefile.in
new revision: 1.25.2.19; previous revision: 1.25.2.18
done
Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.1.2.6; previous revision: 1.1.2.5
done
Checking in mozilla/browser/installer/windows/nsis/updater_append.ini;
/cvsroot/mozilla/browser/installer/windows/nsis/updater_append.ini,v  <--  updater_append.ini
new revision: 1.1.2.1; previous revision: 1.1
done
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.2.2.18; previous revision: 1.2.2.17
done

Comment 35

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

Attachment: branch patch as checked in

Comment 36

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

note: the second UAC prompt will still be seen during the next software update and subsequent software updates after the next one should not display the second UAC prompt. See comment #29 for information on how to verify.

Comment 37

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.7 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.7pre) Gecko/2007090703 BonEcho/2.0.0.7pre and per final update to Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.7pre) Gecko/2007091103 BonEcho/2.0.0.7pre

Same results as in comment #31 with the steps to verify:

-Registry was renamed to Bon Echo after changed to Bon Echo Bogus and File: \freebl3.chk and File: \softokn3.chk was added to the uninstall.log
after update

also no second UAC prompt after the final update from 2007-09-10 build to the 2007-09-11 build see comment #36

adding verified keyword

Comment 38

[Marcia Knous [:marcia]]

Litmus Triage Team: Tomcat will cover the Litmus test case for this one.

Comment 39

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5086 has been added to the Vista specific test suite.