545237 - [e10s] ContentProcessParent is a single-use singleton

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

Once the process dies, the singleton dies and is never created again.  This means that the 'Recover' button in test.xul actually ends up segfaulting the chrome process which tries to dereference a null pointer.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Keep track of the singleton's life and recreate it if necessary

First and foremost, the gSingletonDied variable is stopping the singleton from being recreated.  However, there are some racy life issues wherein sometimes the object becomes unreferenced within ActorDestroy, so the frameloader now holds a perpetual reference to the singleton.  This means that there may actually be two (or more) instances of the class floating around, but as long as the frameloader doesn't leak, they should all be cleaned up eventually.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Notify PIFrameEmbedding's owner on actor deletion

When the content process is killed, the owning frameloader is not made aware of this fact and can try to use the protocol after its death.  This patch fixes this problem.

Comment 3

[Josh Matthews [:jdm]]

There's some cruft left over from dealing with mq in TabParent.cpp in the second patch; where it says |if(nsFrameLoader)| I have changed it locally to read |if(frameLoader)|.

Comment 4

[Josh Matthews [:jdm]]

I suspect that the PIFrameEmbedding patch is subsumed by the patch in bug 545757.

Comment 5

[Benjamin Smedberg]

Comment on attachment 426139 [details] [diff] [review]
Keep track of the singleton's life and recreate it if necessary

>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp


>+    mChildHost = static_cast<nsCOMPtr<nsIObserver> >(parent);

This is a strange cast. Why is mChildHost a nsISupports instead of something more specific? That way mChildHost = parent; would compile, but even in this case you probably want to disambiguate with mChildHost = static_cast<nsIObserver*>(parent), not through a temporary nsCOMPtr.

>diff --git a/content/base/src/nsFrameLoader.h b/content/base/src/nsFrameLoader.h

>+    , mChildHost(nsnull)

It's a nsCOMPtr, it doesn't need to (and shouldn't) be initialized.

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp


>+    //nsCOMPtr<nsIObserver> ContentProcessParent::gSingleton;
> ContentProcessParent* ContentProcessParent::gSingleton;

leftover commented code should be removed. Static nsCOMPtrs are forbidden.

> ContentProcessParent::~ContentProcessParent()
> {
>     NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>-    NS_ASSERTION(gSingleton == this, "More than one singleton?!");
>-    gSingletonDied = PR_TRUE;
>-    gSingleton = nsnull;
>+    //If the previous content process has died, a new one could have
>+    //been started since.
>+    if(gSingleton == this)
>+        gSingleton = nsnull;

Nit, un-snuggle the if(

Comment 6

[Josh Matthews [:jdm]]

Attachment: Keep track of the singleton's life and recreate it if necessary v1.5

The nsCOMPtr now holds an nsIObserver.  All other nits addressed.  Could somebody with commit privileges check this in, please?

Comment 7

[Josh Matthews [:jdm]]

Attachment: Notify PIFrameEmbedding's owner on actor deletion

I was wrong, this patch was not subsumed by the other bug.  The frameloader needs to know when the content process goes down hard, and this patch delivers.

Comment 8

[Benjamin Smedberg]

Comment on attachment 429795 [details] [diff] [review]
Notify PIFrameEmbedding's owner on actor deletion

I think the `useIPC` argument is unnecessary. If you're in ActorDestroy you can still call PIFrameEmbeddingParent::Send__delete__ and it will just fail.

In either case, DestroyChild needs a doccomment.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Notify PIFrameEmbedding's owner on actor deletion

Review comments addressed.

Comment 10

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/projects/electrolysis/rev/46a6412b4a7d