Closed Bug 1518923 Opened 1 year ago Closed 1 year ago

simplify subprocess handle duplication logic in PerformAsyncLaunch

Categories

(Core :: IPC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.

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...?
Attachment #9035443 - Flags: review?(jld)
Blocks: 1514039
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`.
Attachment #9035443 - Flags: review?(jld) → review?(bobowencode)

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

Summary: simplify subprocess handle duplication logic in PerformAsyncLandle → simplify subprocess handle duplication logic in PerformAsyncLaunch
Attachment #9035443 - Attachment is obsolete: true
Attachment #9035443 - Flags: review?(bobowencode)
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.)
Attachment #9036047 - Flags: review?(bobowencode) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b520f03935
simplify subprocess handle duplication logic in PerformAsyncLaunch; r=bobowen
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.