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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

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.

Comment 1

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

Sorry jlebar, don't know why I typed your handle right after I said "Taking".

Comment 2

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

Attachment: Patch for testing; hangs Camera app

Comment 3

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

Attachment: Set a timer to ensure content processes are killed if their tabs take a long time to shut down

Comment 4

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

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).

Comment 5

[Justin Lebar (not reading bugmail)]

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.)

Comment 6

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

Attachment: Set a timer to ensure content processes are killed if their tabs take a long time to shut down, v1.1

This guards against wedged child windows too.  timdream confirmed that gaia removes all these from the DOM on force-close.

Comment 7

[Justin Lebar (not reading bugmail)]

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.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/250a95a6ff00

Comment 9

[Ed Morley [:emorley]]

Backed out for various timeouts in mochitest-2:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=250a95a6ff00

https://hg.mozilla.org/integration/mozilla-inbound/rev/7779197500a9

Comment 10

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

Thanks!

Unsurprisingly, it didn't fail on the platform I tested :/.  I think I know what's wrong though.

Comment 11

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

Attachment: v2

Fixes
 - mark ContentParent dead when we start killing it (again)
 - don't fall over on cancelled popups

Comment 12

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

(rs=jlebar on IRC)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://bug828114.bugzilla.mozilla.org/attachment.cgi?id=700352

Comment 14

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ee7b78ac41d0

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b8bae43380b6