790667 - Setting default browser should never prompt for UAC on Windows 8

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Brian R. Bondy [:bbondy]]

When we set the default browser, depending on what was the default browser before us, we sometimes prompt for UAC.

This has been bothering me for a while but I didn't fully understand what was going on until now.

Inside shared.nsh Function SetAsDfaultAppUser we are making a wrong assumption:

> ; It is only possible to set this installation of the application as the
> ; StartMenuInternet handler if it was added to the HKLM StartMenuInternet
> ; registry keys.
> ; http://support.microsoft.com/kb/297878

The reason for the confusion is this line in the linked article: "Applications which register as Start Menu Internet Applications do so across the entire system (per-machine)" but then it goes on to explain that you can also set HKCU after that.
The start menu internet handler can be set for HKCU and it will be used by default.  If you set HKLM as well then it'll be used for users who don't have an HKCU value set. 

What we should do:
------------------
- When installing we should set both HKLM and HKCU.
- If the user sets the default browser and they are elevated, then we should set HKLM and HKCU
- If the user is not elevated, then we should only set HKCU

This is especially important in Windows 8 when if you have both a UAC prompt and a Windows popup to select your browser, it's a really bad experience.

Comment 1

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

There are registry values under HKLM that are required when setting as default browser and if they are incorrect we must elevate. Specifically, the Capabilities key on Vista and above which we just happen to write under the StartMenuInternet\FIREFOX.EXE key are needed and must be under HKLM.

I think the checks for inconsistencies in the registry values can be made more specific based on the OS version to make it so we elevate less often when setting as default.

In addition, on Windows 8 and possibly other Windows versions I think it might be appropriate to set HKLM and HKCU to prepare to set Firefox as the default on install per previous MS recommendations for browser apps though that should be verified.

The reason it doesn't just set them is so people testing a second install doesn't have their keys left in an inconsistent state and the above would fix that (e.g. taking over existing settings for a Firefox install thereby breaking an existing Firefox install set as default at install time).

Comment 2

[Brian R. Bondy [:bbondy]]

I'm working on a patch now but I'll test all the things you mentioned above. Thanks for the info.  
I'm thinking we can get out of the way more for people who want to set us as the default browser without a UAC prompt.  For example chrome can set default browser fine and it only writes in HKCU entries.

Comment 3

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

At least in the past when I checked out how chrome got around things on Vista and Windows 7 it did request elevation one time due to the reasons I stated. Also, chrome didn't back then allow setting canary builds as default which is likely because they didn't want to deal with the registry settings being inconsistent and needing to elevate to fix them.

Comment 4

[Brian R. Bondy [:bbondy]]

I verified that chrome does elevate on Win7, but not on win8, and everything works correctly on win8 with the capabilities listed under HKCU.  So I think my changes will be gated on if win8 as you suggested.

Comment 5

[Brian R. Bondy [:bbondy]]

Adding tracking '?' flags since this makes for a really poor user experience in win8 when setting default browser.  Having 2 different windows popup outside of the browser window and expecting the user to know what to do is hard.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1

This task is in addition to the work that was done in bug 791019.  If you want to apply the patch for this task, you first need to apply the patch in bug 791019.  Alternately you can just use elm which has all the work already. (or will in 5 min)

Comment 7

[Brian R. Bondy [:bbondy]]

The approach is basically to keep all of the HKLM stuff but also add things to HKCU.  When we want to check what the current value is, we first check the HKCU one and then the HKLM one.

Comment 8

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/projects/elm/rev/e361cc8544d2
(Will sync review comments to elm)

Comment 9

[Brian R. Bondy [:bbondy]]

Actually this applies cleanly even without bug 791019.  But I'd like both of them to land on m-c, aurora, and beta.

Comment 10

[bhavana bajaj [:bajaj]]

Tracking, based on comment 5

Comment 11

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

Comment on attachment 661323 [details] [diff] [review]
Patch v1

