Closed Bug 545237 Opened 10 years ago Closed 10 years ago

[e10s] ContentProcessParent is a single-use singleton


(Core :: IPC, defect)

Not set





(Reporter: jdm, Assigned: jdm)




(2 files, 3 obsolete files)

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.
Assignee: nobody → josh
Blocks: e10s
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.
Attachment #426139 - Flags: review?
Attachment #426139 - Flags: review? → review?(benjamin)
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.
Attachment #426140 - Flags: review?(benjamin)
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)|.
I suspect that the PIFrameEmbedding patch is subsumed by the patch in bug 545757.
Depends on: 545757
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(
Attachment #426139 - Flags: review?(benjamin) → review+
The nsCOMPtr now holds an nsIObserver.  All other nits addressed.  Could somebody with commit privileges check this in, please?
Attachment #426139 - Attachment is obsolete: true
Attachment #426140 - Flags: review?(benjamin)
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.
Attachment #426140 - Attachment is obsolete: true
Attachment #429795 - Flags: review?(benjamin)
Attachment #429795 - Flags: review?(benjamin) → review-
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.
Review comments addressed.
Attachment #429795 - Attachment is obsolete: true
Attachment #430724 - Flags: review?(benjamin)
Attachment #430724 - Flags: review?(benjamin) → review+
Blocks: 556846
Keywords: checkin-needed
Whiteboard: [land in e10s]
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: ContentProcessParent is a single-use singleton → [e10s] ContentProcessParent is a single-use singleton
Whiteboard: [land in e10s]
You need to log in before you can comment on or make changes to this bug.