1279240 - Save previous default browser in the registry when making ourselves the default browser in the installer in windows xp/vista/7, and use data for automigration

Status: VERIFIED FIXED

(Firefox :: Migration, defect, P2)

Description

[:Gijs (he/him)]

On Windows XP/Vista/7, the installer can make us the default browser.

Over in bug 1271775, we'd like to automatically migrate data for the initial Firefox profile on a machine from the default browser.

If the installer makes us the default browser, by the time we run startup *we* are the default browser, and migrating from ourselves obviously doesn't work.

In order to work around this limitation, we should save the previous default browser value into a registry key somehow, in the installer, and then read that registry key from the automigration code.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1279240 - save old default browser when setting as default on win7 and below from the installer,

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

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1279240 - move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin,

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

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1279240 - use registry key to deduce default browser when possible,

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

Comment 4

[:Gijs (he/him)]

One issue I have with the installer patch is that the stub installer runs the setAsDefault code from a helper app, and it does not wait for that helper app to complete before starting Firefox. I noticed this when I added some MessageBox debugging in the routine when trying to debug my patch - while the alert is up (and the code in the helper thus not continuing further) Firefox came up already.

That means there's no guarantee that the registry setting saving has happened by the time Firefox starts, which means it won't be present when the migration code runs. That's a problem for this bug. Rob, is there a way to avoid that and/or would it be bad if we waited for the helper app to finish running?

I think this might also be interesting for other reasons, because it means, AIUI, that it is in principle possible for the user to say "set Firefox as default" in the installer and then be prompted on startup (unless we've now shipped the "don't prompt the user on the initial startup" fix to release? I forget what's happened there, there's been so much discussion...). That experience would be... not good.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8794257 [details]
Bug 1279240 - move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin,

https://reviewboard.mozilla.org/r/80758/#review79464

Looks good (there are tests!), just minor formatting details.

::: xpcom/io/nsLocalFileWin.cpp:1196
(Diff revision 1)
>    return NS_OK;
>  
>  }
>  
> +// Strip a handler command string of its quotes and parameters.
> +static void CleanupHandlerPath(nsString& aPath)

Nit: format like so:

```
static void
CleanupHandlerPath(...)
```

::: xpcom/io/nsLocalFileWin.cpp:1225
(Diff revision 1)
> +  aPath.Trim(" ", true, true);
> +}
> +
> +// Strip the windows host process bootstrap executable rundll32.exe
> +// from a handler's command string if it exists.
> +static void StripRundll32(nsString& aCommandString)

Likewise here.

::: xpcom/io/nsLocalFileWin.cpp:1254
(Diff revision 1)
> +
> +// Returns the fully qualified path to an application handler based on
> +// a parameterized command string. Note this routine should not be used
> +// to launch the associated application as it strips parameters and
> +// rundll.exe from the string. Designed for retrieving display information
> +// on a particular handler.   

Nit: trailing whitespace

::: xpcom/io/nsLocalFileWin.cpp:1255
(Diff revision 1)
> +// Returns the fully qualified path to an application handler based on
> +// a parameterized command string. Note this routine should not be used
> +// to launch the associated application as it strips parameters and
> +// rundll.exe from the string. Designed for retrieving display information
> +// on a particular handler.   
> +/* static */ bool nsLocalFile::CleanupCmdHandlerPath(nsAString& aCommandHandler)

Nit: format like so:

```
/* static */ bool
nsLocalFile::CleanupCmdHandlerPath(...)
```

::: xpcom/tests/unit/test_windows_cmdline_file.js:15
(Diff revision 1)
> +      let f = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFileWin);
> +      f.initWithCommandline(cmdline);
> +      is(f.path, executableFile.path, "Should be able to recover executable path");
> +    }
> +  }
> +  

First off: tests?  For cutting-and-pasting-code patches?  +1 to you.

Nit: trailing whitespace.

Comment 6

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

Comment on attachment 8794258 [details]
Bug 1279240 - use registry key to deduce default browser when possible,

https://reviewboard.mozilla.org/r/80760/#review79472

::: browser/components/migration/MigrationUtils.jsm:620
(Diff revision 1)
>      }
>      catch (ex) {
>        Cu.reportError("Could not detect default browser: " + ex);
>      }
> -    return "";
> +
> +    // "Firefox" is the least useful entry here, and might just be because we've set

