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.

(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?

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.

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

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

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.

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

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

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

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

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

(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?

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

(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?

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

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

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 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?

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

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.

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.

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

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