If about:preferences is open on Windows, it will poll the OS to see if the default browser has changed every second

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: perry)

Tracking

unspecified
Firefox 56
Unspecified
Windows
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [reserve-photon-performance][qf:p2])

Attachments

(2 attachments, 3 obsolete attachments)

It'd probably be better if the OS just told us if things changed. If that's not possible, then we should probably poll less frequently, and only if the Main pane is focused. If about:preferences goes into the background, or we switch panes, we should probably stop the interval.
Flags: qe-verify?

Comment 1

2 years ago
Another candidate for idle dispatch.
Depends on: 1355746
Whiteboard: [photon-performance][qf] → [photon-performance][qf:p1]
Depends on: 1358476
No longer depends on: 1355746
(Reporter)

Comment 2

2 years ago
Here's a rather depressing profile where my super-powerful Win 10 machine (not the reference hardware) hung in the main thread in the parent process for _5 seconds_ because we were waiting for Windows to get back to us about whether or not Firefox is the default browser. And this was with about:preferences in a background tab.

https://perfht.ml/2qoa60r

Comment 3

2 years ago
This is *completely expected*.  Any synchronous IO operation must be expected to not have any bounded execution time.
Priority: P2 → P3
Whiteboard: [photon-performance][qf:p1] → [reserve-photon-performance][qf:p1]
Whiteboard: [reserve-photon-performance][qf:p1] → [reserve-photon-performance][qf:p2]

Updated

