1580168 - Display PID instead of incremental number in the thread list

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

When enabling fission via fission.autostart and overing the tabs, you can see PID numbers.
We should display these PID in the thread list instead of incremental numbers. So that it is easier to match information displayed in Firefox frontend versus DevTools.

Comment 1

[Jason Laster [:jlast]]

Are you planning on working on this or would you like me to?

Also, where are the PIDs defined? Ideally we would have them in the Target form

Comment 2

[Alexandre Poirot [:ochameau]]

This may depend on platform work as we aren't using the same interfaces than Firefox frontend and it is currently hard to retrieve any PID out of the content process message manager objects we are using.

While this end up being displayed in the debugger, this issue has to be fixed on the root actor over here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/process.js#26-30
id should be the PID and then the debugger UI should display that instead, I think.

I'm currently discussing with the DOM team, I should know more by the end of the week.

Comment 3

[Jason Laster [:jlast]]

Alex, any progress on this?

Comment 4

[Alexandre Poirot [:ochameau]]

Ideally, bug 1580447 would allow to implement that.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1580168 - Display PIDs

Comment 6

[Alexandre Poirot [:ochameau]]

Note that we did that during the Berlin workweek. It works but we concluded with Nika and Andreas that it would be better to stop using the parent process message manager to iterate over processes, but instead do bug 1580447.

That was the plan. Now, if you want to land whatever works, this patch is already there.

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #6)

Note that we did that during the Berlin workweek. It works but we concluded with Nika and Andreas that it would be better to stop using the parent process message manager to iterate over processes, but instead do bug 1580447.

That was the plan. Now, if you want to land whatever works, this patch is already there.

Thanks. Looking at devtools/server/actors/process.js there is some rather fragile looking code to get the process list by looking at the children of the ppmm. This patch is also rather complicated as the parent gets the pid of the content process (information which is already known to the parent in C++) by sending the content process a message, letting it fetch its own pid, and then sending that back to the parent.

I take it that bug 1580442 would replace these with well typed/defined accesses to C++ data in the parent over webidl interfaces? That does seem a better approach, and it would make more sense to do bug 1580442 now than land this patch and back it out later when there is a proper interface available (which will surely be needed at some point).

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Brian Hackett (:bhackett) from comment #7)

(In reply to Alexandre Poirot [:ochameau] from comment #6)

Note that we did that during the Berlin workweek. It works but we concluded with Nika and Andreas that it would be better to stop using the parent process message manager to iterate over processes, but instead do bug 1580447.

That was the plan. Now, if you want to land whatever works, this patch is already there.

Thanks. Looking at devtools/server/actors/process.js there is some rather fragile looking code to get the process list by looking at the children of the ppmm.

Is it fragile to use ppmm.{childCount,getChildAt}?
This PID patch doesn't change anything around that. We still depend on that to iterate the already-existing processes.

This patch is also rather complicated as the parent gets the pid of the content process

Yep. That clearly is the workaround part of this patch.

(information which is already known to the parent in C++)

We saw that when discussing with Nika and Andreas. It is available in C++ on ContentParent but would require lots of glue to get it exposed up to JS.
There is a precedent, I think it is remoteType, extracted from ContentParent up to the interface returned by getChildAt() (is it MessageSender?).
But they both agreed that won't scale. We are most likely want to expose more fields and so, instead of piping each individual field from ContentParent up to MessageSender, we should expose ContentParent directly to JS. Have a WebIDL for it and allows to iterate over the existing one. (We do already have the ipc:xxx events to know for the new ones.)

I take it that bug 1580442 would replace these with well typed/defined accesses to C++ data in the parent over webidl interfaces? That does seem a better approach, and it would make more sense to do bug 1580442 now than land this patch and back it out later when there is a proper interface available (which will surely be needed at some point).

I imagine you meant bug 1580447? Yes, that's what we agreed on with Fission team and why I was kind of holding a bit on this bug.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #8)

Is it fragile to use ppmm.{childCount,getChildAt}?

I was thinking of this code:

  getList: function() {
    const processes = [];
    for (let i = 0; i < ppmm.childCount; i++) {
      processes.push({
        // XXX: may not be a perfect id, but process message manager doesn't
        // expose anything...
        id: i,
        // XXX Weak, but appear to be stable
        parent: i == 0,
        // TODO: exposes process message manager on frameloaders in order to compute this
        tabCount: undefined,
      });
    }
    this._mustNotify = true;
    this._checkListening();

    return processes;
  },

Using the index into the children as the id isn't all that great as it will not be stable when the process list changes. Assuming that the parent is the first process is fragile, as at least judging from the code here there isn't much reason to believe that the first process will always be the parent (including after future changes to the codebase).

My understanding from bug 1580447 and what you wrote in comment 6 of this bug is that we'd want IDL interfaces that can replace this loop, iterating over the processes in a way that avoids using the ppmm, and uses new IDL accessors so that the pid can be used as the id. This seems like it should also allow determining which process is the parent process in a cleaner way.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Brian Hackett (:bhackett) from comment #9)

(In reply to Alexandre Poirot [:ochameau] from comment #8)
Using the index into the children as the id isn't all that great as it will not be stable when the process list changes. Assuming that the parent is the first process is fragile, as at least judging from the code here there isn't much reason to believe that the first process will always be the parent (including after future changes to the codebase).

Ah Ok, you were refering to the existing m-c code, yes, I agree!
With the attached patched, it is slightly better as the id is now a real PID instead of an increment.
But we still depend on the order to tell which one is the parent process (parent: i == 0, check is still here).

My understanding from bug 1580447 and what you wrote in comment 6 of this bug is that we'd want IDL interfaces that can replace this loop, iterating over the processes in a way that avoids using the ppmm, and uses new IDL accessors so that the pid can be used as the id. This seems like it should also allow determining which process is the parent process in a cleaner way.

Yes. The idea is to expose ContentParent directly to JS via WebIDL, instead of going throught all these intermediate classes related to ppmm. If you want to look into that, you might want to sync up with Nika or Andreas as there is some GC consideration in order to make ContentParent be exposed to JS.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: patch

With this patch, the threads list in the omniscient debugger toolbox shows pids instead of a counter-based id. The strategy here is a little different from what was discussed in previous comments. This modifies the WebIDL for ProcessMessageManager to add accessors indicating whether the manager is for the parent vs. a content process, and to get the PID of the manager's target process. The devtools use this to fill in the process actor forms, but the ppmm is still used to iterate through all the process message managers.

I avoided creating IDL for ContentParent because a) it didn't seem necessary, and b) it would lead to non-uniform interfaces which the devtools server use to work with other processes. ContentParent instances are only created for content processes, but the devtools need to communicate with the parent process as well. ProcessMessageManagers are created for both the parent and content processes (plus, the ContentParent directly owns its ProcessMessageManager), so they seem a better place to hang information the devtools will need.

Comment 12

[Brian Hackett [Laid off!]]

Also, I'm not sure how to test this. I compared behavior with and without this patch but some things seem pretty broken in both cases, like the process list not updating when opening or closing tabs. Are there any automated tests I should run?

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Bug 1580168 Part 1 - Add processID and isParent accessors to ProcessMessageManager, r=nika.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: Bug 1580168 Part 2 - Use PID as ID of child process actors, r=ochameau.

Depends on D48084

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to Brian Hackett (:bhackett) from comment #11)

I avoided creating IDL for ContentParent because a) it didn't seem necessary, and

