1007490 - Option for timeout until an idle plugin-container is closed

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement, P3)

Description

[sworddragon2]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193813



Actual results:

If all instances for a plugin-container are in idle for 180 seconds the process will close.


Expected results:

180 seconds are a long time to clean up an idle process so it would be nice if the timeout could be controlled in about:config with an option.

Comment 1

[:Gijs (he/him)]

(In reply to sworddragon2 from comment #0)
> If all instances for a plugin-container are in idle for 180 seconds the
> process will close.

Out of curiosity, where did you get this number from?

Comment 2

[Benjamin Smedberg]

Currently hardcoded at http://hg.mozilla.org/mozilla-central/annotate/2a03b34c8953/dom/plugins/base/nsPluginHost.cpp#l730

Controlling this with a pref is acceptable. I do think we should make this shorter by default, perhaps as short as 30 seconds.

Comment 3

[Georg Fritzsche [:gfritzsche]]

This has two parts:
* add a pref, say "dom.ipc.plugins.unloadTimeoutSecs", with the default to browser/app/profile/firefox.js
* set up the timer at [1] with the pref value using Preferences::GetInt()

[1] http://hg.mozilla.org/mozilla-central/annotate/2a03b34c8953/dom/plugins/base/nsPluginHost.cpp#l730

Comment 4

[Georg Fritzsche [:gfritzsche]]

I wonder if we couldn't drop "dom.ipc.plugins.unloadASAP" [1] in favor of a 0 timeout while we're there.

[1] http://hg.mozilla.org/mozilla-central/annotate/2a03b34c8953/dom/plugins/base/nsPluginHost.cpp#l229

Comment 5

[Benjamin Smedberg]

Correction: please add the pref to modules/libpref/src/init/all.js instead of browser/app/profile/firefox.js. The latter is Firefox-only, while the former applies to all gecko-based apps.

Comment 6

[Rafael Vieira]

Hi! I could try to implement it. I'm new, how could I proceed?

Comment 7

[Georg Fritzsche [:gfritzsche]]

Hi Rafeal, from a mail exchange Qeole is already working on this, but thanks for checking here!

Comment 8

[Rafael Vieira]

Oh, Great. I will try fix another bug =) Thanks []'s

Comment 9

[Qeole]

Attachment: bug1007490.patch

