Automigrate still can't determine the default browser correctly on Windows 7 and below

VERIFIED FIXED in Firefox 51

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified, firefox53 verified)

Details

Attachments

(1 attachment)

The default browser detection fixes in bug 1279240 were verified by QE based on the changes to manual importing. However, they don't work in automated migration scenarios. This is evident from the large number of win7 and below machines that bail out of automigration early which is unchanged between the 50 and 51 experiments.

It turns out the cause is that we remove the registry key when we encounter it, and that the getMigratorKeyForDefaultBrowser method is invoked once before AutoMigrate has a chance to call it. Thus, by the time AutoMigrate has been called, the registry key is gone and it no longer finds anything else besides "Firefox".

We should probably persist the correct value inside MigrationUtils, or remove the key later on during the startup/migration process.
Specifically, the XPCOM startup code calls into here:

https://dxr.mozilla.org/mozilla-central/rev/0ddfec7126ec503b54df9c4b7c3b988906f6c882/browser/components/migration/MigrationUtils.jsm#862

aMigratorKey is never passed except when using Firefox Refresh.

This means we hit this else clause:

https://dxr.mozilla.org/mozilla-central/rev/0ddfec7126ec503b54df9c4b7c3b988906f6c882/browser/components/migration/MigrationUtils.jsm#870,882-883

which determines the default browser. If there is data available, we then assign that to the local `migratorKey` variable, but then this code:

https://dxr.mozilla.org/mozilla-central/rev/0ddfec7126ec503b54df9c4b7c3b988906f6c882/browser/components/migration/MigrationUtils.jsm#906-914

calls into AutoMigrate with the original aMigratorKey, instead of the migratorKey which we might have already determined earlier.

We should be using `migratorKey` instead, which will prevent this problem.
Comment on attachment 8813722 [details]
Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case,

https://reviewboard.mozilla.org/r/95112/#review95342
Attachment #8813722 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4153ce5276d5
fix default browser determination on Windows 7 and below in the automigration case, r=jaws
Comment on attachment 8813722 [details]
Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case,