>diff --git a/browser/installer/windows/nsis/shared.nsh b/browser/installer/windows/nsis/shared.nsh
>--- a/browser/installer/windows/nsis/shared.nsh
>+++ b/browser/installer/windows/nsis/shared.nsh
>@@ -29,33 +29,49 @@
>...
>-    ; Only update the Clients\StartMenuInternet registry key values if they
>-    ; don't exist or this installation is the same as the one set in those keys.
>+    ; Only update the Clients\StartMenuInternet registry key values in HKLM if
>+    ; they don't exist or this installation is the same as the one set in those
>+    ; keys.
>     ${StrFilter} "${FileMainEXE}" "+" "" "" $1
>     ReadRegStr $0 HKLM "Software\Clients\StartMenuInternet\$1\DefaultIcon" ""
>     ${GetPathFromString} "$0" $0
>     ${GetParent} "$0" $0
>     ${If} ${FileExists} "$0"
>       ${GetLongPath} "$0" $0
>     ${EndIf}
>     ${If} "$0" == "$INSTDIR"
>-      ${SetStartMenuInternet} ; Does not use SHCTX
>+      ${SetStartMenuInternet} "HKLM"
>+    ${EndIf}
I'm not sure if this changed from the original implementation but it doesn't set those keys if they don't exist like the comment says. Let's talk about this today for a bit.

>+    ; Only update the Clients\StartMenuInternet registry key values in HKCU if
>+    ; they don't exist or this installation is the same as the one set in those
>+    ; keys.  This is only done in Windows 8 to avoid a UAC prompt.
>+    ${If} ${AtLeastWin8}
>+      ReadRegStr $0 HKCU "Software\Clients\StartMenuInternet\$1\DefaultIcon" ""
>+      ${GetPathFromString} "$0" $0
>+      ${GetParent} "$0" $0
>+      ${If} ${FileExists} "$0"
>+        ${GetLongPath} "$0" $0
>+      ${EndIf}
>+      ${If} "$0" == "$INSTDIR"
>+        ${SetStartMenuInternet} "HKCU"
>+      ${EndIf}
>     ${EndIf}
Same here

>@@ -126,31 +142,34 @@
> !macroend
> !define PostUpdate "!insertmacro PostUpdate"
> 
> !macro SetAsDefaultAppGlobal
>   ${RemoveDeprecatedKeys} ; Does not use SHCTX
> 
>   SetShellVarContext all      ; Set SHCTX to all users (e.g. HKLM)
>   ${SetHandlers} ; Uses SHCTX
>-  ${SetStartMenuInternet} ; Does not use SHCTX
>-  ${FixShellIconHandler} ; Does not use SHCTX
>+  ${SetStartMenuInternet} "HKLM"
>+  ${FixShellIconHandler} "HKLM"
>   ${ShowShortcuts}
>   ${StrFilter} "${FileMainEXE}" "+" "" "" $R9
>   WriteRegStr HKLM "Software\Clients\StartMenuInternet" "" "$R9"
> !macroend
> !define SetAsDefaultAppGlobal "!insertmacro SetAsDefaultAppGlobal"
> 
> ; Removes shortcuts for this installation. This should also remove the
> ; application from Open With for the file types the application handles
> ; (bug 370480).
> !macro HideShortcuts
>   ${StrFilter} "${FileMainEXE}" "+" "" "" $0
>   StrCpy $R1 "Software\Clients\StartMenuInternet\$0\InstallInfo"
>   WriteRegDWORD HKLM "$R1" "IconsVisible" 0
>+  ${If} ${AtLeastWin8}
>+    WriteRegDWORD HKCU "$R1" "IconsVisible" 0
I'm kind of surprised by this. Are you sure this is actually used by Win8?

>+  ${EndIf}
>...
>@@ -200,16 +219,19 @@
> !define HideShortcuts "!insertmacro HideShortcuts"
> 
> ; Adds shortcuts for this installation. This should also add the application
> ; to Open With for the file types the application handles (bug 370480).
> !macro ShowShortcuts
>   ${StrFilter} "${FileMainEXE}" "+" "" "" $0
>   StrCpy $R1 "Software\Clients\StartMenuInternet\$0\InstallInfo"
>   WriteRegDWORD HKLM "$R1" "IconsVisible" 1
>+  ${If} ${AtLeastWin8}
>+    WriteRegDWORD HKCU "$R1" "IconsVisible" 1
Same here.

>+  ${EndIf}
>...
>@@ -339,91 +361,96 @@
> ; Adds the HKLM\Software\Clients\StartMenuInternet\FIREFOX.EXE registry
> ; entries (does not use SHCTX).
> ;
> ; The values for StartMenuInternet are only valid under HKLM and there can only
> ; be one installation registerred under StartMenuInternet per application since
> ; the key name is derived from the main application executable.
> ; http://support.microsoft.com/kb/297878
> ;
>+; In Windows 8 this changes slightly, you can store StartMenuInternet entries in
I suspect Win8 only cares about the Capabilities key. You should be able to check by setting the default system level browser. If it doesn't then much of the code won't be needed.

