1518923 - simplify subprocess handle duplication logic in PerformAsyncLaunch

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Nathan Froyd [:froydnj]]

Avoiding handle duplication for certain kinds of processes and allowing
it for everything else seems to be what we're already doing, so let's
make it easier to add new process types with that scheme in mind.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: simplify subprocess handle duplication logic in PerformAsyncLandle

This way of writing things makes it easier to add new process types, which
presumably always want the handle duplication logic discussed here.  I realize
this is not great if you *don't* actually want this duplication (locking down
the sandbox, I guess/), but am not sure of the best way to fix this.  Maybe
static asserting on the number of process types, so when somebody adds one,
it's obvious that you might have to think about whether you need this bit of
code or not?

Of course, we also have the opposite problem with this patch, which is when you
do want to lock something down, you have to remember to go *add* code, versus
removing it.  So maybe this is not such a great idea after all...?

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 9035443 [details] [diff] [review]
simplify subprocess handle duplication logic in PerformAsyncLandle

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

I'm going to redirect the review to Bob Owen, because there might be implications I'm not aware of and/or plans to change the default here.  (I think we talked about this some at the meeting in December: a wrapper class could deal with passing handles child->parent easily enough, but child->child — like between content and a utility process like RDD — would still need the parent to intercede somehow, was my understanding.  So maybe not.)

Also, typo in the commit message: should be `PerformAsyncLaunch`.

Comment 3

[Nathan Froyd [:froydnj]]

Oh my, that's quite a typo, I'm not sure how that happened.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: simplify subprocess handle duplication logic in PerformAsyncLaunch

See comment 1 and comment 2.

Comment 5

[Bob Owen (:bobowen)]

Comment on attachment 9036047 [details] [diff] [review]
simplify subprocess handle duplication logic in PerformAsyncLaunch

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

This only really comes into play when the process is not sandboxed, so unless turned off, by default that would be GPU and VR at the moment.
Hopefully they will be sandboxed by default soon and any new process should be.
So, I think that this makes sense given that we would want things like the handle duplication to work if the sandbox is turned off for testing.
It's one less thing for people to change.

(As an aside it would be nice if we could launch all processes through the sandbox code even if the policy was the same as the parent, but I think we still have issues with some things that need to disable the sandbox to compile.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b520f03935
simplify subprocess handle duplication logic in PerformAsyncLaunch; r=bobowen

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b3b520f03935