Robert Strong (they/them - no direct email) Reporter
	Description • 18 years ago

When an app update is applied Vista's User Account Control (UAC) prompts the user to grant updater.exe elevated priv's to apply the update. Afetr this has completed the app runs and nsPostUpdateWin.js attempts to update registry keys and log files which fails on Vista since the app has not been granted elevated priv's.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 1 • 18 years ago

robert just explained that the way he would recommend we fix this is to move (and port from js to C++) the code from nsPostUpdateWin.js to toolkit/mozapps/update/src/updater/updater.cpp.

as for the values that nsPostUpdateWin.js determines from the application (such as vendorShortName, brandFullName, version, etc), we could have those in updater.ini, so that as updater.exe runs, and it updates the files (including updater.ini, it could then read the updated updater.ini for those values.

robert:  I'm not sure though how we'd handle app.update.vendorName.override and general.useragent.locale (see nsPostUpdateWin.js), as those could be override ridden by prefs, so not available for putting into updater.ini at build time.

	Robert Strong (they/them - no direct email) Reporter
	Comment 2 • 18 years ago

I believe we should be able to add the locale and vendor name to the ini file as well since updater.ini is localized, etc.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 3 • 18 years ago

taking.

robert gave me a brain dump yesterday getting me up to speed on this bug and nsPostUpdateWin.js, and I've begun working on it.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 5 • 18 years ago

per robert strong suggestion, I chatted with bsmedberg, and while he hasn't reviewed my incomplete (and not-ready-for-review) patch, he is OK with me using map, list, fstream and string from the Standard C++ library (and STL) for this windows only code. 

	Darin Fisher
	Comment 6 • 18 years ago

Are you sure you want to be using std::string and not std::wstring?  Won't this have issues on systems where unicode file paths are required?

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 7 • 18 years ago

> Are you sure you want to be using std::string and not std::wstring?  Won't this
> have issues on systems where unicode file paths are required?

thanks darin, you are right.  

the JS code that I ported used uses JS string (which is unicode), and I should be using wstring (unicode), not string (ascii)

but, when I test, I'll make sure to test on a system where unicode file paths are required.

	Jungshik Shin
	Comment 8 • 18 years ago

(In reply to comment #7)
> > Are you sure you want to be using std::string and not std::wstring?  Won't this
> > have issues on systems where unicode file paths are required?

So, on Windows and VC++, 'std::wstring' is always guaranteed to be UTF-16, right?
 
> thanks darin, you are right.  
> 
> the JS code that I ported used uses JS string (which is unicode), and I should
> be using wstring (unicode), not string (ascii)

The following will be done when you switch to std::wstring, but just in case ...
Not only that, you also need to use 'W' APIs explicitly. Alternatively, you need to include '#define UNICODE' in files you're adding so that 'W' APIs are used implicitly. Note that we dropped the support for Win 9x/ME (see, for instance, bug 359808) so that we're free to use 'W' APIs and we MUST use them in any code we  add for Gecko 1.9.
 

	Benjamin Smedberg
	Comment 9 • 18 years ago

Since this code is targeted at ff 2.0.0.* the code needs to be win9x safe.

	Jungshik Shin
	Comment 10 • 18 years ago

(In reply to comment #9)
> Since this code is targeted at ff 2.0.0.* the code needs to be win9x safe.

I'm sorry I missed that. It's a bit of irony that non-Unicode APIs need to be used to support a brand new Vista that has by far the best unicode support among Windows*. However, adding them wouldn't make any practical difference for FF 2.0 because installing FF 2.0 in a directory whose name contains characters outside the system code page does not work for various reasons (see, e.g., bug 360735 and bug 169712).

However, we should not do that for Gecko 1.9 because that's going backward. I guess somehow a way to deal with the requirement(?) for a new patch to be baked in the trunk before getting landed in the branch needs to be devised, though.
(land it in the trunk with an understanding that it'll be removed as soon as it's baked long enough for landing in the branch ?)

	Mike Connor [:mconnor]
	Comment 11 • 18 years ago

moving out for 2.0.0.2

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 12 • 18 years ago

rob strong points out that one way to test this code (including the uninstall.log piece) is to try with a localized windows shortcut .lnk file

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 20 • 18 years ago

in this patch:

1)  copy over all the keys from the old install, and not just "Program Folder Path".  this was a problem with nsPostUpdateWin.js that rob strong pointed out a while ago.  now we copy over "Start Menu Folder" "Create Quick Launch Shortcut" "Create Desktop Shortcut" and "Create Start Menu Shortcut" as well.

2) remove the ifdef around the remove() call for removing the log files

3) for DirPathsAreEqual(), make it work properly for directories without trailing slashes (by ensuring the path names have trailing slashes).