"firefox" to match the actual key value

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to :Gijs Kruitbosch from comment #4)
> One issue I have with the installer patch is that the stub installer runs
> the setAsDefault code from a helper app, and it does not wait for that
> helper app to complete before starting Firefox. I noticed this when I added
> some MessageBox debugging in the routine when trying to debug my patch -
> while the alert is up (and the code in the helper thus not continuing
> further) Firefox came up already.
> 
> That means there's no guarantee that the registry setting saving has
> happened by the time Firefox starts, which means it won't be present when
> the migration code runs. That's a problem for this bug. Rob, is there a way
> to avoid that and/or would it be bad if we waited for the helper app to
> finish running?
> 
> I think this might also be interesting for other reasons, because it means,
> AIUI, that it is in principle possible for the user to say "set Firefox as
> default" in the installer and then be prompted on startup (unless we've now
> shipped the "don't prompt the user on the initial startup" fix to release? I
> forget what's happened there, there's been so much discussion...). That
> experience would be... not good.
It doesn't happen in practice and we want Firefox to launch as soon as possible.

For your use case you can set the value in the stub before launching the helper. Just create a macro in shared.nsh to do this and call it from both the stub and the installer before the calls to set as default.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 8794256 [details]
Bug 1279240 - save old default browser when setting as default on win7 and below from the installer,

https://reviewboard.mozilla.org/r/80756/#review79508

For your use case you can set the value in the stub before launching the helper. Just create a macro in shared.nsh to do this and call it from both the stub and the installer before the calls to set as default. I'll r+ it with that.

Comment 9

