Add GPU process to about:support

RESOLVED FIXED in Firefox 52

Status

()

Core
Graphics
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: dvander, Assigned: gw280)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox52 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
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.

Updated

10 months ago
tracking-e10s: --- → ?
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.

Updated

10 months ago
tracking-e10s: ? → +
(Reporter)

Comment 2

10 months ago
(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.
Assignee: dvander → nobody
Status: ASSIGNED → NEW
Whiteboard: [gfx-noted]
(Reporter)

Updated

9 months ago
Blocks: 1307578
(Reporter)

Updated

9 months ago
No longer blocks: 1264543
(Assignee)

Updated

8 months ago
Assignee: nobody → gwright
(Assignee)

Updated

8 months ago
Depends on: 1314426
(Assignee)

Comment 3

8 months ago
Created attachment 8806492 [details] [diff] [review]
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.
Attachment #8806492 - Flags: review?(mconley)
Attachment #8806492 - Flags: review?(dvander)
(Assignee)

Comment 4

8 months ago
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.
Attachment #8806492 - Flags: review?(mconley)
Attachment #8806492 - Flags: review?(dvander)
(Assignee)

Comment 5

8 months ago
Created attachment 8806802 [details] [diff] [review]
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.
Attachment #8806492 - Attachment is obsolete: true
Attachment #8806802 - Flags: review?(mconley)
Attachment #8806802 - Flags: review?(dvander)
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?
Attachment #8806802 - Flags: review?(mconley) → review+
(Reporter)

Comment 7

8 months ago
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.
Attachment #8806802 - Flags: review?(dvander) → review+
(Assignee)

Comment 8

8 months ago
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

8 months ago
Pushed by gwright@mozilla.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7157478b9a48
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.