This will disable the folder virtualization, then some software update/installer related bugs will be fixed.

that would be "asInvoker", right? Ted, can you act as manifest guru?

Yes, firefox.exe should have the requestedExecutionLevel="asInvoker". xpicleanup.exe should also have the requestedExecutionLevel="asInvoker" to get rid of the dependency on the "Specific Non-Installer" shim. And updater.exe and helper.exe should have the requestedExecutionLevel="requireAdministrator" because installer detection may be turned off by Group Policy.

Don't we use coded privilege elevation for updater/helper? http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsWindowsRestart.cpp#159 I don't think we use automatic installer detection for those, and I don't think we want them running elevated except when launched from our bootstrap code.

ShellExecute() with default (NULL) verb will elevate only if the executable is detected as an installer or embedded a manifest with requestedExecutionLevel="requireAdministrator". updater.exe and helper.exe require elevation even if it's launched via explorer. Just double click to confirm. ShellExecute() with "runas" verb will always elevate, but we didn't so. That said, I reconsidered using "runas" verb and requestedExecutionLevel="asInvoker" manifest. ("asInvoker" manifest is required to disable the automatic installer detection.) it will also fix bug 374900.

updater/helper should never be launched from the shell... only from the apprunner.

Be very careful not to elevate firefox.exe at all. If any "sometimes need to be administrator" activities (such as updating system-wide plugins) are currently in firefox.exe, they should be moved elsewhere. Also beware, that if Firefox is installed to a user-writable location (perhaps it was installed by a non-administrator), then administrator privileges are NOT needed to run updates for that particular implementation and "requireAdministrator" would be plain wrong. So put the elevation request in a place where it will only happen when truly necessary. Good candidates are 1. A small program to run other Fx executables as administrator, by specifying the desired executable on the command line. 2. A Vista-specific argument to a system function for running programs. Either way, this would allow code inside Firefox to determine on a case-by-case basis if elevation is needed or not.