[:Gijs (he/him)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> Comment on attachment 8794256 [details]
> Bug 1279240 - save old default browser when setting as default on win7 and
> below from the installer,
> 
> https://reviewboard.mozilla.org/r/80756/#review79508
> 
> For your use case you can set the value in the stub before launching the
> helper. Just create a macro in shared.nsh to do this and call it from both
> the stub and the installer before the calls to set as default. I'll r+ it
> with that.

The stub doesn't include shared.nsh, as far as I can tell. Am I missing something? Should I just duplicate the code, or stick it in a different shared file, or include shared.nsh?

Comment 10

[Robert Strong (they/them - no direct email)]

Sorry about that, just duplicate the code. At some point the stub and the full installer will be merged and that code can merge at that time.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8794256 [details]
Bug 1279240 - save old default browser when setting as default on win7 and below from the installer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80756/diff/1-2/

Comment 12

[:Gijs (he/him)]

Comment on attachment 8794257 [details]
Bug 1279240 - move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80758/diff/1-2/

Comment 13

[:Gijs (he/him)]

Comment on attachment 8794258 [details]
Bug 1279240 - use registry key to deduce default browser when possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80760/diff/1-2/

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 8794256 [details]
Bug 1279240 - save old default browser when setting as default on win7 and below from the installer,

https://reviewboard.mozilla.org/r/80756/#review80712

::: browser/installer/windows/nsis/installer.nsi:599
(Diff revision 2)
>    SetDetailsPrint none
>  
>    ${Unless} ${Silent}
>      ${MUI_INSTALLOPTIONS_READ} $0 "summary.ini" "Field 4" "State"
>      ${If} "$0" == "1"
> +      ${BackupDefaultBrowser}

Just inline the code

::: browser/installer/windows/nsis/installer.nsi:611
(Diff revision 2)
>        ${Else}
>          GetFunctionAddress $0 SetAsDefaultAppUserHKCU
>          UAC::ExecCodeSegment $0
>        ${EndIf}
>      ${Else}
> +      ClearErrors

Place the ClearErrors immediately before the attempt to write to the registry

::: browser/installer/windows/nsis/shared.nsh:1261
(Diff revision 2)
>  !macroend
>  !define IsFirewallSvcRunning "!insertmacro IsFirewallSvcRunning"
>  !define un.IsFirewallSvcRunning "!insertmacro IsFirewallSvcRunning"
>  
> +; NB: duplicated in stub.nsi. Please keep both copies in sync.
> +!macro BackupDefaultBrowser

Just inline this code

::: browser/installer/windows/nsis/stub.nsi:270
(Diff revision 2)
>  !insertmacro SetBrandNameVars
>  !insertmacro ITBL3Create
>  !insertmacro UnloadUAC
>  
> +; NB: duplicated in shared.nsh. Please keep both copies in sync.
> +!macro BackupDefaultBrowser

Just inline this code

::: browser/installer/windows/nsis/stub.nsi:1782
(Diff revision 2)
>  
>    StrCpy $ProgressCompleted "$ProgressTotal"
>    Call SetProgressBars
>  
>    ${If} "$CheckboxSetAsDefault" == "1"
> +    ${BackupDefaultBrowser}

Just inline this code

Comment 15

[:Gijs (he/him)]

Comment on attachment 8794256 [details]
Bug 1279240 - save old default browser when setting as default on win7 and below from the installer,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80756/diff/2-3/

Comment 16

[:Gijs (he/him)]

Comment on attachment 8794257 [details]
Bug 1279240 - move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80758/diff/2-3/

Comment 17

[:Gijs (he/him)]

Comment on attachment 8794258 [details]
Bug 1279240 - use registry key to deduce default browser when possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80760/diff/2-3/

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 8794256 [details]
Bug 1279240 - save old default browser when setting as default on win7 and below from the installer,

https://reviewboard.mozilla.org/r/80756/#review80724

Looks good and thanks!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc1b725b47d4
save old default browser when setting as default on win7 and below from the installer, r=rstrong
https://hg.mozilla.org/integration/autoland/rev/d0d5180dc062
move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4b69e32e3a83
use registry key to deduce default browser when possible, r=jaws

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it appears to have caused a leak on some Windows VM mochitests like https://treeherder.mozilla.org/logviewer.html#?job_id=4262387&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/016bf0339326

Comment 21

[:Gijs (he/him)]

Comment on attachment 8794257 [details]
Bug 1279240 - move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80758/diff/3-4/

Comment 22

[:Gijs (he/him)]

Comment on attachment 8794258 [details]
Bug 1279240 - use registry key to deduce default browser when possible,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80760/diff/3-4/

Comment 23

[:Gijs (he/him)]

Discussed with Nathan on IRC, found the issue, he suggested a fix, tested and verified locally. It doesn't look like there's other failures on there that are related to the commit, so relanding.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29ae0ea36daf
save old default browser when setting as default on win7 and below from the installer, r=rstrong
https://hg.mozilla.org/integration/autoland/rev/cc17d0140e11
move path parsing of commandline handlers for mimetypes/protocols to nsILocalFileWin, r=froydnj
https://hg.mozilla.org/integration/autoland/rev/2fe60d27c780
use registry key to deduce default browser when possible, r=jaws

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/29ae0ea36daf
https://hg.mozilla.org/mozilla-central/rev/cc17d0140e11
https://hg.mozilla.org/mozilla-central/rev/2fe60d27c780

Comment 26

[:Gijs (he/him)]

Comment on attachment 8794258 [details]
Bug 1279240 - use registry key to deduce default browser when possible,

Approval Request Comment
[Feature/regressing bug #]: allowing us to detect the previous default browser on Windows 7 and below if the installer set the default browser to Firefox
[User impact if declined]: we can't automatically migrate data from other browsers on Windows 7 and below.
[Describe test coverage new/current, TreeHerder]: There is test coverage for the 2nd cset that moved code around between two components, some of which is added in the patch itself, some of which is pre-existing. The browser/ change is very small. There are unfortunately no automated tests for the behaviour of the installer that I am aware of.
[Risks and why]: low risk. All this does is save a particular default value to the registry, and then read that value from the browser code when we first start up. I'm reasonably confident that the code in question is sane, though I've also asked QE to verify that this works as-is.
[String/UUID change made/needed]: no.

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8794258 [details]
Bug 1279240 - use registry key to deduce default browser when possible,

This patch fixes migrating data when setting default browser to Firefox on Windows 7 and below. Although the patch seems a lot to me, this affects users a lot. I want to take this in aurora as early as possible. If anything wrong, we still have time to fix it. In the meanwhile, I still need QE's help to verify.

Hi Florin,
Can we assign a QA to help verify this issue as early as possible?

Comment 28

[Carsten Book [:Tomcat]]

so is this just for this one patch or for all 3 ?

Comment 29

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

(In reply to Gerry Chang [:gchang] from comment #27)
> Hi Florin,
> Can we assign a QA to help verify this issue as early as possible?

Added to our team's list for verification. Set "qe-verify+" for tracking. Keeping needinfo until we have some test results here.

Comment 30

[:Gijs (he/him)]

(In reply to Carsten Book [:Tomcat] from comment #28)
> so is this just for this one patch or for all 3 ?

All 3.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/935a54e00cc5
https://hg.mozilla.org/releases/mozilla-aurora/rev/8b275c9c39bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/77ddc3010bce

Comment 32

[Bogdan Maris, Desktop Test Engineering]

Hi Gijs, I want to verify this but I'm a bit stuck so I need some additional information.

I noticed that the pref for automigrate feature is set to false (browser.migrate.automigrate.enabled;false) by default in all builds although the UI is enabled so I could not see the actual automigration work. I'm not quite sure how this feature works and when does the migration triggers but based on the comments from this bug I understood that it starts when running the installation wizard.

I was thinking of using a user.js file to force the above pref to be enabled hoping that I will see the automigrate UI but that means I have to start Nightly with an already created profile and from what I know I can't use a specific pref and start the Import wizard (maybe I'm mistaken).

Again, maybe all of this is not necessary to verify this bug, only that we save the data for the already 'Default browser' in registry. If so where I can see this?

Comment 33

[:Gijs (he/him)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #32)
> Hi Gijs, I want to verify this but I'm a bit stuck so I need some additional
> information.
> 
> I noticed that the pref for automigrate feature is set to false
> (browser.migrate.automigrate.enabled;false) by default in all builds
> although the UI is enabled so I could not see the actual automigration work.
> I'm not quite sure how this feature works and when does the migration
> triggers but based on the comments from this bug I understood that it starts
> when running the installation wizard.
> 
> I was thinking of using a user.js file to force the above pref to be enabled
> hoping that I will see the automigrate UI but that means I have to start
> Nightly with an already created profile and from what I know I can't use a
> specific pref and start the Import wizard (maybe I'm mistaken).

You can force it with the -migration commandline flag (which makes it easier to test startup migration things, so that might be useful in future), but I am not sure if a user.js modification to the prefs will be picked up for this particular pref (ie browser.migrate.automigrate.enabled) - it might be too early in startup.

> Again, maybe all of this is not necessary to verify this bug, only that we
> save the data for the already 'Default browser' in registry. If so where I
> can see this?

Indeed it should not be necessary to verify this bug. You can check the data (which will have the form of a full commandline handler path) in the registry at HKCU\Software\Mozilla\Firefox, for the key OldDefaultBrowserCommand .

If I'm remembering correctly, the default browser will also be preselected in the migration dialog (if we can't detect the default browser we'll preselect the top value in the list of radio buttons). So at least for Chrome (which is never the top value), if you:

1) make chrome the default
2) install firefox
3) open the initial migration dialog

the difference before/after for this bug should be that chrome wasn't selected before this patch, and is selected afterwards.

Note also that Firefox's initial migration startup will remove the registry key in question, so you'd have to check it before starting Firefox.

Does that help?

Comment 34

[Bogdan Maris, Desktop Test Engineering]

Thanks for the explanation!

Here is what I'm seeing using Latest Nightly 52.0a1 on Windows 7 32bit, Windows Vista 64bit and Windows XP 32bit:

- Selecting "Use Nightly as my default web browser" during installation)
 - Before opening Firefox (after installation), OldDefaultBrowserCommand key is available in HKEY_CURRENT_USER\Software\Mozilla\Firefox\OldDefaultBrowserCommand with value "C:\Program Files\Google\Chrome\Application\chrome.exe" -- "%1"
 - After opening Firefox for the first time the key is still there in regedit with same value but on Import Wizard, Internet Explorer is still the default selected option
Note that:
(Control Panel\Internet Options\Programs does show that IE is not the default browser)

- If "Use Nightly as my default web browser" is not selected, the Key does not appear in regedit and Chrome is the default selected value in Import Wizard.

The results from above are identical to all Windows platforms I tested.

Is it possible that I did something wrong?

Comment 35

[:Gijs (he/him)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #34)
> Thanks for the explanation!
> 
> Here is what I'm seeing using Latest Nightly 52.0a1 on Windows 7 32bit,
> Windows Vista 64bit and Windows XP 32bit:
> 
> - Selecting "Use Nightly as my default web browser" during installation)
>  - Before opening Firefox (after installation), OldDefaultBrowserCommand key
> is available in
> HKEY_CURRENT_USER\Software\Mozilla\Firefox\OldDefaultBrowserCommand with
> value "C:\Program Files\Google\Chrome\Application\chrome.exe" -- "%1"
>  - After opening Firefox for the first time the key is still there in
> regedit with same value but on Import Wizard, Internet Explorer is still the
> default selected option
> Note that:
> (Control Panel\Internet Options\Programs does show that IE is not the
> default browser)
> 
> - If "Use Nightly as my default web browser" is not selected, the Key does
> not appear in regedit and Chrome is the default selected value in Import
> Wizard.
> 
> The results from above are identical to all Windows platforms I tested.
> 
> Is it possible that I did something wrong?

Yes, don't start the migration from within Firefox afterwards. Either ensure that the installer's start-up of Firefox opens the import wizard (by deleting/moving existing Firefox profiles), or avoid having the installer start Firefox (only possible with the full installer, I think, not the stub) and start it manually with the -migration startup option as I suggested in comment #33.

Comment 36

[:Gijs (he/him)]

(We only pre-select the default browser in the import wizard if we're using startup migration.)

Comment 37

[Bogdan Maris, Desktop Test Engineering]

(In reply to :Gijs Kruitbosch from comment #35)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #34)
> > Thanks for the explanation!
> > 
> > Here is what I'm seeing using Latest Nightly 52.0a1 on Windows 7 32bit,
> > Windows Vista 64bit and Windows XP 32bit:
> > 
> > - Selecting "Use Nightly as my default web browser" during installation)
> >  - Before opening Firefox (after installation), OldDefaultBrowserCommand key
> > is available in
> > HKEY_CURRENT_USER\Software\Mozilla\Firefox\OldDefaultBrowserCommand with
> > value "C:\Program Files\Google\Chrome\Application\chrome.exe" -- "%1"
> >  - After opening Firefox for the first time the key is still there in
> > regedit with same value but on Import Wizard, Internet Explorer is still the
> > default selected option
> > Note that:
> > (Control Panel\Internet Options\Programs does show that IE is not the
> > default browser)
> > 
> > - If "Use Nightly as my default web browser" is not selected, the Key does
> > not appear in regedit and Chrome is the default selected value in Import
> > Wizard.
> > 
> > The results from above are identical to all Windows platforms I tested.
> > 
> > Is it possible that I did something wrong?
> 
> Yes, don't start the migration from within Firefox afterwards. Either ensure
> that the installer's start-up of Firefox opens the import wizard (by
> deleting/moving existing Firefox profiles), or avoid having the installer
> start Firefox (only possible with the full installer, I think, not the stub)
> and start it manually with the -migration startup option as I suggested in
> comment #33.

I actually did *not* start the migration from within Firefox (Library/Import and Backup/Import Data...), It automatically started when I installed Firefox (after deleting all the profiles). I also tried this morning with '-migration' command and the outcome is the same as letting the migration wizard start during the first install.

Comment 38

[:Gijs (he/him)]

OK, then it sounds like it must not be working right. Can you file a followup with more details, like if there are errors in the browser console afterwards, and if evaluating:

Cc["@mozilla.org/uriloader/external-protocol-service;1"].
        getService(Ci.nsIExternalProtocolService).getApplicationDescription("http");

in the browser console afterwards produces "Firefox" or something else ? Does:

 WindowsRegistry.readRegKey(
          Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, "Software\\Mozilla\\Firefox", "OldDefaultBrowserCommand");

produce the value from the registry you quoted in comment #37 ?

Comment 39

[:Gijs (he/him)]

Talking to Bogdan on IRC, the problem is that nightly- and devedition-branded builds aren't triggering the existing detection for "Firefox" as a browser.

To avoid this problem, I'm pushing trunk and aurora to try with official branding so that this won't happen.

Trunk: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf5f777968834be0147fcb2c62c6dec637d59f75
Aurora: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=92b5eb53550ab2cc87f6e2a68b96e2af11a33edc

I've filed bug 1314237 to track detecting nightly/aurora better.

Comment 40

[Bogdan Maris, Desktop Test Engineering]

I just finished testing using the try builds from comment 39 and I can report that the reg key "OldDefaultBrowserCommand" appears and deletes itself as it should, before starting and after starting Firefox. Also Chrome will be the default selected option when Import Wizard starts. Based on the above I'll mark this as verified fixed.
I tested on Windows 7 64-bit/32bit, Windows Vista 64bit and Windows XP 32-bit.