544345 - Utilize IPDL hang detection in PluginModuleParent and set timeout based on pref

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

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

How does "dom.ipc.plugins.timeoutSecs" sound?  I'd also like to default its value to that of the JS slow-script timeout based on the assumption that that value was chosen after discussion/experience/UX considerations.

Comment 1

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

Attachment: Kill plugins if they exceed the hang timeout.

This isn't shippable code, just a stepping stone to bug 544095.

Comment 2

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

Attachment: Mochitest of hang killing.

Requesting review because I don't feel 100% comfortable with mochitests.  Don't want nondeterministic crud.

Comment 3

[Benjamin Smedberg]

Comment on attachment 425328 [details] [diff] [review]
Kill plugins if they exceed the hang timeout.

>diff --git a/dom/plugins/PluginModuleParent.cpp b/dom/plugins/PluginModuleParent.cpp

>+static const char* kTimeoutPref = "dom.ipc.plugins.timeoutSecs";

This could be `static char const *const kTimeoutPref`, but it would be best as `static const char kTimeoutPref[]`;

>+    nsContentUtils::RegisterPrefCallback(kTimeoutPref, TimeoutChanged, parent);
>+    TimeoutChanged(kTimeoutPref, parent);

The contentutils pref callback perpetuates the awful nsIPref callback design. But given that the only other option is an XPCOM object, I guess this is expedient. Why can't this be in the PluginModuleParent constructor, though, so that Register/Unregister are a matched pair, easy to find?

r=me with those changes

Comment 4

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

(In reply to comment #3)
> >+    nsContentUtils::RegisterPrefCallback(kTimeoutPref, TimeoutChanged, parent);
> >+    TimeoutChanged(kTimeoutPref, parent);
> 
> The contentutils pref callback perpetuates the awful nsIPref callback design.
> But given that the only other option is an XPCOM object, I guess this is
> expedient. Why can't this be in the PluginModuleParent constructor, though, so
> that Register/Unregister are a matched pair, easy to find?
> 

Alas, but the RPCChannel isn't usable until after toplevelActor.Open() has been called.

Comment 5

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

(In reply to comment #4)
> (In reply to comment #3)
> > >+    nsContentUtils::RegisterPrefCallback(kTimeoutPref, TimeoutChanged, parent);
> > >+    TimeoutChanged(kTimeoutPref, parent);
> > 
> > The contentutils pref callback perpetuates the awful nsIPref callback design.
> > But given that the only other option is an XPCOM object, I guess this is
> > expedient. Why can't this be in the PluginModuleParent constructor, though, so
> > that Register/Unregister are a matched pair, easy to find?
> > 
> 
> Alas, but the RPCChannel isn't usable until after toplevelActor.Open() has been
> called.

Ignore that.  I'll Register() in the ctor, then do the initial TimeoutChanged() in LoadModule().

Comment 6

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

Attachment: Mochitest, updated from e10s latest

Comment 7

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

Pushed http://hg.mozilla.org/projects/electrolysis/rev/4bbb628d71c8
Pushed http://hg.mozilla.org/projects/electrolysis/rev/0f8a10cc7ca3

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/391018912b6c
http://hg.mozilla.org/mozilla-central/rev/19d9c18c2a7a
http://hg.mozilla.org/mozilla-central/rev/d991b15840a8

Comment 9

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/3478c15481bd

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 11

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #0)
> How does "dom.ipc.plugins.timeoutSecs" sound?  I'd also like to default its
> value to that of the JS slow-script timeout based on the assumption that that
> value was chosen after discussion/experience/UX considerations.

The 10 second wall-time default wasn't based on much more than gut check and then seeing if we got too many false positives.

We do get false positives, even (until recent post-3.6 changes, I think) for our session restore chrome code.

I'm way late here, but the obvious difference between the slow script watchdog timeout and this one is this one is fatal -- the slow script dialog lets the user continue or stop. That overrides any default timeout value consideration!

/be

Comment 13

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

As comment 1 noted, these patches were a stepping stone to what I thought bug 544095 was to be, a "Should this plugin be killed?" dialog.  But those patches were ui-r-'d, so the assumptions of comment 0 were invalid.