We did not reached this conclusion in Berlin. ProcessMessageManagers looked like superficial intermediate interfaces which make it unecessarily hard to expose any attribute we care about ContentParent up to ProcessMessageManager interfaces.
Nika or Andreas may elaborate more about this.

b) it would lead to non-uniform interfaces which the devtools server use to work with other processes. ContentParent instances are only created for content processes, but the devtools need to communicate with the parent process as well. ProcessMessageManagers are created for both the parent and content processes (plus, the ContentParent directly owns its ProcessMessageManager), so they seem a better place to hang information the devtools will need.

The two cases: parent process versus content processes are already having distinct codepath.
The first MessageManager returned by ppmm.getChildAt(0) is considered special as it represents the parent process. But we do not use the returned message manager for the parent process.
We ignore it, and are not using the message manager when we debug the parent process:
https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/process.js#114
Also we will be interested into attributes that are specific to content processes and won't have any equivalent for the parent process. (ex: remoteType) So I think it may become benefitial to have content-process specific attributes and class here.
Finally, note that the PID can be retrieved easily without this ppmm interface: Services.appinfo.processID.

Comment 16

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #15)

The first MessageManager returned by ppmm.getChildAt(0) is considered special as it represents the parent process.

There is no documentation to this effect and AFAICT this is only the case because the parent process always happens to create its own ProcessMessageManager first and attach it to the ppmm. The isParent accessor establishes this property in a documented and intuitive fashion.

Also we will be interested into attributes that are specific to content processes and won't have any equivalent for the parent process. (ex: remoteType) So I think it may become benefitial to have content-process specific attributes and class here.

Maybe it will, but this isn't necessary to fix this bug. The accessors added here are reasonable properties for a ProcessMessageManager to have.

Finally, note that the PID can be retrieved easily without this ppmm interface: Services.appinfo.processID.

No, it can't. This accessor returns the pid of the currently running process. The interfaces added here allow the pids of child processes to be accessed from the parent process, without needing any complicated IPC.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a503cd521e0
Part 1 - Add processID and isParent accessors to ProcessMessageManager, r=nika.
https://hg.mozilla.org/integration/autoland/rev/d22a259e7a03
Part 2 - Use PID as ID of child process actors, r=ochameau.

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8a503cd521e0
https://hg.mozilla.org/mozilla-central/rev/d22a259e7a03