Closed Bug 905896 Opened 7 years ago Closed 7 years ago

[e10s] Don't deadlock on a child process for a CPOW

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch timeouts.patch (obsolete) — Splinter Review
Whether this approach will be good long-term is unknown - we may want to have a slow-script dialog, or to allow the child process to continue, or have a timeout backoff. For now let's see what happens if we just kill the process, since that's strictly better than a UI deadlock.

I put the logic in RPCChannel.cpp since ContentParent doesn't have access to RPCChannel, but it feels gross... if you prefer I could just static cast and hook ShouldContinueFromReplyTimeout in ContentParent.
Attachment #791083 - Flags: review?(cjones.bugs)
Comment on attachment 791083 [details] [diff] [review]
timeouts.patch

>diff --git a/ipc/glue/RPCChannel.cpp b/ipc/glue/RPCChannel.cpp
>--- a/ipc/glue/RPCChannel.cpp
>+++ b/ipc/glue/RPCChannel.cpp
>@@ -634,11 +634,41 @@ RPCChannel::OnChannelErrorFromLink()
>     mMonitor->AssertCurrentThreadOwns();
> 
>     if (0 < StackDepth())
>         NotifyWorkerThread();
> 
>     SyncChannel::OnChannelErrorFromLink();
> }
> 
>+static bool sCheckedForDebuggingChildProcess = false;
>+static bool sDebuggingChildProcesses = false;

Nit:

  static enum { UNKNOWN, NOT_DEBUGGING, DEBUGGING } sDebuggingChildren;

or something to that effect.

>+bool
>+RPCChannel::ShouldContinueFromTimeout()

I don't understand why we're overriding this.  In the PContent case,
the only way the parent will block on the child is through urgent
messages, so the SyncChannel::ShouldContinueFromTimeout()
implementation seems fine for now.  If we need something fancier down
the road, I'd rather expose the "IPC stack" to the
ShouldContinueFromReplyTimeout() hook in actors than bake that kind
of logic into the IPC layer.

Thanks for the test :).
Attachment #791083 - Flags: review?(cjones.bugs)
Attached patch v2Splinter Review
w/ nits fixed
Attachment #791083 - Attachment is obsolete: true
Attachment #793244 - Flags: review?(cjones.bugs)
Comment on attachment 793244 [details] [diff] [review]
v2

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

>+bool
>+ContentParent::ShouldContinueFromReplyTimeout()
>+{
>+  // The only time ContentParent sends blocking messages is for CPOWs, so
>+  // timeouts should only ever occur in electrolysis-enabled sessions.

If there's some pref where pref <=> CPOWs enabled, it would be nice to
assert that here.

>diff --git a/ipc/glue/SyncChannel.cpp b/ipc/glue/SyncChannel.cpp

>+    static enum { UNKNOWN, NOT_DEBUGGING, DEBUGGING } sDebuggingChildren = UNKNOWN;

No need for the UNKNOWN, but I don't mind being explicit about the init.

r=me, would be nice to get CPOW assertion in.
Attachment #793244 - Flags: review?(cjones.bugs) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> >+    static enum { UNKNOWN, NOT_DEBUGGING, DEBUGGING } sDebuggingChildren = UNKNOWN;
> 
> No need for the UNKNOWN, but I don't mind being explicit about the init.

No need for the | = UNKNOWN| init, I meant.  But I don't mind.
https://hg.mozilla.org/mozilla-central/rev/f3dce1d8c04f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.