The default bug view has changed. See this FAQ.

don't unload plugins as soon as possible by default

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Plug-ins
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking1.9.1 .6+, status1.9.1 .6-fixed)

Details

(Whiteboard: [crashkill][crashkill-fix])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Right now we unload plugins as soon as their last instance goes away, then load them again when a new instance starts. This, for example, means that if you go from one page with a Flash instance to another page with a Flash instance, the plugin is shut down and unloaded upon leaving the first page and restarted when loading the second. I believe this is a big part of why people report so much plugin i/o and loading/unloading activity, and why we have problems like bug 397053 while other browser don't.

Other sources of load/unload activity at unexpected times are upon launch, where we potentially scan, and when loading/unloading happens on timers for various reasons.

This bug is about improving our unloading behavior so that we don't load/unload so often.
(Assignee)

Comment 1

8 years ago
Created attachment 385592 [details] [diff] [review]
fix v1.0

This patch changes our default behavior to leaving plugins loaded once they are used for the first time. I've added a pref, "plugins.unloadASAP', to revert to our current behavior of unloading plugins as soon as they have no active instances.

This is probably a good start, memory-pressured platforms such as mobile may want to set "plugins.unloadASAP" to "true".

Another thing we could do is unload plugins only after they have had no instances for a certain period of time.
Attachment #385592 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Blocks: 492621, 425490
(In reply to comment #0)
> Right now we unload plugins as soon as their last instance goes away, then load
> them again when a new instance starts. This, for example, means that if you go
> from one page with a Flash instance to another page with a Flash instance, the
> plugin is shut down and unloaded upon leaving the first page and restarted when
> loading the second.

This wouldn't happen every time, because if the plugin's page lands in the bfcache then we'd keep the plugin loaded. But I agree that this is definitely something we should fix.

I think the best approach would be to unload a plugin after it hasn't been used for, say, ten minutes.
I thought we tore down plugin instances when going into bfcache.  At least last I checked....
Hmm. You're right, of course.
(Assignee)

Comment 5

8 years ago
Created attachment 385907 [details] [diff] [review]
fix v1.1
Attachment #385592 - Attachment is obsolete: true
Attachment #385907 - Flags: review?(bzbarsky)
Attachment #385592 - Flags: review?(bzbarsky)
Comment on attachment 385907 [details] [diff] [review]
fix v1.1

I assume the only difference from the first patch is that you removed the unused boolean arg?

If so, this looks ok, but jst should look at this too.  And please do file a bug on unloading at some point; I do think we want to do this.
Attachment #385907 - Flags: superreview?(jst)
Attachment #385907 - Flags: review?(bzbarsky)
Attachment #385907 - Flags: review+
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> (From update of attachment 385907 [details] [diff] [review])
> I assume the only difference from the first patch is that you removed the
> unused boolean arg?

Right.

> If so, this looks ok, but jst should look at this too.  And please do file a
> bug on unloading at some point; I do think we want to do this.

Yup.

Updated

8 years ago
Attachment #385907 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 8

8 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/61a70b55686d
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

8 years ago
I filed bug 501485 about finding a new unloading strategy.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 425490
blocking1.9.1: --- → ?
Created attachment 407617 [details] [diff] [review]
Fix for 1.9.1

This is the same thing as the fix we took on the trunk but ported to 1.9.1. The hope is that this might fix bug 501429, and we should take this for 3.5.5 to find out (no other way to find out it seems, nobody can seem to reproduce this crash).
Attachment #407617 - Flags: review?(joshmoz)
Would this have fixed bug 493601?
blocking1.9.1: ? → .5+
status1.9.1: --- → wanted
(Assignee)

Updated

8 years ago
Attachment #407617 - Flags: review?(joshmoz) → review+

Updated

8 years ago
Attachment #407617 - Flags: approval1.9.1.5?
I don't think so, no. This bug is about us never unloading the actual DLL (which is the behavior we want), but we still destroy plugin instances. I think bug 493601 really talks about plugin instance destruction rather than plugin library unloading.
Comment on attachment 407617 [details] [diff] [review]
Fix for 1.9.1

Approved for 1.9.1.5, a=dveditz for release-drivers

Shouldn't we have a default pref value in all.js for the new pref? Or do you mean  plugins.unloadASAP to be not only hidden but stealth?
Attachment #407617 - Flags: approval1.9.1.5? → approval1.9.1.5+
Landed on 1.9.1. dveditz, the pref is stealth everywhere else as well...

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0be7c0bfe443
status1.9.1: wanted → .5-fixed
(In reply to comment #15)
> Landed on 1.9.1.

This made reftest and mochitest-plain leaks! Firefox and SeaMonkey!
Please back out or fix...
Target Milestone: --- → mozilla1.9.2a1
(In reply to comment #16)
> This made reftest and mochitest-plain leaks! Firefox and SeaMonkey!

On MacOSX (only).
Also implicated in a Tp4 RSS and Tp4 Private Bytes increase on Linux, see dev-tree-management:

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/ce2a4e29550f6673#

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/f1305e1d7f268005#
And ~10% memory regressions on Windows as well:

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/66da50ef00185968#

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/0b152e23d559b005#

From the title of this patch, I don't know if this was unexpected (I mean, we're unloading later). It's possible that this is an artifact of how we measure memory with Talos, but we need to make sure we're not actually regressing memory usage in a way that users will feel without first discussing the pros and cons.
We should probably unload eagerly after initial-startup plugin scan.  I suspect that that would give us back the memory here, since I don't think our test actually trigger plugin load, and would keep users from having all the plugins on their system loaded for 10 mins or whatever.

(We should maybe stop scanning plugins on startup too, for perf reasons, but that's another bug that I can't remember the number of right now.)
Tp4 does load Flash, I'm told.

We shouldn't be scanning plugins until/unless we have to handle an unknown MIME type and we haven't got saved plugin information from a previous run.

Comment 22

8 years ago
I think we already don't scan plugins at startup (bug 506472?).
(In reply to comment #16)
> From the title of this patch, I don't know if this was unexpected (I mean,
> we're unloading later). It's possible that this is an artifact of how we
> measure memory with Talos, but we need to make sure we're not actually
> regressing memory usage in a way that users will feel without first discussing
> the pros and cons.

I don't know what the numbers were like when this change landed on the trunk, but they should've been the same. We're in fact *not* unloading these plugin libraries later (until the OS unloads them on application exit), which is why we see the increase in memory usage when measured as our graphs show. I believe there's a bug on doing something like unloading plugins after n minutes of the last plugin instance being destroyed, but I don't have that bug number handy, and I don't expect that to ever be backported to 1.9.1, or even 1.9.2. And just to be clear here, once a user leaves a page that used plugin x, the memory that plugin x allocated, and the memory we allocated for the plugin instance does get freed (modulo actual memory leaks) like before, we just don't unload the library since we believe we're likely to use it again soon, which is the common case by a long stretch.
(In reply to comment #20)
> We should probably unload eagerly after initial-startup plugin scan.  I suspect
> that that would give us back the memory here, since I don't think our test
> actually trigger plugin load, and would keep users from having all the plugins
> on their system loaded for 10 mins or whatever.

AFAIK, the only system where we actually load plugins on startup is on unix systems, and there we only load any plugins that we find and haven't seen before (per pluginreg.dat in the profile), so for regular users we basically never load all plugins, unless they visit pages that acutally use all their plugins. On other systems we extract the info we need about plugins w/o actually loading the plugin library, so no unloading ever needed due to startup.

> (We should maybe stop scanning plugins on startup too, for perf reasons, but
> that's another bug that I can't remember the number of right now.)

Sure.

Comment 25

8 years ago
(In reply to comment #23) 
> I don't know what the numbers were like when this change landed on the trunk,
> but they should've been the same. We're in fact *not* unloading these plugin
> libraries later (until the OS unloads them on application exit), which is why
> we see the increase in memory usage when measured as our graphs show. I believe
> there's a bug on doing something like unloading plugins after n minutes of the
> last plugin instance being destroyed, but I don't have that bug number handy,

bug 501485

> and I don't expect that to ever be backported to 1.9.1, or even 1.9.2. And just
> to be clear here, once a user leaves a page that used plugin x, the memory that
> plugin x allocated, and the memory we allocated for the plugin instance does
> get freed (modulo actual memory leaks) like before, we just don't unload the
> library since we believe we're likely to use it again soon, which is the common
> case by a long stretch.

Updated

8 years ago
Blocks: 501429

Updated

8 years ago
Whiteboard: [crashkill][crashkill-fix]
Comment on attachment 407617 [details] [diff] [review]
Fix for 1.9.1

>-void nsPluginTag::TryUnloadPlugin(PRBool aForceShutdown)
>+void nsPluginTag::TryUnloadPlugin()
> {
>   PRBool isXPCOM = PR_FALSE;
>   if (!(mFlags & NS_PLUGIN_FLAG_NPAPI))
>     isXPCOM = PR_TRUE;
> 
>-  if (isXPCOM && !aForceShutdown) return;
>+  if (isXPCOM) return;

jst, when this parameter was removed on trunk, it was unused. On the 1.9.1 branch, though, it was actually used so that we knew when we could release (amongst other XPCOM plugins) Java. I think this is responsible for the leaks found in <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5-Unittest/1257211455.1257214299.2063.gz>.
Yes, I have a half baked patch to deal with the leaks, just haven't had time to make sure it's exactly what I want here.
Created attachment 410716 [details] [diff] [review]
(Bv1-SM) Add leak threshold ftb [Superseded by bug 526277]

2 weeks of orange is enough :-(
Attachment #410716 - Flags: review?(kairo)
Comment on attachment 410716 [details] [diff] [review]
(Bv1-SM) Add leak threshold ftb [Superseded by bug 526277]

Just go land bug 526277.
Attachment #410716 - Flags: review?(kairo)
Attachment #410716 - Attachment is obsolete: true
Depends on: 526277, 521599
(In reply to comment #29)
> (From update of attachment 410716 [details] [diff] [review])
> Just go land bug 526277.

Thanks for the hint!
(How are we supposed to find out about such bugs not linked from anywhere? :-<)
Attachment #410716 - Attachment description: (Bv1-SM) Add leak threshold ftb → (Bv1-SM) Add leak threshold ftb [Superseded by bug 526277]
You need to log in before you can comment on or make changes to this bug.