Created attachment 786570 [details] [diff] [review] setgpid For some reason we need to call setpgid in the child. Bill made this change on larch. Without this patch setting a breakpoint in gdb is impossible, because the browser crashes after continuing execution. I will leave it Bill to explain this.
It seems like this probably isn't the right thing to do, but I don't know what's right. The problem is that, when you hit ^C, all processes in the foreground process group receive the SIGINT. If you hit ^C in gdb, gdb normally traps the signal and instead suspends the debuggee. However, it only does this for the process being debugged. So child processes will still get the signal (since they're in the same process group as the parent) and they'll get killed. The patch just puts all child processes in their own process group. This is good for gdb. However, it means that when you're running FF outside a debugger, hitting ^C to kill won't actually kill the children. In reality they'll probably die off soon after the parent dies, but it still seems kind of bad. Maybe bsmedberg will have an idea what to do here.
You could just trap and ignore SIGINT in child processes. Note that IPC channels should notice when the other side closes, fire ActorDestroyed on the root actor, and that should abort the child process.
The problem is that we also want to be able to debug the child process while the parent is running. In that case we don't want ^C to kill the parent. And we don't want to ignore SIGINT in the parent. Maybe I'll try to figure out what chrome does.
I'm confused, I think. If you're debugging a child process, your gdb should be running in a separate terminal session, right? So control-c in that session shouldn't affect either the child or the parent. I was only suggesting to ignore SIGINT in child processes, not the parent process, which should be possible by switching on XRE_GetProcessType
Oh, I see, you're right. I forgot that child processes will get debugged in a different terminal.
Created attachment 787246 [details] [diff] [review] signal-handler This does as suggested.
Comment on attachment 787246 [details] [diff] [review] signal-handler Ted, can you review this?
Attachment #787246 - Flags: review?(benjamin) → review?(ted)
Comment on attachment 787246 [details] [diff] [review] signal-handler Review of attachment 787246 [details] [diff] [review]: ----------------------------------------------------------------- I was a little wary of side-effects of this, but I suspect SIGINT isn't useful for much else so ignoring it shouldn't be terribly harmful. I doubt anyone really expects to be able to Ctrl+C kill non-foreground processes anyway.
Attachment #787246 - Flags: review?(ted) → review+
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.