>+; HKCU.  The icon in start menu for StartMenuInternet is deprecated as of Win7,
>+; but the subkeys are what's important.  Control panel default programs looks
>+; for them only in HKLM pre win8.
>+;
> ; Note: we might be able to get away with using the full path to the
> ; application executable for the key name in order to support multiple
> ; installations.
>-!macro SetStartMenuInternet
>+!macro SetStartMenuInternet RegKey

r- mainly to get answers to the questions.

Comment 12

[Brian R. Bondy [:bbondy]]

I don't have the answer to all of these, although I may be able to get them. The general strategy when implementing this task was to: i) Do everything in HKCU relating to StartMenuInternet that we did in HKLM, ii) Test to make sure everything worked even when removing the HKLM entries.

Comment 13

[Alex Keybl [:akeybl]]

Looks like this will miss FF16 beta 4 - are we concerned with the risk to Win7 and earlier if we make this change before beta 5?

Comment 14

[Brian R. Bondy [:bbondy]]

I think it is very low risk for win7 and below, almost all changes are within checks for win8 and above

Comment 15

[Brian R. Bondy [:bbondy]]

rstrong, just to make sure:
You are OK as discussed on IRC if I post Comment 11 in a new bug for implementation on m-c only and land this as is correct?  If so I think that's the route I will go with.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1'

Rebased since the win8 installer patch with MOZ_METRO will land first.
As mentioned previously I'd file a followup to get rid of the extra entries which may not be needed in HKCU.

Comment 17

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

Comment on attachment 662567 [details] [diff] [review]
Patch v1'

Yes, I'm fine with that

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 662567 [details] [diff] [review]
Patch v1'

[Approval Request Comment]
Bug caused by (feature/regressing bug #): --- Microsoft releasing win8 while beta is out --- 
User impact if declined: Users will sometimes have both a UAC prompt and control panel
Testing completed (on m-c, etc.): I've tested with tbpl builds only.  But my testing involved having this bug, so it should be included.
Risk to taking this patch (and alternatives if risky): Low pre win7, medium win8. 
String or UUID changes made by this patch: none

Comment 19

[Alex Keybl [:akeybl]]

https://hg.mozilla.org/mozilla-central/rev/3c68fdd4f77a

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 662567 [details] [diff] [review]
Patch v1'

[Triage Comment]
Approving for Aurora/Beta and adding verifyme to re-test the installer experience on <=Win7 and Win8.

Comment 21

[Brian R. Bondy [:bbondy]]

Comment on attachment 662567 [details] [diff] [review]
Patch v1'

I know this is already approved, but I'm going to do a rollup patch for the involved bugs for defaults handling on Windows 8 and ask for approval all at once in bug 791019.  If that lands on aurora and beta I'll update the tracking fields in this bug and post the changesets in this bug as well.

Comment 22

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-beta/rev/4130a141a36b
http://hg.mozilla.org/releases/mozilla-aurora/rev/030540c0d243

Comment 23

[Simona Badau, Desktop QA]

Brian,
I was trying to verify this bug on Firefox 16 beta 5 and I can confirm that no UAC prompts when setting Firefox as the default browser from the Tools menu -> Options -> Advanced -> Make Firefox the default Browser, but neither does on Firefox 16 beta 3. I get prompted instead by a flyout that asks: How do you want to open this type of link? Is this part of the UAC also?

Comment 24

[Brian R. Bondy [:bbondy]]

You can reproduce the UAC prompt by installing a different firefox, for example Aurora into a different location, then set Aurora as your default browser.  Then go back and do the steps, it should prompt you for UAC only with the older beta.

Comment 25

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0

Verified on Firefox 16 beta 5 that the UAC is not prompted when setting Firefox as the default browser (I used the instructions from Comment 24).

Thank you Brian for the guidance.

Comment 26

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0

Verified on Firefox 17 beta 3 that the UAC is not prompted when setting Firefox as the default browser.

Comment 27

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0

Verified on Firefox 18 beta 1 (using the instructions from Comment 24) - that the UAC is not prompted when Firefox setting the default browser.

Comment 28

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old