Last Comment Bug 501485 - investigate new NPAPI plugin unloading strategy
: investigate new NPAPI plugin unloading strategy
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal with 6 votes (vote)
: mozilla13
Assigned To: Josh Aas
:
Mentors:
: 535323 538826 650948 656326 657753 662028 673356 675471 688496 688530 693820 701951 711825 720235 (view as bug list)
Depends on: 90268 661260
Blocks: 563227 675471
  Show dependency treegraph
 
Reported: 2009-06-30 14:28 PDT by Josh Aas
Modified: 2012-02-16 11:07 PST (History)
47 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
delayed unload strategy, v1.0 (4.51 KB, patch)
2012-02-10 08:38 PST, Josh Aas
no flags Details | Diff | Review
delayed unload strategy, v1.1 (4.52 KB, patch)
2012-02-10 12:15 PST, Josh Aas
no flags Details | Diff | Review
delayed unload strategy, v2.0 (14.76 KB, patch)
2012-02-10 14:55 PST, Josh Aas
no flags Details | Diff | Review
delayed unload strategy, v2.1 (15.40 KB, patch)
2012-02-12 13:53 PST, Josh Aas
no flags Details | Diff | Review
delayed unload strategy, v2.2 (14.99 KB, patch)
2012-02-14 08:03 PST, Josh Aas
benjamin: review+
Details | Diff | Review

Description Josh Aas 2009-06-30 14:28:27 PDT
Bug 500925 changed our default behavior to never unloading NPAPI plugins once they are loaded. We should investigate a new strategy for unloading plugins that does not regress the following scenario:

"1 window open, 1 tab: Open page X which has a single Flash instance. Go to page Y, which also has a single Flash instance."

In this scenario, if we unload plugins when their last instance is destroyed then we'll completely unload the Flash plugin when we unload page X and load the whole plugin again when we load page Y. We don't want to do that, we want to simply destroy the first instance and then load another instance, keeping the plugin alive the whole time.

Note that the bfcache does not keep plugin instances alive.

A potential solution is to see whether or not a plugin can be unloaded some number of minutes after its last instance is destroyed, killing that timer if another instance is started.
Comment 1 Ted Mielczarek [:ted.mielczarek] 2009-07-02 05:00:32 PDT
There's also a memory pressure notification that stuart added, which would be a good time to free any unused plugins.
Comment 2 Jo Hermans 2009-09-05 13:13:09 PDT
Firefox 3.6 is never unloading the plugins anymore. While this is a very nice speech improvement for websites like Youtube and such, wouldn't people start reporting this as a memory leak (the behavior change is not publicly known yet)? Especially Flash can consume quite a lot of memory.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2009-12-29 13:11:47 PST
*** Bug 535323 has been marked as a duplicate of this bug. ***
Comment 4 u88484 2010-01-14 15:55:22 PST
In the duplicate bug that I filed (see comment 3), I stated the following:

Results after watching a video:

Just opening youtube.com was around 6,000 K
Closing youtube.com without watching a video resulted in releasing 100 K
Watching the video memory went to 39,000 K
closing the youtube tab and memory went down to 13,000 K

This is going to appear as a memory leak to any end user.  So users that complain of memory being held by plugins that might be used
again in same session should add 'plugins.unloadASAP'?  I just foresee a lot of
complaints from users complaining about the memory being held.