2 years ago
No longer blocks: qf-bugs-upforgrabs
Flags: qe-verify? → qe-verify-
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
(In reply to Mike Conley (:mconley) from comment #0)
> It'd probably be better if the OS just told us if things changed. If that's
> not possible, ..

I imagine that is indeed not possible, and probably the reason this was done this way.

That said, there's a comment in this code that mentions Windows 8, so we should double check if Windows 10 didn't introduce a new API for this.


A couple of things to improve:
  - we don't really need to call this every 1 second.. Let's bump the interval
  - it should use requestIdleCallback.. that will probably mean re'scheduling it after every callback, because I don't think there's a requestIdleInterval..
  - make sure that these calls stop whenever about:preferences:
    - is closed (this is probably true, but we need to ensure that the changes don't regress that), or
    - is not the foreground tab (I doubt that this is true at the moment), or
    - the "General" pane is not the one selected
Priority: P3 → P1
(Assignee)

Comment 5

2 years ago
Posted patch 1357154-schedule.patch (obsolete) — Splinter Review
(1/2) - schedule default browser polling for Windows in requestIdleCallback.
Attachment #8880492 - Flags: review?(florian)
(Assignee)

Comment 6

2 years ago
(2/2) - cache Firefox's executable path in nsWindowShellService::IsDefaultPath.
Attachment #8880495 - Flags: review?(ehsan)

Comment 7

2 years ago
Comment on attachment 8880495 [details] [diff] [review]
Cache executable path in Windows

Review of attachment 8880495 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/shell/nsWindowsShellService.cpp
@@ +230,4 @@
>      return NS_OK;
>    }
>  
> +  if (!cachedExeLongPath[0]) {

One thing to note here is that if GetLongPathNameW fails here on the first call, cachedExeLongPath[0] may end up remaining non-null, so on the second call to the function the function may expect the cache to be valid and use it while the cached variable may contain the short path...

@@ +230,5 @@
>      return NS_OK;
>    }
>  
> +  if (!cachedExeLongPath[0]) {
> +      if (!::GetModuleFileNameW(0, cachedExeLongPath, MAX_BUF)) {

This information is already obtained in BinaryPath::Get() basically in the beginning of main().  I think it is better to move this layer of caching there and call BinaryPath::Get() here.

@@ +235,5 @@
> +        return NS_OK;
> +      }
> +      // Convert the path to a long path since GetModuleFileNameW returns the path
> +      // that was used to launch Firefox which is not necessarily a long path.
> +      if (!::GetLongPathNameW(cachedExeLongPath, cachedExeLongPath, MAX_BUF)) {

But since you need the long version of the path, why not add a GetLong() helper method to BinaryPath that does this (and the caching needed) and use it here?
Attachment #8880495 - Flags: review?(ehsan)
Comment on attachment 8880492 [details] [diff] [review]
1357154-schedule.patch

Review of attachment 8880492 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not convinced the approach is correct:
- could we change nsIShellService to add an API doing the isDefaultBrowser check asynchronously (ie. off main thread), and call a callback with the result?
- 5s is a very long time to wait to see the UI be updated when the preferences window is in the foreground.
- why do we need timers at all when about:preferences is in a background tab?

I also have some comments about the patch itself (by the way, please include 8 lines of context in future patches), mostly coding style details:

::: browser/components/preferences/in-content-new/main.js
@@ +57,5 @@
>          // in case the default changes. On other Windows OS's defaults can also
>          // be set while the prefs are open.
> +        let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                    .getService(Ci.nsIWindowMediator);
> +        let win = wm.getMostRecentWindow("navigator:browser");

Services.wm

@@ +59,5 @@
> +        let wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                    .getService(Ci.nsIWindowMediator);
> +        let win = wm.getMostRecentWindow("navigator:browser");
> +
> +        function pollForDefaultBrowser() {

Use something like:
let pollForDefaultBrowser = () => {
and then you no longer need the .bind calls.

@@ +62,5 @@
> +
> +        function pollForDefaultBrowser() {
> +          let uri = win.gBrowser.currentURI.spec;
> +
> +          if ((uri === "about:preferences" || uri === "about:preferences#general") &&

Our code usually uses == rather than ===, unless we specifically need ===.

@@ +64,5 @@
> +          let uri = win.gBrowser.currentURI.spec;
> +
> +          if ((uri === "about:preferences" || uri === "about:preferences#general") &&
> +              document.visibilityState === "visible") {
> +                this.updateSetDefaultBrowser();

nit: the indent is incorrect on this line.
Attachment #8880492 - Flags: review?(florian)
(Assignee)

Comment 9

2 years ago
Attachment #8880492 - Attachment is obsolete: true
Attachment #8880495 - Attachment is obsolete: true
Attachment #8881876 - Flags: review?(nfroyd)
(Assignee)

Comment 10

2 years ago
Attachment #8881878 - Flags: review?(florian)
Comment on attachment 8881876 [details] [diff] [review]
Cache Windows executable path after BinaryPath::Get is called in main

Review of attachment 8881876 [details] [diff] [review]:
-----------------------------------------------------------------

Bouncing back to Ehsan, since he has some context on this already.
Attachment #8881876 - Flags: review?(nfroyd) → review?(ehsan)

Comment 12

2 years ago
Comment on attachment 8881876 [details] [diff] [review]
Cache Windows executable path after BinaryPath::Get is called in main

Review of attachment 8881876 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/build/BinaryPath.h
@@ +20,5 @@
>  #include "mozilla/UniquePtrExtensions.h"
>  
>  #ifdef MOZILLA_INTERNAL_API
> +#include "nsCOMPtr.h"
> +#include "nsIFile.h"

It doesn't seem like you are using either of these.
Attachment #8881876 - Flags: review?(ehsan) → review+
(Assignee)

Comment 13

2 years ago
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #12)
> It doesn't seem like you are using either of these.

I don't, but they're used here: http://searchfox.org/mozilla-central/source/xpcom/build/BinaryPath.h#176, and the original files that used BinaryPath.h (nsBrowserApp.cpp and nsAppRunner.cpp) seemed to #include those two headers above BinaryPath.h. That wasn't the case for nsWindowShellService.cpp, so I added those two #includes there.

Comment 14

2 years ago
(In reply to :Perry Jiang from comment #13)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #12)
> > It doesn't seem like you are using either of these.
> 
> I don't, but they're used here:
> http://searchfox.org/mozilla-central/source/xpcom/build/BinaryPath.h#176,
> and the original files that used BinaryPath.h (nsBrowserApp.cpp and
> nsAppRunner.cpp) seemed to #include those two headers above BinaryPath.h.
> That wasn't the case for nsWindowShellService.cpp, so I added those two
> #includes there.

Oh I see, makes sense!  Thanks.
Comment on attachment 8881878 [details] [diff] [review]
Schedule IsDefaultBrowser in requestIdleCallback

Review of attachment 8881878 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks (and sorry for the delay!)

(In reply to Florian Quèze [:florian] [:flo] from comment #8)

> I'm not convinced the approach is correct:
> - could we change nsIShellService to add an API doing the isDefaultBrowser
> check asynchronously (ie. off main thread), and call a callback with the
> result?

Felipe convinced me that caching the path (ie. attachment 8881876 [details] [diff] [review]) should be enough to avoid the jank, and that using an idle callback is all that remained to do in the front-end code :-).

::: browser/components/preferences/in-content-new/main.js
@@ +57,5 @@
>          // when the user will select the default.  We refresh here periodically
>          // in case the default changes. On other Windows OS's defaults can also
>          // be set while the prefs are open.
> +        let wm = Services.wm;
> +        let win = wm.getMostRecentWindow("navigator:browser");

nit: you don't need the 'wm' variable.
Attachment #8881878 - Flags: review?(florian) → review+

Comment 17

2 years ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/226d0fafbd95
Cache Windows executable path after BinaryPath::Get is called in main. r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6b00d91e1c
Schedule IsDefaultBrowser from about:preferences using requestIdleCallback. r=florian

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/226d0fafbd95
https://hg.mozilla.org/mozilla-central/rev/3d6b00d91e1c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Iteration: --- → 56.2 - Jul 10
You need to log in before you can comment on or make changes to this bug.