4) set the default value for certain keys, like rob does in installer.nsi.

for example:  HKEY_LOCAL_MACHINE|]\SOFTWARE\Mozilla\Mozilla Firefox\2.0.0.1 (en-US)" value is "2.0.0.1 (en-US)".  (xxx todo, ask rob why we do this.)

5)  fix key name to be "Comments", like installer.nsi (nsPostUpdateWin.js did "Comment", which looks like a bug, according to the msdn docs.  will confirm with rob.)

more code cleanup.

this patch passes my first rounds of testing on Windows XP, with unicode, on an en-US system.

next up:

1)  work on some additional todo items from initial conversation with rob.
2)  testing on Vista, Windows9x
3)  test on japanese system and system with non ascii path names
4)  figure out branding issues with updater.ini
5)  move to xulrunner application.ini per conversation with bsmedberg

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 21 • 18 years ago

talked to rob, and he has clarified the following open issues:

> set the default value for certain keys, like rob does in installer.nsi.
> for example:  HKEY_LOCAL_MACHINE|]\SOFTWARE\Mozilla\Mozilla Firefox\2.0.0.1
> (en-US)" value is "2.0.0.1 (en-US)".  (xxx todo, ask rob why we do this.)

yes, we want postupdate_win.cpp to do this, following installer.nsi.   I'll fix the comment in the code.

> 5)  fix key name to be "Comments", like installer.nsi (nsPostUpdateWin.js did
> "Comment", which looks like a bug, according to the msdn docs.  will confirm
> with rob.)

yes, this is a bug with the current nsPostUpdateWin.js which will be fixed with postupdate_win.cpp.  I'll fix the comment in the code.

> work on some additional todo items

the remaining item (from rob's initial brain dump) is to walk the children of <ROOT>\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall, look for items with matching InstallLocation values, and remove them *before* we create the new key.

working on that now.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 23 • 18 years ago

before the holiday break, robert and I chatted about this patch and a problem.

in the past, this approach worked because after installing an update, nsPostUpdateWin.js would be run when the "updated" browser would first start up.

with this approach (moving the logic into updater.exe), there is a problem.  If this fix is part of updater.exe for 2.0.0.2, it *will not* be run when the user upgrades to 2.0.0.2.  That will execute the 2.0.0.1 updater.exe already on disk.  The 2.0.0.2 updater.exe will be run when the users upgrades 2.0.0.2 to 2.0.0.3.

	Jay Patel [:jay]
	Comment 24 • 18 years ago

seth:  so what's the latest on this?  is this patch good enough for us now?  i guess fixing it now and cleaning things up in 2.0.0.3 is better than nothing.  any other ideas to get around the problem you mentioned?

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 25 • 18 years ago

> any other ideas to get around the problem you mentioned?

robert strong had an interesting idea about how to get around the chicken and egg problem, which was to make firefox.exe launch updater.exe.

details:

1) make the changes in this bug to updater.exe (fx 2.0.0.2)
2) make additional changes to firefox.exe (for fx 2.0.0.2) that would launch updater.exe to update the registry.  the fix would be to the startup code, and it would first check if updater.exe had been run for version 2.0.0.2.
3) if not, it would run updater.exe (the new one, for fx 2.0.0.2) and updater.exe would set a new reg key indicating it had been run for version 2.0.0.2.

note, for 2.0.0.1 -> 2.0.0.2, you'd get the UAC prompt twice: once for the 2.0.0.1 updater.exe, and once again for the 2.0.0.2 updater.exe launched from the startup of fx 2.0.0.2

for the update to fx 2.0.0.3 we'd only get the UAC prompt once because, the fx 2.0.0.2 updater.exe would update the registry and set the reg key indicating it had been run for 2.0.0.3, so the code firefox.exe startup would not run it again.

I think this is something we could do for fx 2.0.0.2, but I like bsmedberg's idea in bug #366846 better, since it is cleaner.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 26 • 18 years ago

when rob proposed this idea, he also mentioned that when we launched updater.exe from the startup code, we'd pass a special param so that we'd only execute the chicken-and-egg code that we cared about, and not the code to apply an update.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 27 • 18 years ago

actually, what about this idea to solve the chicken and the egg problem, and simplify my patch:

keep nsPostUpdateWin.js, but make it a simple stub that will launch updater.exe (with special arguments), but only if something has changed.  nsPostUpdateWin.js already has code that reads the registry to see if something has changed, and it does not need elevated privileges to do that.

if something has changed, nsPostUpdateWin.js will launch updater.exe with the following arguments:

updater.exe "VendorShortName" "AppVersion" "Locale" "PlatformVersion" "AppProcessName" "BrandFullName" "URLInfoAbout" "URLUpdateInfo"

