539406 - e10s: Content processes do not die when started asynchronously

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

Attachment: Band-aid fix

In the course of my work with jetpacks and multiple processes, I've started creating multiple content processes.  However, the processes do not die when GeckoChildProcesses' destructor is hit, as mProcessHandle is 0 when AsyncLaunch() is used in the ContentProcessParent.  This occurs because AsyncChannel::Open() resets the channel's listener to be the AsyncChannel object, so GeckoChildProcessHost::OnChannelConnected is never called.

Comment 1

[Josh Matthews [:jdm]]

Comment on attachment 421418 [details] [diff] [review]
Band-aid fix

This specifically patches the problem by having AsyncChannel retain any existing listener and call the associated signal on it as well.  I'm not particularly happy with it - suggestions?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Just to make sure I understand ... so you're saying that at, e.g.

ContentProcessParent::ContentProcessParent()
    : mMonitor("ContentProcessParent::mMonitor")
{
    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
    mSubprocess = new GeckoChildProcessHost(GeckoProcessType_Content);
    mSubprocess->AsyncLaunch();
    Open(mSubprocess->GetChannel(), mSubprocess->GetChildProcessHandle());
}

mSubprocess->GetChildProcessHandle() is coming back as garbage or 0?

Comment 3

[Josh Matthews [:jdm]]

Hmm, I don't think I noticed that usage of it.  It probably does come back zero, I'm just not sure where the value is actually used.  The most noticeable effect is in the GeckoChildProcessHost destructor when base::EnsureProcessTerminated isn't called since mProcessHandle is 0.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 421418 [details] [diff] [review]
Band-aid fix

I'm OK with this approach.

>diff --git a/ipc/chromium/src/chrome/common/ipc_channel.h b/ipc/chromium/src/chrome/common/ipc_channel.h
>--- a/ipc/chromium/src/chrome/common/ipc_channel.h
>+++ b/ipc/chromium/src/chrome/common/ipc_channel.h
>@@ -74,6 +74,10 @@
>   // Modify the Channel's listener.
>   void set_listener(Listener* listener);
> 
>+#ifdef CHROMIUM_MOZILLA_BUILD
>+  Listener* get_listener();
>+#endif
>+

I somewhat prefer the interface

  Listener* set_listener(Listener* listener) {
    Listener* old = listener_;
    listener_ = listener;
    return old;
  }

but not particularly strongly.  I'll leave it to your judgment.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> Hmm, I don't think I noticed that usage of it.  It probably does come back
> zero, I'm just not sure where the value is actually used.

It's used by IPDL to (i) kill the child process on fatal errors; (ii) "share" shmem segments to the child on Windows.  That code would have fallen over before your patch, if we had ever tried that in with a content process! ;)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> (In reply to comment #3)
> > Hmm, I don't think I noticed that usage of it.  It probably does come back
> > zero, I'm just not sure where the value is actually used.
> 
> It's used by IPDL to (i) kill the child process on fatal errors; (ii) "share"
> shmem segments to the child on Windows.  That code would have fallen over
> before your patch, if we had ever tried that in with a content process! ;)

I should amend that even after your patch that code will still fall over, but we can cover that in followup bugs.

Comment 7

[Josh Matthews [:jdm]]

Yeah, I noticed that.  I'll submit a revised patch for this bug with your suggested interface, but I'm busy at LCA 2010 for the next couple days.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Band-aid fix v2

The r+ should carry over, since all that changed was the interface according to cjone's suggestions.

Comment 9

[Josh Matthews [:jdm]]

Hey, can somebody with privileges commit this?  It's just been kicking around for a while.

Comment 10

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/projects/electrolysis/rev/32890a8440f3
http://hg.mozilla.org/projects/electrolysis/rev/a388481c3f5e