Option for timeout until an idle plugin-container is closed

RESOLVED FIXED in mozilla32

Status

()

Core
Plug-ins
P3
enhancement
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sworddragon2, Assigned: qeole)

Tracking

29 Branch
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=gfritzsche] [lang=c++] [good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Severity: normal → enhancement

Updated

3 years ago
Component: Untriaged → IPC
Product: Firefox → Core

Comment 1

3 years ago
(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?
Flags: needinfo?(sworddragon2)
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.
Status: UNCONFIRMED → NEW
Component: IPC → Plug-ins
Ever confirmed: true
Flags: needinfo?(sworddragon2)
Priority: -- → P3
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
Whiteboard: [mentor=gfritzsche] [lang=c++] [good first bug]
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
OS: Linux → All
Hardware: x86_64 → All
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

3 years ago
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!
Assignee: nobody → qeole

Comment 8

3 years ago
Oh, Great. I will try fix another bug =) Thanks []'s
(Assignee)

Comment 9

3 years ago
Created attachment 8420288 [details] [diff] [review]
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.
Attachment #8420288 - Flags: review?(georg.fritzsche)
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).
Attachment #8420288 - Flags: review?(georg.fritzsche) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8420563 [details] [diff] [review]
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?
Attachment #8420288 - Attachment is obsolete: true
Attachment #8420563 - Flags: feedback?(georg.fritzsche)
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.
Attachment #8420563 - Flags: feedback?(georg.fritzsche) → feedback+
(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?
(Assignee)

Comment 14

3 years ago
Created attachment 8420700 [details] [diff] [review]
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.
Attachment #8420563 - Attachment is obsolete: true
Attachment #8420700 - Flags: review?(georg.fritzsche)
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
Attachment #8420700 - Flags: review?(georg.fritzsche)
Attachment #8420700 - Flags: review?(benjamin)
Attachment #8420700 - Flags: feedback+
(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?
(Assignee)

Comment 17

3 years ago
Created attachment 8421066 [details] [diff] [review]
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.
Attachment #8420700 - Attachment is obsolete: true
Attachment #8420700 - Flags: review?(benjamin)
Attachment #8421066 - Flags: review?(benjamin)
Attachment #8421066 - Flags: feedback?(georg.fritzsche)
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.
Attachment #8421066 - Flags: feedback?(georg.fritzsche) → feedback+
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?
Attachment #8421066 - Flags: review?(benjamin) → review+
Flags: needinfo?(aklotz)
I don't think we have any such measurements at the moment.
Flags: needinfo?(aklotz)
(Assignee)

Comment 23

3 years ago
Created attachment 8423462 [details] [diff] [review]
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.
Attachment #8421066 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-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.
Flags: needinfo?(georg.fritzsche)
Keywords: checkin-needed
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
Flags: needinfo?(georg.fritzsche)
Thanks, Georg! I'll add this back to the checkin-needed queue.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/81bb7d281d8c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/81bb7d281d8c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Blocks: 1018200

Updated

3 years ago
Duplicate of this bug: 1066995
You need to log in before you can comment on or make changes to this bug.