Hello, I attach a patch for the bug (sorry Rafael!)
I set default value for timeout at 30 seconds (see comment #2), and suppressed the “dom.ipc.plugins.unloadASAP” option as suggested in comment #4.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8420288 [details] [diff] [review]
bug1007490.patch

Review of attachment 8420288 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good already! This should be good with the following things addressed.

I think we should have the preference name as a constant next to kPrefJavaMime here so we only define it explicitly once:
http://hg.mozilla.org/mozilla-central/annotate/a8602e656d86/dom/plugins/base/nsPluginHost.cpp#l126

Also, we should probably pass the second - optional - parameter to GetUInt() with our default value of 30 here (otherwise we default to 0 if the prefence isn't present).

Finally, we should use Preferences::GetUInt() instead of GetInt() here as we only want & expect unsigned values (sorry for misleading here).

Comment 11

[Qeole]

Attachment: bug1007490-v2.patch

Attached a revised patch based on comment #10.

I have two remarks/questions:

− Passing the second argument to GetUint() to define a default value AND declaring the option in file modules/libpref/src/init/all.js implies that we declare the 30 seconds-default value in two distinct files.
Is it necessary to give a second arg to GetUint()? Isn't it enough to declare the option with a default value in the all.js file?
(Or the opposite: is it necessary to declare the option in the all.js file? But if we don't, it does not show up in the about:config tab)

− On my computer (Kubuntu 13.10 64bits, in case it matters), if I create a profile, plugin-containers don't seem to close (at all) the first time I run this new profile.
If I close Firefox and open it again, then open new plugin-containers, they close after expected timeout.
This happens regardless of my modifications (on Firefox v31 and v32 at least). Is it a known/expected behaviour?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8420563 [details] [diff] [review]
bug1007490-v2.patch

Review of attachment 8420563 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me except the define mentioned below.

::: dom/plugins/base/nsPluginHost.cpp
@@ +127,5 @@
>  
> +// How long we wait before unloading a plugin-container once it
> +// comes to idle. Defaults to 30 seconds.
> +static const char *kPrefUnloadPluginTimeoutSecs = "dom.ipc.plugins.unloadTimeoutSecs";
> +#define DEFAULT_PLUGIN_UNLOADING_TIMEOUT 30

Please don't use defines - "static const uint32_t" works fine and avoids all the issues associated with them.

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to qeole from comment #11)
> Is it necessary to give a second arg to GetUint()? Isn't it enough to
> declare the option with a default value in the all.js file?

I think it's good to have a fallback value other than 0, all.js is not guaranteed to have the value and the pref could still be removed.

> − On my computer (Kubuntu 13.10 64bits, in case it matters), if I create a
> profile, plugin-containers don't seem to close (at all) the first time I run
> this new profile.

Hm, this is probably the background thumbnailing process - it re-uses our plugin-container to create thumbnails for about:newtab in a seperate process.
Does it start too if you don't open any pages that load plugins?

Comment 14

[Qeole]

Attachment: bug1007490-v3.patch

New patch in response to comment #12, with no "#defines".

In reply to comment #13, following comment #11:
> Hm, this is probably the background thumbnailing process - it re-uses our plugin-container to create
> thumbnails for about:newtab in a seperate process.
> Does it start too if you don't open any pages that load plugins?

No it does not start in that case.
If I
− don't open a tab with a plugin on first run of a new profile,
− then close Firefox,
− then open Firefox
− and open a tab that calls a plugin, there is no problem (plugin is unloaded as expected).

It just happens if I open a plugin during the first instance of Firefox that runs the new profile.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8420700 [details] [diff] [review]
bug1007490-v3.patch

Review of attachment 8420700 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good, only having a minor complaint below.
The commit message for the patch here should be in the format "Bug XXX - short description of the patch." and you should drop the notes for the different patch versions. Comments on changes from patch to patch version can be added in Bugzilla here.

Moving review to Benjamin who can actually sign this off.

::: dom/plugins/base/nsPluginHost.cpp
@@ +127,5 @@
>  
> +// How long we wait before unloading a plugin-container once it
> +// comes to idle. Defaults to 30 seconds.
> +static const char *kPrefUnloadPluginTimeoutSecs = "dom.ipc.plugins.unloadTimeoutSecs";
> +static const uint32_t defaultPluginUnloadingTimeout = 30;

You should prefix the constant with "k" per the coding style guide (and the existing style in the file):
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to qeole from comment #14)
> In reply to comment #13, following comment #11:
> > Hm, this is probably the background thumbnailing process - it re-uses our plugin-container to create
> > thumbnails for about:newtab in a seperate process.
> > Does it start too if you don't open any pages that load plugins?
> 
> No it does not start in that case.
> If I
> − don't open a tab with a plugin on first run of a new profile,
> − then close Firefox,
> − then open Firefox
> − and open a tab that calls a plugin, there is no problem (plugin is
> unloaded as expected).
> 
> It just happens if I open a plugin during the first instance of Firefox that
> runs the new profile.

Hm, that is strange. I assume that the patch here shouldn't affect that behavior either way - can you check that and file a new bug on this so we can investigate further?

Comment 17

[Qeole]

Attachment: bug1007490-v4.patch

I added a new patch with hopefully correct commit message and variable naming. Also, I split a line which grew very long.

In response to comment #14:
> Hm, that is strange. I assume that the patch here shouldn't affect that behavior either way - can you
> check that and file a new bug on this so we can investigate further?

My patch does not affect this behavior (I tested on an Aurora version I haven't compiled). I would like to try to first reproduce it on Windows (or maybe at least on another machine), and then I'll file a bug.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8421066 [details] [diff] [review]
bug1007490-v4.patch

Review of attachment 8421066 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: dom/plugins/base/nsPluginHost.cpp
@@ +125,5 @@
>  static const char *kPrefDisableFullPage = "plugin.disable_full_page_plugin_for_types";
>  static const char *kPrefJavaMIME = "plugin.java.mime";
>  
> +// How long we wait before unloading a plugin-container once it
> +// comes to idle. Defaults to 30 seconds.

Nitpicking: "becomes idle" or just "an idle plugin process". Should be totally fine to fix this after review.

Comment 19

[John Schoenick [:johns]]

How does doing this jive with the concerns that launching plugin-container is a huge source of jank (e.g. Bug 998863)?

Comment 20

[Georg Fritzsche [:gfritzsche]]

Good question, maybe we need to:
* land this with the pref set to the current timeout and
* land telemetry on termination frequencies to judge the impact

Comment 21

[Benjamin Smedberg]

Comment on attachment 8421066 [details] [diff] [review]
bug1007490-v4.patch

So the thing we're balancing here is: we know that some users experience serious leaks with the Flash plugin, and so we want to kill it regularly so that we end up with a clean one. But we also have the jank associated with starting up a new one.

Because I have a hunch that the shorter timeout will turn out better, I'm going to mark r+ and I think this should land as-is on nightly. We should run an experiment to measure some comparative numbers on beta. 

Aklotz, do you know if we already measure the plugin launch time in a telemetry metric or expose it to script in a way that we could build an experiment on?

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

I don't think we have any such measurements at the moment.

Comment 23

[Qeole]

Attachment: bug1007490-v5.patch

I attach the final version of the patch, with minor modification in comments from comment #18.

For later readers, telemetry support regarding the discussion above is the object of bug 1011136.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Can we get this pushed to tryserver (or post a link to a tryserver run if it's already been pushed) so we can make sure nothing blows up when it gets checked in? Georg should be able to help with that if you're unsure how to do it.
Once that happens you can re-add the checkin-needed keyword.

Comment 25

[Georg Fritzsche [:gfritzsche]]

I didn't expect any issues with this patch, but here's a try push (running most of our automated tests like they would run after an actual check-in):
https://tbpl.mozilla.org/?tree=Try&rev=fcd0716a02dd

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Thanks, Georg! I'll add this back to the checkin-needed queue.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81bb7d281d8c

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81bb7d281d8c