Closed Bug 790667 Opened 8 years ago Closed 7 years ago

Setting default browser should never prompt for UAC on Windows 8

Categories

(Firefox :: Shell Integration, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18
Tracking Status
firefox15 --- wontfix
firefox16 + verified
firefox17 + verified
firefox18 + verified

People

(Reporter: bbondy, Assigned: bbondy)

References

(Depends on 1 open bug)

Details

(Whiteboard: [win8] [completed-elm])

Attachments

(1 file, 1 obsolete file)

19.24 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
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.
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).
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.
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.
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.
OS: Windows Vista → Windows 8
Summary: Setting default browser should never prompt for UAC → Setting default browser should never prompt for UAC on Windows 8
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.
Attached patch Patch v1 (obsolete) — Splinter Review
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)
Attachment #661323 - Flags: review?(robert.bugzilla)
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.
Depends on: 791019
http://hg.mozilla.org/projects/elm/rev/e361cc8544d2
(Will sync review comments to elm)
Whiteboard: completed-elm
Actually this applies cleanly even without bug 791019.  But I'd like both of them to land on m-c, aurora, and beta.
Whiteboard: completed-elm → [win8] [completed-elm]
Depends on: 791501
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.
Attachment #661323 - Flags: review?(robert.bugzilla) → review-
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.
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?
I think it is very low risk for win7 and below, almost all changes are within checks for win8 and above
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.
Attached patch Patch v1'Splinter Review
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.
Attachment #661323 - Attachment is obsolete: true
Attachment #662567 - Flags: review?(robert.bugzilla)
Comment on attachment 662567 [details] [diff] [review]
Patch v1'

Yes, I'm fine with that
Attachment #662567 - Flags: review?(robert.bugzilla) → review+
Depends on: 792662
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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
Attachment #662567 - Flags: approval-mozilla-beta?
Attachment #662567 - Flags: approval-mozilla-aurora?
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.
Attachment #662567 - Flags: approval-mozilla-beta?
Attachment #662567 - Flags: approval-mozilla-beta+
Attachment #662567 - Flags: approval-mozilla-aurora?
Attachment #662567 - Flags: approval-mozilla-aurora+
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.
Attachment #662567 - Flags: approval-mozilla-beta+
Attachment #662567 - Flags: approval-mozilla-aurora+
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?
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.
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.
QA Contact: simona.marcu
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.
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.
Blocks: 831612
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.