(In reply to comment #5) > updater/helper should never be launched from the shell... only from the > apprunner. I suggested it only to confirm updater/helper will always request the elevation unless installer detection is disabled.

I don't have Vista to test on, but this should work. Someone want to verify on Vista?

We should probably do this for XULRunner and the XULRunner stub (which doesn't get a manifest at all, currently).

Comment on attachment 262673 [details] [diff] [review] add requestedExecutionLevel="asInvoker" to FF and TB manifests >Index: browser/app/firefox.exe.manifest >=================================================================== >RCS file: /cvsroot/mozilla/browser/app/firefox.exe.manifest,v >retrieving revision 1.3 >diff -u -5 -r1.3 firefox.exe.manifest >--- browser/app/firefox.exe.manifest 1 Oct 2005 02:10:17 -0000 1.3 >+++ browser/app/firefox.exe.manifest 24 Apr 2007 19:40:11 -0000 >@@ -17,6 +17,14 @@ > publicKeyToken="6595b64144ccf1df" > language="*" > /> > </dependentAssembly> > </dependency> >+<ms_asmv2:trustInfo xmlns:ms_asmv2="urn:schemas-microsoft-com:asm.v2"> Is there any reason not to use asm.v3? >+ <ms_asmv2:security> >+ <ms_asmv2:requestedPrivileges> >+ <ms_asmv2:requestedExecutionLevel level="asInvoker"> nit: how about setting uiAccess="false" as well... it is the default but it appears most people are explicitly setting it. Then if there is a bug with setting it as default we won't be exposed and if there is a bug with setting uiAccess we'll be in the same boat as everyone else (which seems to be just about everyone) setting it. With answers or changes r=me

(In reply to comment #8) > Created an attachment (id=262673) [details] > add requestedExecutionLevel="asInvoker" to FF and TB manifests > > I don't have Vista to test on, but this should work. Someone want to verify on > Vista? Note that this patch will have no effect on tinderbox build until bug 330231 is fixed. (It's a reason why I set dependency)

(In reply to comment #10) > >+<ms_asmv2:trustInfo xmlns:ms_asmv2="urn:schemas-microsoft-com:asm.v2"> > Is there any reason not to use asm.v3? I copied this verbatim from the "Windows Vista Developer Story" docs. Is there any reason to use v3? I know the documentation is few and far between, so I haven't seen anything authoritative either way. > >+ <ms_asmv2:security> > >+ <ms_asmv2:requestedPrivileges> > >+ <ms_asmv2:requestedExecutionLevel level="asInvoker"> > nit: how about setting uiAccess="false" as well... it is the default but it > appears most people are explicitly setting it. Then if there is a bug with > setting it as default we won't be exposed and if there is a bug with setting > uiAccess we'll be in the same boat as everyone else (which seems to be just > about everyone) setting it. Ok, I'll add that. I got the impression that it only had any effect if you were at admin level, but I may have been confused. (In reply to comment #11) > (In reply to comment #8) > > Created an attachment (id=262673) [details] [details] > > add requestedExecutionLevel="asInvoker" to FF and TB manifests > > > > I don't have Vista to test on, but this should work. Someone want to verify on > > Vista? > Note that this patch will have no effect on tinderbox build until bug 330231 is > fixed. (It's a reason why I set dependency) > (In reply to comment #11) >Note that this patch will have no effect on tinderbox build until bug 330231 is >fixed. (It's a reason why I set dependency) Yeah, I actually intend to fix that via bug 377100, which I'll check in tomorrow morning.

I don't know of any value either way regarding using v2 vs v3... I have seen v3 showing up more lately but nothing in the way of reasons to use one vs. the other. I did notice that the manifest parsing restart bug references using v3. http://support.microsoft.com/kb/921337 either way is fine with me on the version and thanks

Ok, I changed the NS to v3, and added uiAccess="false". I also updated the XR manifest while I was at it. These manifests probably aren't going to do us any good until we ensure that the tinderboxen are using objdirs, but I'll get to that.

Comment on attachment 262712 [details] [diff] [review] FF, TB and XR manifests, address comments [checked in] A Firefox build with this patch runs ok in Vista. I didn't do any serious testing of it though. I'm going to land this, but it won't have any effect until I can get bug 377100 fixed.

Comment on attachment 262712 [details] [diff] [review] FF, TB and XR manifests, address comments [checked in] Checked in, leaving this bug open until it's actually fixed in our nightly builds.

I'm getting this message when I compile Firefox trunk now with VC8SP1 on XPSP2. Is it expected? d:/mozbuild/mozilla/browser/app/firefox.exe.manifest : manifest authoring warning 81010002: Unrecognized Element "requestedPrivileges" in namespace "urn:schemas-microsoft-com:asm.v3".

I hadn't noticed that before, thanks. Microsoft says it's a known issue, and is fixed in the mt.exe that ships with the Vista SDK, as well as the one that will ship with VC9: https://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=237720 But it's just a warning, so it's harmless.

We don't have to wait for bug 377100 about branch because Fx2 is built by VC6.

Yeah, we could port this to branch easily if it's wanted.

This will fix many depending bugs. I strongly want to land this on the branch. Could you port this to branch?

The downside to that is that this patch hasn't been tested on trunk, because of bug 377100, so we're not sure of what other consequences it has.

This should be fixed on trunk, since the Firefox tinderboxen use objdirs now. Could someone with Vista do some testing?

I verified that the add-ons mgr. is now behaving correctly on Vista. The uninstall button for DOMi is now disabled. Thanks!

Does this patch work for the branches, or do we need a ported one? Code freeze is looming

Will have to catch this next time.

No regressions are reported. Ted, would you make a branch patch?

Comment on attachment 262712 [details] [diff] [review] FF, TB and XR manifests, address comments [checked in] This patch applies cleanly against the 1.8 branch. I don't have VC6, so I can't test it, but it's not a terribly complicated patch.

One of our distribution partner has made a complaint about this bug (actually Bug 371803.) Please land the patch on 1.8.1.7.

Comment on attachment 262712 [details] [diff] [review] FF, TB and XR manifests, address comments [checked in] I compiled 1.8 branch with this patch using VC6 and confirmed it fixed bug 371803. I believe this patch will have no significant side effect.

I believe that the reason we have not been able to land this on the branch is because those tinderboxes do not use objdirs, and so the manifest would be overwritten improperly. And we cannot switch to use objdirs on the branch because that (apparently) breaks the talkback build process.

(In reply to comment #31) We don't have to bothered to switch to use objdirs because VC6 doesn't generate manifest. See comment #19. I tested without objdirs at comment #30. IIRC, the branch is built using VC6. Am I missing something?

If the manifest is overwritten, bug 330231 should also occur on the branch. It's contrary to the fact. Bug 332031 is a trunk only bug.

> Bug 332031 is a trunk only bug. Sorry, bug 330231.

Comment on attachment 262712 [details] [diff] [review] FF, TB and XR manifests, address comments [checked in] approved for 1.8.1.7, a=dveditz for release-drivers

Checked in on the 1.8 branch: Checking in browser/app/firefox.exe.manifest; /cvsroot/mozilla/browser/app/firefox.exe.manifest,v <-- firefox.exe.manifest new revision: 1.2.38.2; previous revision: 1.2.38.1 done Checking in mail/app/thunderbird.exe.manifest; /cvsroot/mozilla/mail/app/thunderbird.exe.manifest,v <-- thunderbird.exe.manifest new revision: 1.2.38.2; previous revision: 1.2.38.1 done Checking in xulrunner/app/xulrunner.exe.manifest; /cvsroot/mozilla/xulrunner/app/xulrunner.exe.manifest,v <-- xulrunner.exe.manifest new revision: 1.1.12.1; previous revision: 1.1 done

-> v. with Fx 2.0.0.8

Anyone knows if this fix should be ported to Mozilla Sunbird too? There exists some issues with creating the initial profile on some (all?) Vista systems (Bug 428324) but the reason is unclear.

(In reply to comment #39) > Anyone knows if this fix should be ported to Mozilla Sunbird too? Definitely to prevent folder virtualization Bug 428324 appears to also affect XP per Bug 428324 comment #0 so I think it is unrelated.