Josh Aas Assignee
	Description • 15 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.

	Josh Aas Assignee
	Comment 1 • 15 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 15 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 15 years ago

I thought we tore down plugin instances when going into bfcache.  At least last I checked....

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 4 • 15 years ago

Hmm. You're right, of course.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 15 years ago

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.

	Josh Aas Assignee
	Comment 7 • 15 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.

	Josh Aas Assignee
	Comment 8 • 15 years ago

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/61a70b55686d

	Josh Aas Assignee
	Comment 9 • 15 years ago

I filed bug 501485 about finding a new unloading strategy.

	Johnny Stenback (:jst)
	Comment 11 • 15 years ago

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

	Daniel Veditz [:dveditz]
	Comment 12 • 15 years ago

Would this have fixed bug 493601?

	Johnny Stenback (:jst)
	Comment 13 • 15 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 14 • 15 years ago

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?

	Johnny Stenback (:jst)
	Comment 15 • 15 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 16 • 15 years ago

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

	Serge Gautherie (:sgautherie)
	Comment 17 • 15 years ago

(In reply to comment #16)
> This made reftest and mochitest-plain leaks! Firefox and SeaMonkey!

On MacOSX (only).

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 18 • 15 years ago

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#

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 19 • 15 years ago

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.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 20 • 15 years ago

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

	Benjamin Smedberg
	Comment 21 • 15 years ago

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.

	Jesse Ruderman
	Comment 22 • 15 years ago

I think we already don't scan plugins at startup (bug 506472?).

	Johnny Stenback (:jst)
	Comment 23 • 15 years ago

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

	Johnny Stenback (:jst)
	Comment 24 • 15 years ago

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

	Jo Hermans
	Comment 25 • 15 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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 26 • 15 years ago

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

	Johnny Stenback (:jst)
	Comment 27 • 15 years ago

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.

	Serge Gautherie (:sgautherie)
	Comment 28 • 15 years ago

2 weeks of orange is enough :-(

	Reed Loden [:reed]
	Comment 29 • 15 years ago

Comment on attachment 410716 [details] [diff] [review]
(Bv1-SM) Add leak threshold ftb [Superseded by bug 526277]

Just go land bug 526277.

	Serge Gautherie (:sgautherie)
	Comment 30 • 15 years ago

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