Closed Bug 1357146 Opened 3 years ago Closed 2 years ago

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

Categories

(Firefox :: General, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Status
firefox55 --- verified
firefox56 --- verified

People

(Reporter: mconley, Assigned: dimi)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p1][photon-performance][qa-commented])

Attachments

(1 file)

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. :/
Flags: qe-verify?
Priority: -- → P2
Another candidate for idle dispatch.
Depends on: 1355746
Any slow down and jank in first use must be prioritized because negative users first experience may cause them to leave.
Whiteboard: [qf][photon-performance] → [qf:p1][photon-performance]
Depends on: 1358476
No longer depends on: 1355746
(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
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
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?
Flags: needinfo?(ehsan)
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)?
Blocks: 1191242
Flags: needinfo?(ehsan) → needinfo?(dolske)
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?
Flags: needinfo?(clarkbw)
(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.
Flags: needinfo?(clarkbw)
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.
Flags: needinfo?(dolske)
Great!  Let's remove it then!  Thanks for the quick feedback!
Hey Dimi, are you still interested in working on this bug?
Flags: needinfo?(dlee)
(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!
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Flags: needinfo?(dlee)
Priority: P2 → P1
Comment on attachment 8874678 [details]
Bug 1357146 - Remove BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS telemetry.

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

Thanks!
Attachment #8874678 - Flags: review?(jaws) → review+
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec1ca0d0f8ec
Remove BROWSER_SET_DEFAULT_TIME_TO_COMPLETION_SECONDS telemetry. r=jaws
https://hg.mozilla.org/mozilla-central/rev/ec1ca0d0f8ec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.7 - Jun 12
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.
Whiteboard: [qf:p1][photon-performance] → [qf:p1][photon-performance][qa-commented]
(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?
Flags: needinfo?(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.
Flags: needinfo?(mconley)
Thanks Mike, updating the status accordingly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.