542341 - Add RequestRunToCompletion method to ContentProcessParent

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Patch to add bool ContentProcessParent::RequestRunToCompletion()

Thanks to nsIThreadObserver::AfterProcessNextEvent, we can support a very simple interface for blocking the child process until the parent process has finished processing its current event.

I propose adding the method

  bool ContentProcessParent::RequestRunToCompletion();

which will call and return the result of BlockChild(), and ensure that UnblockChild() is called before the parent process returns to its event loop.  It should be fine to call RequestRunToCompletion() multiple times, but of course only the first call during a given event should actually trigger BlockChild().

Some care has to be taken to call UnblockChild() only when the current event has been processed, and not when any nested events (such as user input events for modal dialogs) are processed.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 423632 [details] [diff] [review]
Patch to add bool ContentProcessParent::RequestRunToCompletion()

Jonas mentioned that smaug may have experience working with JS run-to-completion and nested events, so perhaps he knows a better way to do this.

Comment 2

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

Comment on attachment 423632 [details] [diff] [review]
Patch to add bool ContentProcessParent::RequestRunToCompletion()

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
>--- a/dom/ipc/ContentProcessParent.cpp
>+++ b/dom/ipc/ContentProcessParent.cpp
>-NS_IMPL_ISUPPORTS1(ContentProcessParent, nsIObserver)
>+NS_IMPL_THREADSAFE_ISUPPORTS2(ContentProcessParent,
>+                              nsIObserver,
>+                              nsIThreadObserver)
> 

Why does this need to be thread safe?

>+/* void afterProcessNextEvent (in nsIThreadInternal thread, in unsigned long recursionDepth); */
>+NS_IMETHODIMP
>+ContentProcessParent::AfterProcessNextEvent(nsIThreadInternal *thread,
>+                                            PRUint32 recursionDepth)
>+{
>+    if (mRunToCompletionDepth &&
>+        !--mRunToCompletionDepth) {
>+#ifdef DEBUG
>+            printf("... ran to completion.\n");
>+#endif
>+            UnblockChild();
>+    }
>+

UnblockChild() is fallible, technically, but I think that any
failure will lead to the child process being terminated, so
you're OK here.

One more point: this is no fault of your patch, but
ContentProcessParent should implement the
|ActorDestroy(ActorDestroyReason why)| interface, which is
invoked when ContentProcessParent is "destroyed" wrt to IPDL.
When that method is invoked, the child is "dead" for all intents
and purposes.  This will be needed to migrate to the
multiple-content-process world, but more apropos to your patch, I
think you should be unhooking ContentProcessParent from thread
observation and reinstalling |mOldObserver| on |ActorDestroy()|.
You should also set mRunToCompletionDepth to 0 there.  Otherwise,
even now when a content process crashes, the "zombie"
ContentProcessParent will continue to uselessly observe events
forever, AFAICT.

If this is more than you prefer to bite off ATM, I have no
problem with this being done in a follow up.

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Addressing cjones's suggestions.

(In reply to comment #2)
> >+NS_IMPL_THREADSAFE_ISUPPORTS2(ContentProcessParent,
> >+                              nsIObserver,
> >+                              nsIThreadObserver)
>
> Why does this need to be thread safe?

nsThread::PutEvent can be called from any thread, and that method contains the statement

  nsCOMPtr<nsIThreadObserver> obs = GetObserver();

which can cause ContentProcessParent* to be AddRef'd from the current (i.e., any) thread.

> UnblockChild() is fallible, technically, but I think that any
> failure will lead to the child process being terminated, so
> you're OK here.

I'll trust you :)

> You should also set mRunToCompletionDepth to 0 there.

Do you think it's safe/wise/necessary to call UnblockChild() if mRunToCompletionDepth > 0 at the time of destruction?

> If this is more than you prefer to bite off ATM, I have no
> problem with this being done in a follow up.

Nah, this is simple enough and definitely worth doing.

Comment 4

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

(In reply to comment #3)
> > You should also set mRunToCompletionDepth to 0 there.
> 
> Do you think it's safe/wise/necessary to call UnblockChild() if
> mRunToCompletionDepth > 0 at the time of destruction?
> 

Nah, IPC is disconnected at that point.

Comment 5

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

Comment on attachment 423895 [details] [diff] [review]
Addressing cjones's suggestions.

>diff --git a/dom/ipc/ContentProcessParent.cpp b/dom/ipc/ContentProcessParent.cpp
>--- a/dom/ipc/ContentProcessParent.cpp
>+++ b/dom/ipc/ContentProcessParent.cpp
>@@ -74,21 +74,46 @@ ContentProcessParent::GetSingleton()
>+void
>+ContentProcessParent::ActorDestroy(ActorDestroyReason why)
>+{
>+    nsCOMPtr<nsIThreadObserver>
>+        kungFuDeathGrip(static_cast<nsIThreadObserver*>(this));
>+    nsCOMPtr<nsIObserverService>
>+        obs(do_GetService("@mozilla.org/observer-service;1"));
>+    if (obs)
>+        obs->RemoveObserver(static_cast<nsIObserver*>(this), "xpcom-shutdown");
>+    nsCOMPtr<nsIThreadInternal>
>+        threadInt(do_QueryInterface(NS_GetCurrentThread()));
>+    if (threadInt)
>+        threadInt->SetObserver(mOldObserver);
>+    if (mRunToCompletionDepth) {
>+        mRunToCompletionDepth = 0;
>+        UnblockChild();
>+    }
>+}
>+

As we discussed, UnblockChild() isn't needed there.  (It'll just error out.)

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Pushed to projects/electrolysis:
http://hg.mozilla.org/projects/electrolysis/rev/c2d2de888c99