I also didn't get an answer about this, "I couldn't find in the bug you linked to the actual time limit until the
process is killed.  There was talk of 10 minutes but that doesn't seem like it
was the final answer."  Bug 500925 is the mentioned bug
Comment 5 Kevin Brosnan [:kbrosnan] 2011-04-18 15:36:26 PDT
*** Bug 650948 has been marked as a duplicate of this bug. ***
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-11 10:00:59 PDT
*** Bug 656326 has been marked as a duplicate of this bug. ***
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-05-18 05:58:17 PDT
*** Bug 657753 has been marked as a duplicate of this bug. ***
Comment 8 Wayne Mery (:wsmwk, NI for questions) 2011-05-28 03:28:04 PDT
*** Bug 538826 has been marked as a duplicate of this bug. ***
Comment 9 Jo Hermans 2011-06-04 06:19:49 PDT
*** Bug 662028 has been marked as a duplicate of this bug. ***
Comment 10 dindog 2011-06-04 09:29:59 PDT
(In reply to comment #0)
> A potential solution is to see whether or not a plugin can be unloaded some
> number of minutes after its last instance is destroyed, killing that timer
> if another instance is started.

Here is a solution, 
1.we differ the setting of each plug-in, and have a gerneral killing time keep on record the time since its last used, says the threshold 2mins.

2. when first time a plug-in meets the time, pop-up asking user "whether auto kill the instance when it's unused next time?" as we ask them whether keep a password.

So user can kill the less using plug-ins and keep the active ones (like Flash)
Comment 11 Robert Longson 2011-07-21 23:21:12 PDT
*** Bug 673356 has been marked as a duplicate of this bug. ***
Comment 12 Himanshu 2011-07-21 23:44:35 PDT
The processes after closed takes a long time to release memory.
Comment 13 Danial Horton 2011-07-21 23:53:24 PDT
Firefox should terminate the process when you disable the plugin in the AOM.
Comment 14 Himanshu 2011-07-21 23:55:10 PDT
I mean to say that if I close any flash website, then the plugin-container still consumes memory and is not released immediately.

I don't want to disable the plugin.
Comment 15 Danial Horton 2011-07-22 00:14:22 PDT
I wasn't responding to you.

This bug has nothing to do with flash or flash plugin container memory usage.
Comment 16 Himanshu 2011-07-22 04:51:53 PDT
Then why my bug Bug 673356 has been marked as a duplicate of this bug.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-22 07:15:33 PDT
Comment 15 is just wrong, as far as I can tell. This bug is precisely about deciding when to kill plugin-container when there are no plugin instances around.

Note that "immediately" would actually suck a lot on page transitions, so is probably the wrong answer.  Hence the "investigate" bit.
Comment 18 Danial Horton 2011-07-22 15:33:23 PDT
Its not about the container taking a long time to release memory, thats a flash bug itself.
Comment 19 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-08-23 13:55:01 PDT
*** Bug 675471 has been marked as a duplicate of this bug. ***
Comment 20 Robert Longson 2011-09-22 11:23:31 PDT
*** Bug 688530 has been marked as a duplicate of this bug. ***
Comment 21 Angelo Borsotti 2011-09-22 11:51:53 PDT
The workaround I am using is to close firefox when I see too many plugin-container.exe processes running, and open it again. This cannot be, naturally,
the final solution. The browser cannot start new processes and never terminate
them when they are no longer needed. So, if it is really time consuming to
start them when need occurs, let's keep them alive for some time when they are no longer needed (in case the user would open shortly another page requiring them).
Comment 22 Matthias Versen [:Matti] 2011-09-23 16:06:53 PDT
*** Bug 688496 has been marked as a duplicate of this bug. ***
Comment 23 Kevin Brosnan [:kbrosnan] 2011-10-11 14:38:21 PDT
*** Bug 693820 has been marked as a duplicate of this bug. ***
Comment 24 Jo Hermans 2011-11-11 17:20:50 PST
*** Bug 701951 has been marked as a duplicate of this bug. ***
Comment 25 Benjamin Smedberg [:bsmedberg] 2011-12-01 09:51:15 PST
I'm going to take this, but I'll want plugin-as-content to land first otherwise the merge will be difficult.
Comment 26 Danial Horton 2011-12-01 10:03:32 PST
i can't seem to find a bug for that, is there one?
Comment 27 Benjamin Smedberg [:bsmedberg] 2011-12-01 10:05:48 PST
bug 90268
Comment 28 Robert Longson 2011-12-18 06:09:13 PST
*** Bug 711825 has been marked as a duplicate of this bug. ***
Comment 29 Jo Hermans 2012-01-22 11:46:31 PST
*** Bug 720235 has been marked as a duplicate of this bug. ***
Comment 30 sworddragon2 2012-01-22 13:28:46 PST
I have opened a ticket too but it was marked as a duplicate (I read the first post of this ticket before but it hasn't described the closing of plugin-container (was there already plugin-container at this date?)). I would like to have something like dom.ipc.plugins.idleTimeoutSecs which defines the time in seconds after plugin-container will be closed if no plugin used it.
Comment 31 Danial Horton 2012-01-22 14:26:42 PST
the plugin container loads the plugin, not the other way around ;p

for npapi unloading to work, content and the container need to notify each other of whats going on is the gist i get from this, or else you could terminate things like buffered video's simply because theres no activity after the video buffers completely.
Comment 32 sworddragon2 2012-01-22 15:16:43 PST
>the plugin container loads the plugin, not the other way around ;p

I know this but my post seems to look a little confusing about this :P

>for npapi unloading to work, content and the container need to notify each other of whats going on is the gist i get from this, or else you could terminate things like buffered video's simply because theres no activity after the video buffers completely.

Normally after a video is buffered it is playing and the flash plugin is processing it. Even if I pause a video there is a moderate cpu usage. If I kill the plugin-container the video says that the flash plugin has crashed.

Doesn't Firefox already know with this way that the flash plugin is already working? Otherwise how does Firefox know that the flash plugin has crashed? Just by watching if plugin-container is running?
Comment 33 Josh Aas 2012-02-10 07:26:25 PST
Benjamin - do you mind if I take this bug or are you planning to work on it soon?
Comment 34 Josh Aas 2012-02-10 08:38:00 PST
Created attachment 596059 [details] [diff] [review]
delayed unload strategy, v1.0

I wrote this pretty quickly but it seems to work alright. I'll need to think about this and do some more testing to be confident, but it'd be good to get review and feedback now.
Comment 35 Josh Aas 2012-02-10 12:15:35 PST
Created attachment 596140 [details] [diff] [review]
delayed unload strategy, v1.1

Fixes a minor bug that probably had no real consequences (incorrect return value from timer Notify callback).
Comment 36 Benjamin Smedberg [:bsmedberg] 2012-02-10 12:28:04 PST
Have you looked at the existing bugs related to unloading plugins? My main worry for this bug was that we'd cause those other bugs to appear by default. See bug 661260, especially comment 1, and also some of the commentary in bug 520639. I suspect that we should attempt to unload only OOPP plugins, and never in-process plugins.
Comment 37 Josh Aas 2012-02-10 13:31:08 PST
I think we should entirely remove the ability to unload any plugin from a non-plugin process. I don't even want to offer that option with a pref any more. I'll do that in an updated patch, and the unloadASAP pref will only apply to OOPP.
Comment 38 Josh Aas 2012-02-10 14:55:28 PST
Created attachment 596181 [details] [diff] [review]
delayed unload strategy, v2.0

Updated to never try to unload in-process plugin libraries. Only tested on Mac OS X so far.
Comment 39 Josh Aas 2012-02-12 13:53:03 PST
Created attachment 596522 [details] [diff] [review]
delayed unload strategy, v2.1

Cancel the timer (if there is one) when a new instance is created.
Comment 40 Josh Aas 2012-02-12 17:39:37 PST
We should probably fix bug 661260 before making plugin unloading much more likely for the average user.
Comment 41 Josh Aas 2012-02-13 06:38:55 PST
try server run:

https://tbpl.mozilla.org/?tree=Try&rev=bfb36b156244
Comment 42 Josh Aas 2012-02-14 08:03:05 PST
Created attachment 597036 [details] [diff] [review]
delayed unload strategy, v2.2

Updated to current trunk.
Comment 43 Josh Aas 2012-02-14 11:00:24 PST
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/27a6f4156f0a
Comment 44 Marco Bonardo [::mak] 2012-02-15 09:01:25 PST
https://hg.mozilla.org/mozilla-central/rev/27a6f4156f0a
Comment 45 IU 2012-02-16 08:10:44 PST
I don't see 'dom.ipc.plugins.unloadASAP' anywhere in about:config.  Is it something that has to be added manually or did something get missed?
Comment 46 dindog 2012-02-16 08:19:19 PST
(In reply to IU from comment #45)
> I don't see 'dom.ipc.plugins.unloadASAP' anywhere in about:config.  Is it
> something that has to be added manually or did something get missed?

I believe you have to create it by yourself, like old plugins.unloadASAP.

Firefox will unload plugin after it's unused 3 minute by default now.
Comment 47 Daniel Cater 2012-02-16 11:06:14 PST
Does this respond to memory pressure events (i.e. immediately unload on memory pressure)?
Comment 48 Benjamin Smedberg [:bsmedberg] 2012-02-16 11:07:05 PST
No.

Note You need to log in before you can comment on or make changes to this bug.