1357146 - Attempting to set Firefox as default browser results in an nsITimer firing every second for up to 10 minutes

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

Here's the code in question: http://searchfox.org/mozilla-central/rev/24eec01c4e43c0f029b0148900b5f8ed508f7e21/browser/components/nsBrowserGlue.js#2362-2381

Looks like this code is polling the OS to see if we're the default yet, after attempting to set as default. Looks like it'll cancel the timer if it finds that we're default, or bail out after 600 seconds (10 minutes) of polling without the default being set.

Looking at https://mzl.la/2oEyYQE, it looks like we're hitting the worst case scenario pretty often. :/

Comment 1

[(no longer active)]

Another candidate for idle dispatch.

Comment 2

[Naveed Ihsanullah [:naveed]]

Any slow down and jank in first use must be prioritized because negative users first experience may cause them to leave.

Comment 3

[Hector Zhao [:hectorz]]

(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #0)
> Here's the code in question:
> http://searchfox.org/mozilla-central/rev/
> 24eec01c4e43c0f029b0148900b5f8ed508f7e21/browser/components/nsBrowserGlue.
> js#2362-2381
> 
> Looks like this code is polling the OS to see if we're the default yet,
> after attempting to set as default. Looks like it'll cancel the timer if it
> finds that we're default, or bail out after 600 seconds (10 minutes) of
> polling without the default being set.

On Win 7 (and earlier versions which is no longer supported), we at Beijing office patched the shell service to wait for the exit of helper.exe to decide whether Fx is successfully set as default browser. Not sure if that's better than polling. Unfortunately we never figured out what to do on Win 8+.

(In reply to Hector Zhao [:hectorz] from bug 1329874 comment #4)
> 
> https://dxr.mozilla.org/addons/source/addons/647194/chrome.manifest#102
> https://dxr.mozilla.org/addons/source/addons/647194/components/
> nsShellServiceStartup.js
> https://dxr.mozilla.org/addons/source/addons/647194/components/
> nsShellService.js
> https://dxr.mozilla.org/addons/source/addons/647194/tracking/modules/
> getExitCode.js

Comment 4

[Dimi Lee [:dimi]]

Hi Ehsan,
I would like to work on this bug.
To make sure I understand this bug correctly, what we want to do is replace timer[1] with idle dispatch hence
this bug will depend on the patch in Bug 1368072. Is that correct?

Comment 5

[(no longer active)]

I think before deciding on the implementation strategy for fixing this, we should first decide what the right fix is.

This telemetry was added in bug 1191242.  BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS is set to expire never(!) <http://searchfox.org/mozilla-central/rev/972b7a5149bbb2eaab48aafa87678c33d5f2f045/toolkit/components/telemetry/Histograms.json#10059> so it seems like the intention was for this data to be collected indefinitely.  My preferred approach here is to remove this telemetry probe completely since I think it's harmful to the performance of Firefox (even if we run it off of idle dispatch -- checking whether you're the default browser can be very expensive on Windows, but I didn't add this, so it's not my call.

jaws is out of office until June 5, so Dolske, Hi!

In the 2 years since we have added this probe, how have we used the data that we have collected?  How much longer do we expect to gain value out of this data collection?  When can we stop collecting this data?  Can I advocate for this telemetry to be removed in Firefox 55 (54 even)?

Comment 6

[(no longer active)]

Oh, looks like Bryan Clark is monitoring this data long term as per bug 1191242 comment 18.

Hi Bryan,
Can you please explain how you have used this data in the past 2 years and how you expect to use it in the future?  The collection of this data is very expensive and I don't think the continued collection of it is acceptable for performance reasons.  How much longer do we need this data and for what purpose?  Can we use other data or other ways of collecting this data that doesn't involve asking the OS for the default browser?

Comment 7

[Peter Dolanjski [:pdol]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #6)
> Oh, looks like Bryan Clark is monitoring this data long term as per bug
> 1191242 comment 18.
> 
> Hi Bryan,
> Can you please explain how you have used this data in the past 2 years and
> how you expect to use it in the future?  The collection of this data is very
> expensive and I don't think the continued collection of it is acceptable for
> performance reasons.  How much longer do we need this data and for what
> purpose?  Can we use other data or other ways of collecting this data that
> doesn't involve asking the OS for the default browser?

I'll answer this as it falls under my product area now.
We are not using this probe, to my knowledge.  Sure, it might we valuable if we optimized the default browser UI to minimize the time to make the change, but in practice the number will also be affected by migrations to Win10.

Given the above described perf hit, I'm fine with disabling this probe at the earliest convenience.

Comment 8

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

I am also fine with removing this probe. We have collected enough data on this probe now and we could add something like it back if we decide to change our default-browser walkthrough again.

Comment 9

[(no longer active)]

Great!  Let's remove it then!  Thanks for the quick feedback!

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Hey Dimi, are you still interested in working on this bug?

Comment 11

[Dimi Lee [:dimi]]

(In reply to :Felipe Gomes (needinfo me!) from comment #10)
> Hey Dimi, are you still interested in working on this bug?

Sure, I'll work on it!

Comment 12

[Dimi Lee [:dimi]]

Attachment: Bug 1357146 - Remove BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS telemetry.

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

Comment 13

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

Comment on attachment 8874678 [details]
Bug 1357146 - Remove BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS telemetry.

https://reviewboard.mozilla.org/r/146034/#review149982

Thanks!

Comment 14

[Dimi Lee [:dimi]]

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe889a742136078615418dcef930b7e26e0b525

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec1ca0d0f8ec
Remove BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS telemetry. r=jaws

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/ec1ca0d0f8ec

Comment 17

[Mike Conley (:mconley) (:⚙️)]

For QA:

For testing this bug, you'll want to make sure that the various ways that Firefox can register itself as the default browser still work.

Comment 18

[Adrian Florinescu [:aflorinescu]]

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #17)
> For QA:
> 
> For testing this bug, you'll want to make sure that the various ways that
> Firefox can register itself as the default browser still work.

AFAIK, there are only two ways to register FF as default browser: preferences and command line.
Explored there areas around setting FF as default browser there are no apparent regressions on setting FF as the default browser. 

Environments used: Windows 10x64, Ubuntu 16.04, MacOSX 10.12, Windows 7 x32
Versions: 
55.0b9 20170713130618 
56.0a1 20170801100311

Mike, in regards to actually verifying that the nsITimer event is not spamming anymore on setDefaultBrowser, is that by chance covered by tests? Should I be doing anything additionally here, or is the above enough from your point of view to mark this as verified?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

I don't think it's necessary for you to check that the nsITimer event is being spammy - just that setting the default browser still works. I think what you've done is probably sufficient.

Comment 20

[Adrian Florinescu [:aflorinescu]]

Thanks Mike, updating the status accordingly.