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

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

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.

Comment 1

[:Gijs (he/him)]

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 2

[:Gijs (he/him)]

Attachment: Bug 1319816 - fix default browser determination on Windows 7 and below in the automigration case,

As noted on the bug, because we call getMigratorKeyForDefaultBrowser() multiple times,
its value no longer reflects the (deleted) registry key for subsequent calls.

While we can fix this for the automigrate case by just passing the default we determined a few
lines earlier (and that seems worth doing to avoid busywork), there are 2 small problems
with this:

1) if the default browser has no data, `migratorKey` won't be set, and so we'll call the same
method again anyway, and the message reported in the error console will be that we can't
migrate from Firefox, when the real problem is that we can't migrate from the original
default browser.

2) there are other callers besides AutoMigrate. Specifically, migration.js also calls this
method.

To deal with these, I've fixed getMigratorKeyForDefaultBrowser() to return the same
registry-based value for its lifetime if we hit the 'the default is firefox, go look for an
earlier default' case.

I've verified that either the s/aMigratorKey/migratorKey/ or the change to
getMigratorKeyForDefaultBrowser() are sufficient to make this work properly in the
automigration case.

While I was here, I also updated one of the error messages to be more explicit.

Review commit: https://reviewboard.mozilla.org/r/95112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95112/

Comment 3

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

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4153ce5276d5
fix default browser determination on Windows 7 and below in the automigration case, r=jaws

Comment 5

[:Gijs (he/him)]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4153ce5276d5

Comment 7

[Gerry Chang [:gchang]]

Hi Brindusa,
Can you help find someone to verify if this is fixed on latest nightly?

Comment 8

[:Gijs (he/him)]

Here's a trypush with automigration enabled on nightly to be able to test this:

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

Comment 9

[Simona Badau, Desktop QA]

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?

Comment 10

[:Gijs (he/him)]

(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...

Comment 11

[Julien Cristau [:jcristau]]

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

Comment 12

[Gerry Chang [:gchang]]

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.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/786488b0735e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1b954c82dd04

Comment 15

[Simona Badau, Desktop QA]

(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?

Comment 16

[:Gijs (he/him)]

(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.

Comment 17

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a3902bfdf15e1a237e6ae443b40a29c5be9f675

Comment 18

[Simona Badau, Desktop QA]

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.