Justin Dolske [:Dolske] Reporter
	Description • 9 years ago

In bug 1184508, we implemented the ability to restore a previously-cached default application registry key hash. This allows a user to make Firefox their default browser again, without going through the default Windows 10 default-app selection UI (which we know causes lots of user confusion, resulting in users being unable to figure out how to make Firefox the default). We only do this after prompting the user if they want to make Firefox their default browser.

Microsoft has informed us that they plan to remove this capability, such that apps will no longer be able to restore a previous user choice this way. So we should remove/disable this code, since it will no longer work. (AIUI they will do this by make writing to the relevant registry key fail.)

For potential late-landing in Firefox 44 (in RC, about to ship next week) I'd like to do an absolutely safe/minimal patch. Probably just have RestoreWin8RegistryHashes() immediately fail? This should already be handled, and fallback to the normal Windows 10 app-defaults UI. In 45+ we can do a deeper code removal/cleanup.

	Justin Dolske [:Dolske] Reporter
	Comment 2 • 9 years ago

Comment on attachment 8709653 [details] [diff] [review]
Patch for Beta44

Looks right, but we should also disable the related UITour check (from bug 1190586).

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 3 • 9 years ago

I didn't do a one-line patch for UITour as it didn't make much sense to me.

	Justin Dolske [:Dolske] Reporter
	Comment 5 • 9 years ago

Comment on attachment 8709668 [details] [diff] [review]
Patch for Beta44

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

r+ with the one change.

::: browser/components/uitour/UITour.jsm
@@ -1809,5 @@
> -          }
> -        }
> -
> -        appinfo["canSetDefaultBrowserInBackground"] =
> -          canSetDefaultBrowserInBackground;

Seems like it would be safest to leave this property set in appinfo, but hardcoded to false. That way no snippets or tour pages that might be assuming it's always present should break.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 6 • 9 years ago

Good point.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 7 • 9 years ago

Well, it's a bit more nuanced than that because OSX doesn't allow it to be set in the background by default in newer versions of the OS but it does in older versions.

	Justin Dolske [:Dolske] Reporter
	Comment 9 • 9 years ago

Comment on attachment 8709669 [details] [diff] [review]
Patch for Aurora45 and Nightly46

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

I think GetPrefString() and SetPrefString() in nsWindowsShellService [from the original patch in bug 1184508] are also unused and can be removed. Also GetHashPref() and GetUserChoiceKeyName().

(Looks like the patch also cleaned up IsAARDefault(), but those changes are fine to keep?)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 11 • 9 years ago

Comment on attachment 8709700 [details] [diff] [review]
Patch for Beta44

Approval Request Comment
[Feature/regressing bug #]: see comment #0
[User impact if declined]: setting the default browser may fail on Windows 10 for some users
[Describe test coverage new/current, TreeHerder]: changing this code to always return false will follow the code path that most users already followed.
[Risks and why]: This is the lowest risk patch that could be written to disable the feature. No outside risks are expected.
[String/UUID change made/needed]: none

	Justin Dolske [:Dolske] Reporter
	Comment 12 • 9 years ago

I also emailed release-drivers about this separately, given that Firefox 44 RC1 is already in testing.

	Justin Dolske [:Dolske] Reporter
	Comment 13 • 9 years ago

Comment on attachment 8709699 [details] [diff] [review]
Patch for Aurora45 and Nightly46

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

Comment 9 still applies to this Nightly/Aurora patch. Looks like we crossed paths with an unfortunately-similar attachment number (comment 4 created attachment 8709669 [details] [diff] [review], which I reviewed. Comment 8 created attachment 8709699 [details] [diff] [review]).

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 14 • 9 years ago

Jared, what kind of testing have you did with a local build that contains your patch? I specifically want to know whether you tried the scenario of changing your default browser on win8/win10 and whether that worked ok. As I consider taking this directly to RC, I clearly cannot take an untested patch. Thanks!

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 15 • 9 years ago

I confirmed with a local build of mozilla-beta that setting Firefox as the default browser works. I switched between Edge and Firefox multiple times (while Firefox was running and when it was not running, then opening Firefox). The "default browser" prompt appeared during start-up when it wasn't set and didn't appear when it was set.

I tested on Windows 10 build 11099.rs1_release.160109-1156 on mozilla-beta tree.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 16 • 9 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15)
> I confirmed with a local build of mozilla-beta that setting Firefox as the
> default browser works. I switched between Edge and Firefox multiple times
> (while Firefox was running and when it was not running, then opening
> Firefox). The "default browser" prompt appeared during start-up when it
> wasn't set and didn't appear when it was set.
> 
> I tested on Windows 10 build 11099.rs1_release.160109-1156 on mozilla-beta
> tree.

Thanks a lot! I really appreciate your prompt testing on this one. Will take it in Fx44-build2 today.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 17 • 9 years ago

Comment on attachment 8709700 [details] [diff] [review]
Patch for Beta44