in the real world, that would be something like:

updater.exe "Mozilla" "2.0.0.1" "en-US" "1.8.1.1" "firefox.exe" "Mozilla Firefox" "http://www.mozilla.org/" "http://www.mozilla.org/products/firefox/"

nsPostUpdateWin.js already has code to get these values, so it would pass them to updater.exe, and I would not need my changes to determine these values (so update.ini would not change.)

I would fix updater.cpp so that, on windows only, if it got these command line arguments, only then would it call PostUpdate(), which is the C++ port of the code in nsPostUpdateWin.js that writes to the registry, updates log files, etc.

as far as the chicken and the egg, that problem is solved because nsPostUpdateWin.js is executed by the updated version of firefox (fx 2.0.0.2) after update.  if changes are needed, it will launch the new updater.exe (fx 2.0.0.2) which will have my fix.

benjamin / robert, what do you think?

	Benjamin Smedberg
	Comment 28 • 18 years ago

That sounds like a fine plan. Is there any particular reason to keep this functionality in updater.exe instead of a separate .exe?

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 29 • 18 years ago

correction, my example should have been something like:

updater.exe "Mozilla" "2.0.0.2" "en-US" "1.8.1.2" "firefox.exe" "Mozilla
Firefox" "http://www.mozilla.org/" "http://www.mozilla.org/products/firefox/"

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 30 • 18 years ago

> That sounds like a fine plan. Is there any particular reason to keep this
> functionality in updater.exe instead of a separate .exe?

thanks for the response, benjamin.

as for why to use updater.exe instead of a separate .exe, i think we are going to sign updater.exe, so using updater.exe means we'll have to sign, and the user will have to trust, one less application.  but I'll check that with a mozilla vista guru.

I'll start on my plan.  if using a separate .exe turns out to be the right thing, I'll do that.

	Benjamin Smedberg
	Comment 31 • 18 years ago

I'm pretty sure we are signing all DLLs and EXEs now.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 34 • 18 years ago

added code to look for SOFTWARE\Classes\FirefoxHTML\shell\open\command, and fix the value to be:

<apppath> -url "%1" -requestPending

also, fix DirPathsAreEqual() to be case insensitive which matters when we have FIREFOX.EXE, firefox.exe, etc.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 35 • 18 years ago

updating ddeexec keys under <root>\SOFTWARE\Classes\FirefoxHTML\shell\open

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 36 • 18 years ago

what's new:

1)  if we update the FirefoxHTML key, ensure and set the FirefoxURL keys
2)  when writing the app path to the registry, use helper method GetShortPath()
3)  take care of the "URL Protocol" (thanks to robert for spotting that.)

chatted with robert, and Launch() is not the right way to start up postupdater.exe.

Instead, the plan is to keep the check in nsPostUpdateWin.js, but call it from startup, and use WinLaunchChild (from our startup code) to create postupdater.exe with elevated privs.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 37 • 18 years ago

updates:

1)  port over robert's "clean up ifexec" cruft from the installer
2)  fix the "-requestPending" for the following protocols HTTP, HTTPS, FTP, GOPHER

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 38 • 18 years ago

fix the Launch() vs WinLaunchChild() problem by adding a new interface, nsIPostUpdaterLauncher, which is only built on windows.

the nsXULAppInfo singleton implements it (again, only on windows).  nsPostUpdateWin.js uses the launch() method of this interface to call WinLaunchChild() with the path to postupdater.exe.

	Masatoshi Kimura [:emk]
	Comment 39 • 18 years ago

> chatted with robert, and Launch() is not the right way to start up
> postupdater.exe.
Really? We will have to use ShellExecute (which is used by nsLocalFile::Launch()) rather than CreateProcess (which is used by WinLaunchChild()) at least until Fx 2.0.0.3. Although this patch is for Fx 2.0.0.2, it is not a good practice to depend on compatibility shims.

	Robert Strong (they/them - no direct email) Reporter
	Comment 40 • 18 years ago

We are going to need to request elevation the same way as the patch you wrote in bug 351949.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 41 • 18 years ago

robert had a *fantastic* idea this morning to simplify the solution to this bug:

1)  fix nsPostUpdateWin.js to not do any registry writing / creating.  that will all be done by the uninstaller exe, which already has this functionality and branding strings.

2)  fix nsPostUpdateWin.js to keep all the log file clean up code, but write the results to a temp log file, one that does not require firefox.exe to have elevated privs.

3)  remove postupdater.exe / postupdater.ini, not longer needed.