Approval Request Comment
[Feature/regressing bug #]: bug 1279240
[User impact if declined]: on Windows XP, Vista and Windows 7, we aren't able to auto-import data from another browser in the vast majority of cases (falling back to the manual import)
[Describe test coverage new/current, TreeHerder]: there's circumstantial test coverage (that would fail if I broke something catastrophically), but no test coverage specifically for this functionality working perfectly. However, there is telemetry that indicates that this is broken in the wild, and that telemetry will also help indicate if it's properly fixed once this lands.
[Risks and why]: low, this is a relatively small correctness fix that is well-understood. It's also still early in the beta cycle, and we have telemetry to help us assess whether it's effective.
[String/UUID change made/needed]: no
Attachment #8813722 - Flags: approval-mozilla-beta?
Attachment #8813722 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4153ce5276d5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Hi Brindusa,
Can you help find someone to verify if this is fixed on latest nightly?
Flags: needinfo?(brindusa.tot)
Here's a trypush with automigration enabled on nightly to be able to test this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd70d02e1deb7a668fb2b22cbd41dc22dbd564be
Mozilla/5.0 (Windows NT 6.1; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161125042139

Verified that when using the latest Nightly 53.0a1 on Windows 7 - the default browser (Chrome/IE) is properly detected and data is automatically migrated. I used a Nightly 53 build that was downloaded from Treeherder and had the preference "browser.migrate.automigrate.enabled" set to true. 

Please note that the fix could not be verified on the older OS'es than Windows 7 because the latest Nightly 53 is no longer supported - "Sorry Nightly can't be installed. This version of Nightly requires Microsoft Windows 7 or newer".

Gijs, I noticed that on Firefox 50 release, Safari is also listed in the Import wizard dialog but on the latest Nightly 53 it's not. In this situation I receive the Import wizard dialog with the option to migrate from IE/Chrome/Don't import anything. Is Safari intentionally removed from the import wizard dialog list of browsers?
Flags: needinfo?(brindusa.tot) → needinfo?(gijskruitbosch+bugs)
(In reply to Simona B [:simonab ] from comment #9)
> Mozilla/5.0 (Windows NT 6.1; rv:53.0) Gecko/20100101 Firefox/53.0
> Build ID: 20161125042139
> 
> Verified that when using the latest Nightly 53.0a1 on Windows 7 - the
> default browser (Chrome/IE) is properly detected and data is automatically
> migrated. I used a Nightly 53 build that was downloaded from Treeherder and
> had the preference "browser.migrate.automigrate.enabled" set to true. 
> 
> Please note that the fix could not be verified on the older OS'es than
> Windows 7 because the latest Nightly 53 is no longer supported - "Sorry
> Nightly can't be installed. This version of Nightly requires Microsoft
> Windows 7 or newer".

Oh, right. I guess we'll have to wait for uplift approval to verify on pre-win7 OSes, then... I'd missed that we were dropping vista as well. :-)

> Gijs, I noticed that on Firefox 50 release, Safari is also listed in the
> Import wizard dialog but on the latest Nightly 53 it's not. In this
> situation I receive the Import wizard dialog with the option to migrate from
> IE/Chrome/Don't import anything. Is Safari intentionally removed from the
> import wizard dialog list of browsers?

This is on Windows, and we removed support for Safari on Windows a while ago: bug 1276701. So yes, that sounds normal.

To be clear, did you manually open the migration dialog on the builds from comment 9? When we (can) automatically migrate data (so, when you install Firefox while the default browser is Chrome/IE), we shouldn't be opening the import dialog...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(simona.marcu)
Comment on attachment 8813722 [details]
Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case,

let's take this in aurora so it can be tested, since nightly dropped winxp/vista
Attachment #8813722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8813722 [details]
Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case,

Fix an automigration issue and was verified. Beta51+. Should be in 51 beta 4.
Attachment #8813722 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to :Gijs Kruitbosch from comment #10)
> This is on Windows, and we removed support for Safari on Windows a while
> ago: bug 1276701. So yes, that sounds normal.
> 
> To be clear, did you manually open the migration dialog on the builds from
> comment 9? When we (can) automatically migrate data (so, when you install
> Firefox while the default browser is Chrome/IE), we shouldn't be opening the
> import dialog...

No, I didn't open the migration dialog (I used a Nightly build provided in Comment 8 where automigration is enabled by default and the migration dialog is not opened). I also checked the migration dialog from a Nightly build where the pref browser.migrate.automigrate.enabled is set to false (downloaded it from http://archive.mozilla.org/).

Gijs, could you please also help me with the trypushes with automigration enabled on aurora and beta?
Flags: needinfo?(simona.marcu) → needinfo?(gijskruitbosch+bugs)
(In reply to Simona B [:simonab ] from comment #15)
> (In reply to :Gijs Kruitbosch from comment #10)
> > This is on Windows, and we removed support for Safari on Windows a while
> > ago: bug 1276701. So yes, that sounds normal.
> > 
> > To be clear, did you manually open the migration dialog on the builds from
> > comment 9? When we (can) automatically migrate data (so, when you install
> > Firefox while the default browser is Chrome/IE), we shouldn't be opening the
> > import dialog...
> 
> No, I didn't open the migration dialog (I used a Nightly build provided in
> Comment 8 where automigration is enabled by default and the migration dialog
> is not opened). I also checked the migration dialog from a Nightly build
> where the pref browser.migrate.automigrate.enabled is set to false
> (downloaded it from http://archive.mozilla.org/).
> 
> Gijs, could you please also help me with the trypushes with automigration
> enabled on aurora and beta?

automigration is enabled by default on beta right now, so using the beta 4 build (rather than a 'normal' treeherder build off mozilla-beta) should be enough. I'll get a trybuild for aurora going in a second.
Thanks Gijs for all your help and guidance. 

Verified as fixed using the latest Firefox 51.0b4 (Build ID: 20161128075558)and Firefox Developer Edition 53.0a2 (Build ID: 20161129031443) on Windows XP, Windows Vista and Windows 7.

Based on the above and on Comments 9 and Comment 10, setting the status of this issue to Verified.
You need to log in before you can comment on or make changes to this bug.