778866 - Plugin-Container.exe and child plug-in processes can be orphaned when Firefox.exe is killed during a hang

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Jeromie Clark]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120717110313

Steps to reproduce:

Firefox experiences a plug-in related hang.  I then go into task-manager and kill Firefox.exe.


Actual results:

The plugin-container.exe process and FlashPlayer_*.exe processes persist. 


Expected results:

plugin-container.exe and FlashPlayer_*.exe processes should exit when Firefox.exe terminates.

Comment 1

[Jeromie Clark]

We were initially investigating this as a Flash Player bug, but the result of our analysis is that plugin-container doesn't exit when Firefox is killed during a hang. As child processes, we don't exit either.  Upon re-launch, our orphaned processes persist, and users see three or four FlashPlayer_Plugin processes instead of two.

We're tracking this as 3265801

Comment 2

[Matthias Versen [:Matti]]

probably a dupe of bug 577157

Comment 3

[Jeromie Clark]

We consider this issue to be fairly serious.  Flash Player causes the initial hang in plugin-container, but plugin-container does not exit when firefox.exe is killed.

This ultimately results in a a leak of both the plugin-container.exe and FlashPlayerPlugin_xx_xx_xx.exe processes, which can consume a significant amount of system resources.

Please let us know if there's anything we can do to help move this issue along.

Comment 4

[Benjamin Smedberg]

Hrm. Assuming that the main thread of the plugin-container process is not itself hung, it's supposed to detect this condition here http://hg.mozilla.org/mozilla-central/annotate/2dfbe4b9403b/dom/plugins/ipc/PluginModuleChild.cpp#l702 and _exit (we should consider using TerminateProcess here, _exit still runs static destructors and atexit handlers).

I'll debug a bit.

Comment 5

[Benjamin Smedberg]

Killing firefox.exe while plugin-container is running normally, I hit actordestroy and exit properly. So I'm guessing that the main thread is blocked in your case, in which case our I/O thread would have noticed that firefox had gone away and posted an event to the main thread to deal with it, but the I/O thread doesn't commit hara-kiri itself.

We could possibly set up a flag on the Channel itself to have the channel do the aborting during unexpected shutdown: I'm not sure how complicated that would be. cjones?

jeclark, if you could get a stacktrace from the main thread of plugin-container while this is happening, that would be useful to confirm my diagnosis.

Comment 6

[Benjamin Smedberg]

Taking for now to keep this assigned.

Comment 7

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

(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #5)
> We could possibly set up a flag on the Channel itself to have the channel do
> the aborting during unexpected shutdown: I'm not sure how complicated that
> would be. cjones?
> 

It would be very messy.

The case of FF-crashes-and-plugin-main-thread-wedged isn't something I've cared much about so far, but I think the only sane way we have to deal with this is creating a watchdog for the main thread.

Comment 8

[Benjamin Smedberg]

Attachment: Optional abort on channel error, rev. 1

Here's what I'm thinking.

Comment 9

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

Comment on attachment 650323 [details] [diff] [review]
Optional abort on channel error, rev. 1

Sorry for the review lag.

>diff --git a/ipc/glue/AsyncChannel.h b/ipc/glue/AsyncChannel.h
>--- a/ipc/glue/AsyncChannel.h
>+++ b/ipc/glue/AsyncChannel.h
>@@ -102,16 +102,20 @@ public:
> 
>     // Close the underlying transport channel.
>     void Close();
> 
>     // Force the channel to behave as if a channel error occurred. Valid
>     // for process links only, not thread links.
>     void CloseWithError();
> 
>+    void SetAbortOnError(bool abort) {

AssertWorkerThread()

r=me with that

Comment 10

[Benjamin Smedberg]

cjones, that assert would be wrong: I am calling SetAbortOnError from the link thread, not the worker thread.

Comment 11

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

Right.  Note that this is racy then.

Comment 12

[Benjamin Smedberg]

Yes ok, I think we're ok with racy here.

Comment 13

[Benjamin Smedberg]

https://hg.mozilla.org/integration/fx-team/rev/cc324226959b

Totally forgot about this one.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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