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

VERIFIED FIXED in Firefox 51

Status

()

defect
P2
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug)

45 Branch
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified)

Details

Attachments

(3 attachments)

Assignee

Description

3 years ago
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.
Assignee

Updated

3 years ago
Blocks: 1271800
Assignee

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Comment 4

3 years ago
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.
Flags: needinfo?(robert.strong.bugs)

Comment 5

3 years ago
mozreview-review
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.
Attachment #8794257 - Flags: review?(nfroyd) → review+
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
Attachment #8794258 - Flags: review?(jaws) → review+
(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.
Flags: needinfo?(robert.strong.bugs)
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.
Attachment #8794256 - Flags: review?(robert.strong.bugs) → review-
Assignee

Comment 9

3 years ago
(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?
Flags: needinfo?(robert.strong.bugs)
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.
Flags: needinfo?(robert.strong.bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8794256 - Flags: review?(mhowell)
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
Attachment #8794256 - Flags: review?(robert.strong.bugs) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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!
Attachment #8794256 - Flags: review?(robert.strong.bugs) → review+

Comment 19

3 years ago
Pushed by gijskruitbosch@gmail.com:
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
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
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 23

3 years ago
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.
Assignee

Updated

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 24

3 years ago
Pushed by gijskruitbosch@gmail.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29ae0ea36daf
https://hg.mozilla.org/mozilla-central/rev/cc17d0140e11
https://hg.mozilla.org/mozilla-central/rev/2fe60d27c780
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Assignee

Updated

3 years ago
Blocks: 1309613
Assignee

Updated

3 years ago
Flags: qe-verify?
Assignee

Comment 26

3 years ago
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.
Attachment #8794258 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(florin.mezei)
Attachment #8794258 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
so is this just for this one patch or for all 3 ?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: qe-verify? → qe-verify+
Assignee

Comment 30

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

All 3.
Flags: needinfo?(gijskruitbosch+bugs)
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?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 33

3 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bogdan.maris)
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?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(florin.mezei)
Flags: needinfo?(bogdan.maris)
Assignee

Comment 35

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bogdan.maris)
Assignee

Comment 36

3 years ago
(We only pre-select the default browser in the import wizard if we're using startup migration.)
(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.
Flags: needinfo?(bogdan.maris) → needinfo?(gijskruitbosch+bugs)
Assignee

Comment 38

3 years ago
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 ?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bogdan.maris)
Assignee

Comment 39

3 years ago
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(bogdan.maris)
Assignee

Updated

3 years ago
Depends on: 1319816
You need to log in before you can comment on or make changes to this bug.