1297792 - Add GPU process to about:support

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

If the GPU process is running, we should have a row in about:support that lists the PID. In nightly builds it'd also be nice to have a "kill" button that terminates it.

Comment 1

[Benjamin Smedberg]

We don't show the PID or a kill button for any other processes. I'm not sure why we need/want this for this one in particular.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> We don't show the PID or a kill button for any other processes. I'm not sure
> why we need/want this for this one in particular.

Maybe the PID is not needed outside of nightly, but some indicator should be there. On nightly having the PID/kill button could be quite useful to folks working or testing the GPU process.

Comment 3

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1297792-Add-a-diagnostic-button-to-kill-the-GPU-.patch

No need to put the GPU process status in here as it shows under the "Decision Log" section.

Comment 4

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8806492 [details] [diff] [review]
0001-Bug-1297792-Add-a-diagnostic-button-to-kill-the-GPU-.patch

forgot to add the pid to this.

Comment 5

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1297792-Add-a-diagnostic-button-to-kill-the-GPU-.patch

OK, updated patch with pid visible. I also now check if the pid is -1 (no gpu process) to determine whether to show the diag or not.

Comment 6

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

Comment on attachment 8806802 [details] [diff] [review]
0001-Bug-1297792-Add-a-diagnostic-button-to-kill-the-GPU-.patch

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

Tentative r+, but note that there's going to be 0 user feedback when this occurs, and then the information on the page (like the GPU pid) will be out of date. about:support isn't something most users end up getting to, and it's clear our design criteria there are a lot looser. Still, a thing to keep in mind if you weren't already aware.

::: toolkit/content/aboutSupport.js
@@ +292,5 @@
>  
> +    if (AppConstants.NIGHTLY_BUILD) {
> +
> +      let gpuProcessPid = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIDOMWindowUtils)

You get nsIDOMWindowUtils twice. Might as well alias it somewhere instead of getInterface'ing it both times.

@@ +304,5 @@
> +                .getInterface(Ci.nsIDOMWindowUtils)
> +                .terminateGPUProcess();
> +        });
> +
> +        gpuProcessKillButton.addEventListener("click", terminateGPUProcess);

Might as well just do:

gpuProcessKillButton.addEventListener("click", function(e) {
  winUtils.terminateGPUProcess();
});

Also, as far as I can tell, there's no user feedback that this was successful. I guess that's okay?

@@ +306,5 @@
> +        });
> +
> +        gpuProcessKillButton.addEventListener("click", terminateGPUProcess);
> +        gpuProcessKillButton.textContent = strings.GetStringFromName("gpuProcessKillButton");
> +        addRow("diagnostics", "GPUProcessPid", gpuProcessPid);

This is going to go out of date once the GPU process is killed and respawned. Is that okay?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8806802 [details] [diff] [review]
0001-Bug-1297792-Add-a-diagnostic-button-to-kill-the-GPU-.patch

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

Thanks! I had applied this locally and it already helped me quickly find two restart bugs.

Only suggestion is that maybe we should #ifdef NIGHTLY_BUILD the kill button.

Comment 8

[George Wright (:gw280) (needinfo me!)]

The button is already nightly only, so no issues there.

I think that no visual feedback isn't a huge problem. This is a nightly-only thing that's intended for debug use only, and really the entire page should be refreshed when the GPU process dies. There might be value in forcing a reload, but I want to avoid updating some data and giving the impression that what's on the page is current, when it really is all out of date.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7157478b9a48
Add a diagnostic button to kill the GPU process from about:support r=dvander,mconley

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7157478b9a48