1184508 - Remember registry hash for later use on Win8+

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

We have to use the default apps UI on Win8+ because associations are protected by a cryptographic hash and we can't tell how to calculate the hash value from the scratch.
Once the hash value is calculated, however, we can remember the value to bypass the default apps UI from then onward. In particular, if Firefox was the default browser before upgrading from Windows 8.1, the user can take back the default browser from Microsoft Edge without being bothered by the boring UI.
Even if the hash value is changed, Firefox will fallback to the current method (i.e. launch the default apps UI). So this is a strict UX improvement.

Comment 1

[Jim Mathies [:jimm]]

What is the hash based on?

Comment 2

[Masatoshi Kimura [:emk]]

Just copying values calculated by the default apps UI. Therefore this patch can't bypass the dialog at first.
For file extensions, values are taken from HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\FIleExts\<ext>\UserChoice. For URL protocols, values are taken from HKCU\Software\Microsoft\Windows\Shell\Associations\UrlAssociations\<protocol>\UserChoice.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> Just copying values calculated by the default apps UI. Therefore this patch
> can't bypass the dialog at first.
> For file extensions, values are taken from
> HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\FIleExts\<ext>\UserCh
> oice. For URL protocols, values are taken from
> HKCU\Software\Microsoft\Windows\Shell\Associations\UrlAssociations\<protocol>
> \UserChoice.

I'm working on getting my win8 laptop updated and built so I can test this. I'll come back to it later today.

Brian you worked with these hashes back when we were doing win8 default browser updates. Any thoughts on the approach here - trying to preserve these values after they have been set?

Comment 4

[Brian R. Bondy [:bbondy]]

Maybe a bit intrusive but something like that works for me. I remember testing out an application for fast metro status sharing back when that was a thing with win8.  During install time it did the same thing of asking you to set the default for each first so it could copy out the value.  I think we have registry wrappers already, seems like overkill to make new ones.

Comment 5

[Jim Mathies [:jimm]]

None of this patch applies to tip cleanly, mind updating?

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: patch v2

* Rebased to tip.
* Used nsIWindowsRegKey instead of reinventing the wheel.

Comment 7

[Masatoshi Kimura [:emk]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf84b7da846

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8635647 [details] [diff] [review]
patch v2

Review of attachment 8635647 [details] [diff] [review]:
-----------------------------------------------------------------

ZGenerally looks great but I'm concerned about jank on the main thread.

::: browser/components/shell/nsWindowsShellService.cpp
@@ +276,5 @@
>                        0, nullptr, nullptr, &si, &pi)) {
>      return NS_ERROR_FAILURE;
>    }
>  
> +  if (aWait) {

This is on the main thread and will jank the entire browser UI for the duration. Are we sure we need this? I there any way we can do this async?

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: patch v3

(In reply to Jim Mathies [:jimm] from comment #8)
> This is on the main thread and will jank the entire browser UI for the
> duration. Are we sure we need this? I there any way we can do this async?

This is needed only if the default browser is changed from a different Firefox installation. So I think we can live without this. Most users will install only one Firefox instance. Also, Firefox will fallback to the old method even if it fails to set the default browser.

Comment 10

[Masatoshi Kimura [:emk]]

To be clear, v3 removed the aWait parameter.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8636289 [details] [diff] [review]
patch v3

Review of attachment 8636289 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/shell/nsWindowsShellService.cpp
@@ +370,5 @@
> +
> +  LPCWSTR progID = isProtocol ? L"FirefoxURL" : L"FirefoxHTML";
> +  bool isDefault = !wcsicmp(registeredApp, progID);
> +  CoTaskMemFree(registeredApp);
> +  if (!isDefault) {

looks like you can return isDefault here instead.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: patch for checkin

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2645ea066437

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7c48a3782efe

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c48a3782efe

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

This looks like a patch that we'll want on Firefox 40 so we can get it to users hopefully before they upgrade to Windows 10 and lose their default web browser association.

:emk, I read through the code and it looks like we'll want it for Firefox 40. Can you confirm that if we get this on Firefox 40 and users run it on Windows 8 before updating to Windows 10, their "default browser" experience will be a lot better? (note that we only have a few more days to get the uplift requests in and landed on beta before the final betas will be built)

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> This looks like a patch that we'll want on Firefox 40 so we can get it to
> users hopefully before they upgrade to Windows 10 and lose their default web
> browser association.
> 
> :emk, I read through the code and it looks like we'll want it for Firefox
> 40. Can you confirm that if we get this on Firefox 40 and users run it on
> Windows 8 before updating to Windows 10, their "default browser" experience
> will be a lot better? (note that we only have a few more days to get the
> uplift requests in and landed on beta before the final betas will be built)

Yes, I confirmed using Windows 8.1 VM and Build 10320 ISO.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]:
We have done a lot of work towards default browser UI with Firefox 40, as the default browser UI is much more complicated in Windows 10.

Can you please rebase the patch for mozilla-aurora and mozilla-beta? It looks like mozilla-aurora just needs the header file moved up to the top of the file but mozilla-beta needs more manual rebasing.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: patch for aurora

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Default browser UX would be much worse on Windows 10.
[Describe test coverage new/current, TreeHerder]: Manually tested.
[Risks and why]: Low. Even if Firefox fails to set the default, it will fallback to the current method.
[String/UUID change made/needed]: none

Comment 19

[Masatoshi Kimura [:emk]]

Attachment: patch for beta

Approval Request Comment
See the above request comment for aurora.

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: patch for beta+bug 791501

Bug 791501 will conflict with this bug. If bug 791501 get a beta-approval, please use this patch instead.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8637503 [details] [diff] [review]
patch for beta

This change has been manually tested. Worst case this should fall back to the existing behaviour if there is a failure. We'll take this change in support of browser default in Windows 10 in beta7. Please verify the fix in beta as well. Beta+ Aurora+

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking 40 as this is important for the Windows 10 launch.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92caaca12e6d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e52fe64a174f

Comment 25

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting for verification in Beta from either Masatoshi or Softvision.

Comment 26

[Masatoshi Kimura [:emk]]

I verified this using the latest Firefox 40 beta and Windows 10 RTM.

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify+ flag since Masatoshi verified this in Firefox 40 Beta, and that should suffice.

Comment 28

[Nick Nguyen [:osunick]]

I need to understand this a little better, can someone answer these questions?

1. Starting with Fx40 if the user has Firefox as a default on Windows 8 or 8.1 we store the hash so that we can reapply the change when the user upgrades to 10 and they decide to set Firefox as default again, we can do it without an interstitial dialog?

2. If the user upgrades from 39 to 40 on Win8 and has Firefox as default, do we store this hash, or does the user have to reapply the default change for us to catch it?

3. If the user upgrades from 7 there is no hash to store so we fall back to the associations settings pane, right?

Comment 29

[Masatoshi Kimura [:emk]]

(In reply to Nick Nguyen [:osunick] from comment #28)
> 1. Starting with Fx40 if the user has Firefox as a default on Windows 8 or
> 8.1 we store the hash so that we can reapply the change when the user
> upgrades to 10 and they decide to set Firefox as default again, we can do it
> without an interstitial dialog?

Yes.

> 2. If the user upgrades from 39 to 40 on Win8 and has Firefox as default, do
> we store this hash, or does the user have to reapply the default change for
> us to catch it?

If the user launches Firefox 40 on Win8, the hash will be stored. Firefox will check if it is the default and store the hash if it is.

> 3. If the user upgrades from 7 there is no hash to store so we fall back to
> the associations settings pane, right?

Right.