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

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

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

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.

Comment 1

[(no longer active)]

Another candidate for idle dispatch.

Comment 2

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

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

[(no longer active)]

This is *completely expected*.  Any synchronous IO operation must be expected to not have any bounded execution time.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: 1357154-schedule.patch

(1/2) - schedule default browser polling for Windows in requestIdleCallback.

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Cache executable path in Windows

(2/2) - cache Firefox's executable path in nsWindowShellService::IsDefaultPath.

Comment 7

[(no longer active)]

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?

Comment 8

[Florian Quèze [:florian]]

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.

Comment 9

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Cache Windows executable path after BinaryPath::Get is called in main

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Schedule IsDefaultBrowser in requestIdleCallback

Comment 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[(no longer active)]

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.

Comment 13

[Perry Jiang [:perry] [no longer employee, use ni?]]

(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

[(no longer active)]

(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 15

[Florian Quèze [:florian]]

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.

Comment 16

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1357154: Schedule IsDefaultBrowser in requestIdleCallback

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/226d0fafbd95
https://hg.mozilla.org/mozilla-central/rev/3d6b00d91e1c