4)  nsPostUpdaterWin.js will make a check (still to be decided) and will launch (using the nsIPostUpdaterLaunch interface, to be renamed to nsIPostUpdateWinCleanup) the uninstaller exe with a command line argument (to indicate "do registry update cleanup") and the path to the temp log file.  the uninstaller exe will be launched with WinLaunchChild() and will (soon) do so and and ask for elevated privs.  It will do all the reg cleanup, and since it is elevated, it can move the log into the right place (where nsPostUpdateWin.js can not, as firefox.exe is not elevated.)

5)  additionally, in nsWindowsShellService::SetDefaultBrowser(), robert can write the C++ version of this code to launch the uninstaller process:

 var app =
    Components.classes["@mozilla.org/xre/app-info;1"].
    getService(Components.interfaces.nsIXULAppInfo).
    QueryInterface(Components.interfaces.nsIPostUpdateWinCleanup);
 
 app.makeDefault();
                                                                       
I'll go make my small part of these changes, and attach a patch for the trunk.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 45 • 18 years ago

nsPostUpdateWin.js is currently firefox specific (as it uses FirefoxHTML as it's magic check).  so for now, disable calling it unless MOZ_PHOENIX.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 50 • 18 years ago

mscott: for thunderbird, there are three places we need to worry about:

1) in nsPostUpdateWindow.js, we'll need to fix checkRegistry() [not yet landed] for tbird.

for firefox, we look for the -requestPending (part of robert's dde work for firefox) on the FirefoxHTML key.

+function checkRegistry()
+{
+  // XXX todo
+  // this is firefox specific
+  // figure out what to do about tbird and sunbird, etc   
+  LOG("checkRegistry");
+     
+  var result = false;
+  try {
+    var key = new RegKey();
+    key.open(RegKey.prototype.ROOT_KEY_CLASSES_ROOT, "FirefoxHTML\\shell\\open\\command", key.ACCESS_READ);
+    var commandKey = key.readStringValue("");
+    LOG("commandKey = " + commandKey);
+    // if "-requestPending" is not found, we need to do the cleanup
+    result = (commandKey.indexOf("-requestPending") == -1);

2) mozapps/update/src/nsUpdateService.js.in

+#ifdef MOZ_PHOENIX
+        // for now, this is firefox only.  
+        // we need to fix both nsPostUpdateWin.js and 
+        // the uninstaller to work for thunderbird and sunbird
+        //
         // Perform platform-specific post-update processing.
         if (POST_UPDATE_CONTRACTID in Components.classes) {
           Components.classes[POST_UPDATE_CONTRACTID].
               createInstance(Components.interfaces.nsIRunnable).run();
         }
+#endif

once we fix nsPostUpdateWin.js to be tbird friendly we can remove that ifdef.

3)  we'd need a thunderbirdHelper.exe (see #368353, it's an app based on our NSIS uninstaller that does all the log copying and registry clean up that nsPostUpdateWin.js used to do.)

I'll log a spin on bug on these issues.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 51 • 18 years ago

> I'll log a spin on bug on these issues.

see bug #368362

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 54 • 18 years ago

Comment on attachment 253264 [details] [diff] [review]
updated to log if app.update.log.PostUpdate (or app.update.log.all) is set, and some other changes, per robert strong

seeking additional review from bsmedberg.  robert wanted me to ask bsmedberg for his opinion on the nsIWinAppHelper name.

	Robert Strong (they/them - no direct email) Reporter
	Comment 55 • 18 years ago

Comment on attachment 253264 [details] [diff] [review]
updated to log if app.update.log.PostUpdate (or app.update.log.all) is set, and some other changes, per robert strong

HelperApp should be AppHelper... Seth said he would resubmit if I give him a pony

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 56 • 18 years ago

this matches what robert checked into the trunk.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 57 • 18 years ago

Comment on attachment 253292 [details] [diff] [review]
for my pony

seeking additional review from bsmedberg

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 59 • 18 years ago

fixed landed (by robert strong) on the trunk.  still need to back port to the MOZILLA_1_8_BRANCH.

thanks again to robert for his idea (in comment #41) that greatly simplified this fix.

(note to bsmedberg, I would still like a review from you, especially of the interface name and build changes.)

	Benjamin Smedberg
	Comment 60 • 18 years ago

Comment on attachment 253292 [details] [diff] [review]
for my pony

I vaguely don't like the name "helper.exe" or nsIWinAppHelper, but I don't have a better suggestion, so r=me ;-)

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 61 • 18 years ago

the #ifdef MOZ_PHOENIX change to nsUpdateService.js.in was not checked in, but we want it.

I've checked it in, but I'll spin up a new bug about it, so that we don't forget to backport it to the MOZILLA_1_8_BRANCH.

additionally, I'll mscott and lilmatt know about it, and what it means for mail and calendar.

	(not reading, please use seth@sspitzer.org instead) Assignee
	Comment 62 • 18 years ago

fixed on the branch.