As Dolske elaborated in detail in his email to r-d, without this fix we may find ourselves in a Fx 44 dot release position pretty soon given that Microsoft will silently reject this way of restoring the default browser. Jared has tested the local patch to ensure setting Firefox as a default browser on win8/10 works. Let's uplift to m-r.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 9 years ago

https://hg.mozilla.org/releases/mozilla-release/rev/45a3e4181f46

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 9 years ago

Tracked for Fx45 and 46 so we ensure we land a fix for this in those versions too.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 9 years ago

One more try at tracking :)

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 21 • 9 years ago

Masatoshi, can you confirm that the changes to IsAARDefault by bug 1184508 are still something that we should keep while backing out the storing and assignment of the default-app hash?

I've looked at those changes and I don't see any mention within bug 1184508 as to why we're not calling `IsAARDefault(pAAR, L"http")` and `IsAARDefault(pAAR, L"html")` within the `aCheckAllTypes` branch of `IsDefaultBrowserVista`.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 22 • 9 years ago

Further, IsDefaultBrowserVista will always return true on Windows Vista and 7 if aCheckAllTypes is false. This doesn't seem right. Prior to bug 1184508 it used to call IsAARDefaultHTTP if aCheckAllTypes was false and it only called IsAARDefaultHTML if aCheckAllTypes was true. Now there is no call in the code to check if there is an entry for Firefox in the Application Association Registry for the HTML extension. We can probably file a separate bug for this, but I'm curious to hear what you think about it.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 23 • 9 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Further, IsDefaultBrowserVista will always return true on Windows Vista and
> 7 if aCheckAllTypes is false.

Sorry, I missed a negation, this will always return true for Windows 8 and higher. Note that the Chromium source code has some comments about QueryCurrentDefault not being reliable prior to Windows 8, https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/util/shell_util.cc&q=CLSID_ApplicationAssociationRegistration&sq=package:chromium&l=996

	Masatoshi Kimura [:emk]
	Comment 25 • 9 years ago

Comment on attachment 8710761 [details] [diff] [review]
Patch for Aurora45 and Nightly46

Bug 1184508 moved IsAARDefault() calls into SaveWin8RegistryHash(). SaveWin8RegistryHash() will not only save the registry hash but also check the default. SaveWin8RegistryHash() returns a boolean value to indicate if the browser is default handler for the passed extension/protocol.

So you can't just remove SaveWin8RegistryHash(). You will have to restore IsAARDefault(pAAR, L"http") and IsAARDefault(pAAR, L"html") somewhere.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 26 • 9 years ago

Thanks for the review. This was a bit confusing, but I reduced it to these changes. I walked through the code paths for Vista/7 and Win8/10 and they appear to hit all of the necessary checks.

	Masatoshi Kimura [:emk]
	Comment 27 • 9 years ago

Comment on attachment 8711091 [details] [diff] [review]
Patch for Nightly47, Aurora46, and Beta45

LGTM, but an official peer should sign-off.

	Pulsebot
	Comment 28 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/5e12b660b8c0

	Carsten Book [:Tomcat]
	Comment 29 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5e12b660b8c0

	Sylvestre Ledru [:Sylvestre]
	Comment 31 • 8 years ago

Jared, could you fill the uplift requests to aurora & beta? Thanks

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 32 • 8 years ago

Comment on attachment 8711091 [details] [diff] [review]
Patch for Nightly47, Aurora46, and Beta45

Approval Request Comment
[Feature/regressing bug #]: removal of feature that will no longer work on windows10
[User impact if declined]: potential for errors when setting Firefox as the default browser
[Describe test coverage new/current, TreeHerder]: manual testing
[Risks and why]: low risk, this removes some extra code paths that may have been followed if we detected certain conditions. this will make all default-browser code follow the same path which actually simplifies the code
[String/UUID change made/needed]: none

	Sylvestre Ledru [:Sylvestre]
	Comment 33 • 8 years ago

Comment on attachment 8711091 [details] [diff] [review]
Patch for Nightly47, Aurora46, and Beta45

Taking it just like the release.

Should be in 45 beta 3.

	Carsten Book [:Tomcat]
	Comment 34 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/569408867299

	Carsten Book [:Tomcat]
	Comment 35 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/2cb72f2115e7

	Cornel Ionce [:noni] [Hubs QA]
	Comment 36 • 8 years ago

Verified that setting Firefox as the default browser is working properly by executing various scenarios:
*setting Fx as default browser while running/when closed; 
*ensured that the "default browser" prompt was displayed at Fx start when not default and that is not displayed when it was the default browser;
*checked that the "Firefox is not your default browser" message is correctly displayed when Fx is not the default browser; also checked that the "Firefox is currently your default browser" is shown when Firefox is default;
*switched the default browser several times between Firefox, Edge, Safari and Chrome to make sure that no issues are shown and Firefox could be set as default every time. 

Testing was performed on two different machines running Windows 10 x64 and Windows 10x86.
Tested with:
*Firefox 44.0.2, build ID: 20160210153822
*Firefox 45 beta 9, build ID: 20160223142613
*2016-02-24 Aurora 46.0a2
*2016-02-24 Nightly 47.0a2.