Closed Bug 828114 Opened 7 years ago Closed 7 years ago

Force-closing app won't kill subprocess if main thread is wedged

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: cjones, Assigned: cjones)

Details

Attachments

(3 files, 1 obsolete file)

We found this out in two bugs this week.  The current code does

void
ContentParent::ShutDownProcess()
{
  if (!mIsDestroyed) {
...
    Close();

to close the IPC channel.  We need to add an additional timer here to KillHard() the subprocess if it doesn't respond in time.

We've been relying on this to r- things like slow-script timeout, so this needs to block.  Taking.
Sorry jlebar, don't know why I typed your handle right after I said "Taking".
Assignee: justin.lebar+bug → jones.chris.g
There are some intricacies in this approach around shutting down processes with opened-window tabs.  We can chat tomorrow to see what's feasible (I don't understand that code very well).
Sorry for making you wait all day on this one; I'm not great at the constant
context-switching of these work-weeks.

> void
>+ContentParent::NotifyTabDestroying(PBrowserParent* aTab)
>+{
>+    // There can be more than one PBrowser for a given app process
>+    // because of popup windows.  When the last one starts closing,
>+    // kick off another task to ensure the child process *really*
>+    // shuts down.
>+    if (ManagedPBrowserParent().Length() > 1) {
>+        return;
>+    }
>+
>+    MOZ_ASSERT(!mForceKillTask);
>+    int32_t timeoutSecs =
>+        Preferences::GetInt("dom.ipc.tabs.shutdownTimeoutSecs", 5);
>+    if (timeoutSecs > 0) {
>+        MessageLoop::current()->PostDelayedTask(
>+            FROM_HERE,
>+            mForceKillTask = NewRunnableMethod(this, &ContentParent::KillHard),
>+            timeoutSecs * 1000);
>+    }
>+}

Does the ContentParent always shut down once its last TabParent is destroyed?
There's no way we might resurrect a ContentParent by creating a new PBrowser
inside it?

This looks good to me aside from that concern.  What's the concern you alluded
to in comment 4?  (I can come over to talk if you'd like.)
Attachment #699546 - Flags: review?(justin.lebar+bug) → review+
This guards against wedged child windows too.  timdream confirmed that gaia removes all these from the DOM on force-close.
Attachment #699546 - Attachment is obsolete: true
Attachment #699875 - Flags: review?(justin.lebar+bug)
Comment on attachment 699875 [details] [diff] [review]
Set a timer to ensure content processes are killed if their tabs take a long time to shut down, v1.1

Cool.
Attachment #699875 - Flags: review?(justin.lebar+bug) → review+
Thanks!

Unsurprisingly, it didn't fail on the platform I tested :/.  I think I know what's wrong though.
Attached patch v2Splinter Review
Fixes
 - mark ContentParent dead when we start killing it (again)
 - don't fall over on cancelled popups
Attachment #700352 - Flags: review+
(rs=jlebar on IRC)
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ee7b78ac41d0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.