Last Comment Bug 478823 - navigator.plugins.refresh() leaks memory, gives problems with MS Silverlight detection script.
: navigator.plugins.refresh() leaks memory, gives problems with MS Silverlight ...
Status: VERIFIED FIXED
: mlk, testcase
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Windows XP
: -- critical with 5 votes (vote)
: ---
Assigned To: Scott Greenlay [:sgreenlay]
:
Mentors:
: 395378 470481 (view as bug list)
Depends on: 596078
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-16 21:16 PST by simeon.malchev
Modified: 2011-03-02 13:42 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta8+
.14-fixed


Attachments
Screenshot of my Windows Task Manager (104.09 KB, image/jpeg)
2009-02-16 21:18 PST, simeon.malchev
no flags Details
Memory leak - second test (106.63 KB, image/jpeg)
2009-02-18 21:19 PST, simeon.malchev
no flags Details
Leaking script (7.91 KB, text/plain)
2009-07-24 12:35 PDT, u348128
no flags Details
Fully reduced test-case (226 bytes, text/html)
2009-07-24 13:53 PDT, u348128
no flags Details
fix v1.0 (7.37 KB, patch)
2010-02-01 23:15 PST, Josh Aas
no flags Details | Diff | Review
Plugin Leak Fix (v1.0) (6.65 KB, patch)
2010-09-22 14:33 PDT, Scott Greenlay [:sgreenlay]
no flags Details | Diff | Review
ignore invalid dll files altogether, v1.0 (1.89 KB, patch)
2010-09-28 00:36 PDT, Josh Aas
no flags Details | Diff | Review
ignore invalid dll files altogether, v1.1 (2.10 KB, patch)
2010-09-28 00:58 PDT, Josh Aas
no flags Details | Diff | Review
ignore invalid dll files altogether, v1.2 (2.10 KB, patch)
2010-09-28 15:26 PDT, Scott Greenlay [:sgreenlay]
jaas: review+
jst: review+
Details | Diff | Review
ignore invalid dll files altogether (1.9.2), v1.0 (2.15 KB, patch)
2010-10-20 14:06 PDT, Scott Greenlay [:sgreenlay]
no flags Details | Diff | Review
ignore invalid dll files altogether (1.9.2), v1.1 (2.24 KB, patch)
2010-10-22 15:51 PDT, Scott Greenlay [:sgreenlay]
jaas: review+
brandon: approval1.9.2.14+
Details | Diff | Review

Description simeon.malchev 2009-02-16 21:16:19 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6

Hi,

I left my firefox open over the weekend with the following 5 urls open into tabs into it:

1. http://www.sqlinform.com/
2. http://technet.microsoft.com/en-au/library/cc917718.aspx
3. http://msdn.microsoft.com/en-us/library/ms187745.aspx
4. http://www.coderanch.com/t/69293/BEA-Weblogic/weblogic-startup
5. http://www.applettalk.com/sqlserver-2000-driver-for-jdbc-behaviour-on-sql-server-resta-vt8097.html

On Monday morning according to Windows Task Manager, memory usage: 1.4GB !

Firefox version: 3.0.6

From my perspective, this is a quite severe memory leak.

Thanks,
Simeon


Reproducible: Always

Steps to Reproduce:
1. Open the given above 5 urls (one or more then one of them is causing the problem, however I'm not sure which one)
2. Wait for a day or two...
3. Check the amount of consumed memory by firefox.



I'm not sure what is the severity of such issue. It could be Normal, but it could be Critical, as well.
Comment 1 simeon.malchev 2009-02-16 21:18:31 PST
Created attachment 362679 [details]
Screenshot of my Windows Task Manager
Comment 2 WADA 2009-02-16 23:22:30 PST
> I left my firefox open over the weekend with the following 5 urls open

Suspend(&resume) or Sleep is executed during the week end?
When problem occurred, termination of Fx was successful?

New issue with Fx 3.0.6? (No such problem with Fx 3.0.5, 3.0.4,...?)
Can you see such phenomenon with -safe-mode of Fx? (add-on is disabled)
Can you see such phenomenon with new profile? (no add-on is installed)
Comment 3 simeon.malchev 2009-02-18 21:18:33 PST
I've started firefox yes: 
"C:\Program Files\Mozilla Firefox\firefox.exe" -safe-mode

From the dialog window that I got I choose, the first option disable all addons, and then I click the restart button (the most left button).

I openened the mentioned above 5 urls and left the firefox open for 16 hours overnight. Memory consumption was 384MB at the morning, see the attachment. Termination of Fx was successful on the morning. Suspend(&resume) or Sleep is not executed overnight. Unfortunately I won't be able to test for the other Fx versions.
Comment 4 simeon.malchev 2009-02-18 21:19:39 PST
Created attachment 363055 [details]
Memory leak - second test
Comment 5 WADA 2009-02-19 01:05:44 PST
> Unfortunately I won't be able to test for the other Fx versions.

Sorry for my unclear question. My question was;
  Have you experienced such large virtual memory size of Fx in the past,
  before upgrading to current Fx 3.0.6? Or Fx 3.0.6 is first Fx you use?
Comment 6 simeon.malchev 2009-02-19 12:31:44 PST
Hi Wada, I'm using Fx for about 2 years. I've never expirienced similar problem before, however may be I've never beeing opening exactly these URLs before. Still, my professional opinion is that this problem is something relatively new, that has happened in Fx 3.0.6 or may be in 0.5, as well (I can't be completely sure which has been the first version with the problem)
Comment 7 WADA 2009-03-07 21:04:36 PST
(In reply to comment #3)
> I've started firefox yes: 
> "C:\Program Files\Mozilla Firefox\firefox.exe" -safe-mode
>(snip)
> I openened the mentioned above 5 urls and left the firefox open for 16 hours overnight.
> Memory consumption was 384MB at the morning, (nip)

Did you experience "huge virtual memory consumption like 1.4GB" while you were using Fx 3.0.5 with "-safe-mode"?
Did the "huge virtual memory consumption like 1.4GB" occur again during your daily use of Fx 3.0.6(upgraded 3.0.7) without "-safe-mode"?
Comment 8 martin.zwickel 2009-04-05 23:35:56 PDT
I have the same issue. I have it since I use the 3.1_beta versions.
Everything is fine while I use firefox. But if I let it stay idle for a day (over night) it allocates really much memory:

7099 root      20   0 4757m 3.0g 7860 D    0 79.3   1871:51 firefox-bin

This was after the pages got read from swap. Before it had 3.2g resistent! It always allocates most of my memory over night.
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2009-06-13 13:11:32 PDT
Simeon, others, can you narrow this problem to a single URL, started in safe mode
 https://support.mozilla.com/en-US/kb/Safe+Mode
etc, with latest beta
 http://www.mozilla.com/en-US/firefox/all-beta.html
Comment 10 u348128 2009-07-23 14:01:59 PDT
It looks like that at least the second site is leaking memory.

It is also taking about 5% CPU time, while in IE8 it doesn't leak and doesn't take CPU.

Lot of Javascript, but it shouldn't be hard to reduce the test case.

Lucas
Comment 11 u348128 2009-07-23 14:02:13 PDT
On FF3.5.1.
Comment 12 u348128 2009-07-24 12:35:45 PDT
Created attachment 390514 [details]
Leaking script

The leak is in the silverlight detection script:

http://i2.technet.microsoft.com/Platform/cjs/Silverlight-bn20090616.js

It looks if Silverlight is loaded. If not it waits 3 seconds and looks again.
In each cycle you have a leak.

If the user has no Silverlight or turned it off, it will continue leaking.

In the attachment, I copied the script inline without references. I increased the speed to 10 times a second, showing a much faster leaking (note, leaking may occasionally stop for a while). Turn off Silverlight for reproduction.

The question remains whether the script is leaking (building huge datastructures) or FF. I am pretty sure FF is, because the memory is not reclaimed after closing the tab.

I loaded the attachment without any other tabs. I waited until FF grew from 45MB to 180MB. Then I opened a new tab and closed the leaking tab. The memory usage did not go down and continuing surfing as normal, was on a much higher memory footprint (around 200MB, while normally I surf with 140MB).

Probably the script can further reduced and getting the Silverlight out.

Lucas
Comment 13 u348128 2009-07-24 13:53:29 PDT
Created attachment 390537 [details]
Fully reduced test-case

The leak is in the function:

navigator.plugins.refresh()

The following script eats your memory (see also attachment):

<html>
<head>
<title>Firefox Memory Leak</title></head>
<body>
<script type="text/javascript">

function leaker(){
  navigator.plugins.refresh()
  setTimeout(leaker,50)
};

leaker();

</script>
</body>
</html>

Since this function is in Silverlight detection and used all over Microsoft MSDN pages, it should be solved ASAP. Maybe ported back to 3.0.13.
Comment 14 u348128 2009-07-24 14:09:27 PDT
Can someone put the status and keywords right? I don't have the proper rights to do so. This bug should get more attention now.
Comment 15 Samuel Sidler (old account; do not CC) 2009-07-27 10:34:54 PDT
Comment on attachment 390537 [details]
Fully reduced test-case

Clearing approval request. The approval1.9.1.2 flag is used for requesting approval before landing. I don't think this testcase needs to land, rather, you need a patch to fix the issue before landing.

This bug is really in the wrong component though, so it's unlikely to get an attention.
Comment 16 u348128 2009-07-27 10:48:26 PDT
Can you correct the flags and keywords? I don't have the rights. I just volunteered to analyze the problem.

testcase-wanted should change.
Unconfirmed, should be confirmed (if you can confirm).
And correct the components.
Comment 17 Jo Hermans 2009-08-17 12:06:21 PDT
dupe of bug 395378 ?
Comment 18 Jo Hermans 2009-08-17 15:08:19 PDT
it's also reported in bug 470481 (see the URL that was blocked as a workaround)
Comment 19 u348128 2009-08-17 15:23:45 PDT
Keep this bug? This one is assigned to someone. Should have been solved earlier. Time has been wasted by not solving it.

Note, that you have the memory leak in the Silverlight detection program, when you have DISABLED Silverlight. The script will retry, until Silverlight is there.
Comment 20 Jo Hermans 2009-08-18 01:12:37 PDT
(In reply to comment #19)
> Keep this bug? This one is assigned to someone. Should have been solved
> earlier. Time has been wasted by not solving it.

I only say this because they have to duplciated together. I don't care in what order.

> 
> Note, that you have the memory leak in the Silverlight detection program, when
> you have DISABLED Silverlight. The script will retry, until Silverlight is
> there.

Sure ?

if(!Silverlight.isBrowserRestartRequired&&Silverlight.onSilverlightInstalled){try{navigator.plugins.refresh()}catch(e){}

Will this code be executed when the plugin is disabled ?
Comment 21 u348128 2009-08-18 01:46:46 PDT
*** Bug 395378 has been marked as a duplicate of this bug. ***
Comment 22 u348128 2009-08-18 01:47:23 PDT
*** Bug 470481 has been marked as a duplicate of this bug. ***
Comment 23 u348128 2009-08-18 10:44:03 PDT
#20

I monitored memory behavior and it did only reproduce when Silverlight was disabled. It is the code that sets the timer.

When Silverligth enabled, there might be also a memory leak, but that is a one time leak (it will not repeat after 3 seconds).
Comment 24 Jo Hermans 2009-08-18 13:17:36 PDT
I don't have SilverLight - it's not disabled, it's not installed. That's not the same thing.
Comment 25 Eric Youngdale 2009-08-19 08:13:37 PDT
Oh, thank god that someone is starting to look at this one.  This bug has been driving me crazy for months (I posted one of the dups).  Generally I block the silverlight script, which sort of solves the problem, but before I did this, firefox would crash on a daily basis due to address space exhaustion.

I did some instrumentation back in Jan, and the results are here:

https://bugzilla.mozilla.org/show_bug.cgi?id=470481

There are stack traces from the locations where it appears that the leaked memory was being allocated.

The memory isn't completely leaked - I discovered that if you shut down Firefox, the dll exit routines end up freeing all of the memory.  Thus it appears that the memory isn't lost, but it is just held in a way that it can't be freed.
Comment 26 u348128 2009-08-19 10:27:53 PDT
Yes, this is a good example of wasting time by not solving a problem.
Comment 27 Josh Aas 2009-08-19 10:45:51 PDT
(In reply to comment #26)
> Yes, this is a good example of wasting time by not solving a problem.

Such comments are not productive. See sections 1-1 and 1-2 of Bugzilla Etiquette.

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 28 Josh Aas 2009-09-10 06:05:17 PDT
I don't see this leak using mozilla-central on Windows Vista with SilverLight 3.0. The reports here seem credible but I can't see the leak so I can't fix it. Will look for a new owner.
Comment 29 u348128 2009-09-10 11:33:43 PDT
For me it reproduces with all plugins disabled.

Lucas
Comment 30 Myrrh 2010-01-24 12:20:08 PST
1. SilverLight control script can be found at a whole *.microsoft.com site. Like a here: http://www.microsoft.com/global/en/us/RenderingAssets/Silverlight.js or here: 
http://i3.microsoft.com/global/en/us/RenderingAssets/Silverlight.js
2. Steps to reproduce (successfully confirmed on):
 a) Clean install (as well as already installed) Windows XP x32 (any SP's), Windows Server 2008 x32 (any SP's), Windows 7, Windows Server 2008 R2 x64;
 b) Clean install of any of the released builds of FF from 3.0.x to 3.5.x;
 c) Open in FF http://www.microsoft.com (at this step, out of dozens PC&FF installations, SilverLight plugin installation offer cannot be seen);
 d) Open Process Monitor (ProcMon, By Mark Russinovich and Bryce Cogswell, can be found here: http://technet.microsoft.com/en-us/sysinternals/bb896645.aspx ). Add filter: "Process Name, contains, firefox.exe, Include";
 e) Enjoy the file and registry activity produced by the FF;
 f) Silverlight.isInstalled() function fail, thus, parent function Silverlight.WaitForInstallCompletion() get stuck in the try..catch statement, which will be calling again every 3sec SilverLight.isInstalled() function and, worst of all, navigator.plugin.refresh(), that caused continuous activity of FF.
 g) The more tabs contained Silvelight.js opened the more will be activity (each script working independently). The more plugins installed in FF the more path will be passed through file system and registry. Summary, if more tabs and more plugins in FF instance, than more quickly memory leakage can be seen. Some of the statistics can be found in the related Bug id 516202 https://bugzilla.mozilla.org/show_bug.cgi?id=516202
3) The question is... I understand that this is more than political issue than programmatic. Because, you see, this is vMicrosoftv and vSilveLightv and the rest... But, why in the world dom.max_script_run_time don't work here?
Comment 31 simeon.malchev 2010-01-25 03:30:41 PST
Being the original submitter of this bug, I need to admit that I was simply shocked by the above comment by Myrrh. 

Is it really possible to be true that such completely reproducible and major memory leak will stay completely unaddressed for now 12 months because of “political” issues or similar?

Is it possible an FF developer to clarify this?
Comment 32 Eric Youngdale 2010-01-25 07:13:56 PST
I have to admit that it amazes me that this issue is still open as well.  The repro steps are straightforward enough.  As I dig around, I find more and more duplicates of what is probably the same issue - some of them (including the one I posted (470481) have stack traces where the allocations took place).  Yet they all hang in a sort of limbo waiting for reasons that are not obvious.

If I had time to waste, I would look into it myself and try and come up with a fix.  My problem is that I don't know the architecture of the thing, so I would have to spend a lot of time learning internals - something I would have no use for once I found and fixed the bug.

I should add that there are issues besides memory consumption.  On a laptop, a FF with lots of tabs can also consume a significant fraction of the CPU which can run down the battery and cause the thing to run hot.  The laptop will then turn on the fan which causes even more power consumption.  It was over a year ago that I posted my report, but this was one of the things that got me to start to investigate.

I was frustrated enough that I started to investigate other browsers, but I haven't found anything else that is worthy of considering.
Comment 33 u348128 2010-01-25 11:16:31 PST
Note, that the leak has nothing to do with Silverlight or Microsoft software. Only the Silverlight script triggers it.

CPU consuming is often due to flash advertisements. It might be an idea, to stop flash or Javascript execution in a non-active tab. But that is another discussion.
Comment 34 simeon.malchev 2010-01-25 22:44:28 PST
Let’s try to summarise what we have here:

1. Severe memory leak triggered by a major web site which is most probably visited by thousands of FF users every day worldwide.

2. The bug is proven to be in FF, is easily reproducible and affects all FF 3.x.x versions on any recent MS Windows.

3. The bug stays open and untouched for 12 months now.

4. Does this sound for you as a prioritisation and ownership issue? For me it does. Can please everyone who's receiving this update consider to vote for the bug as "major"? The voting could be done on the top of the bug page next to Importance. Maybe if more people are voting for this bug as Major it will attract the attention of the FF devs and hopefully one of them will take ownership over it.
Comment 35 Eric Youngdale 2010-01-26 05:59:00 PST
If you look through at all of the closed duplicates, you find some that are far older.  Like this one:

https://bugzilla.mozilla.org/show_bug.cgi?id=395378

This one was reported on 2007-09-07.  This puts the age of this bug at about 30 months.
Comment 36 Eric Youngdale 2010-01-26 11:00:55 PST
I am inclined to say that this should be "critical" and not "major".  The description for this state is:

Critical  	crashes, loss of data, severe memory leak

and all of these conditions have been met.  I think Simon needs to change it - I can only change the priority of my own bugs it seems.
Comment 37 simeon.malchev 2010-01-26 12:41:40 PST
Changed the Importance to Critical. Should I change as well the Keywords and the Component? Is the right Component plug-ins?
Comment 38 Myrrh 2010-01-26 14:09:52 PST
(In reply to comment #35)
> If you look through at all of the closed duplicates, you find some that are far
> older.  Like this one:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=395378
> 
> This one was reported on 2007-09-07.  This puts the age of this bug at about 30
> months.

Yep, navigator.plugins.refresh() is a major culprit. And what a coincidence - Ben is posting the bug 2007-09-07 and RTW v.1.0 of the SilverLight is http://en.wikipedia.org/wiki/Microsoft_Silverlight#Release_history .
But IMHO in this case it's not simply bug. It's more then a exploit for FF. Thousands FF users visiting MS site (no wonder for MS Windows users). Many of them suffer of 'sluggish', 'memory leaks', etc. FF credits drop down at a glance. No wonder. But... Right now I have FF with the most ..."have you disable any addons?"... AdblockPlus-vs-NoScript(it's a funny company, yea?), FoxyProxy, FlashGot, MenuEditor, oldbar, SaveWithUrl, TabMixPlus(what a bunch of potentially leakers...) Regularly changing useragents for job needs (please print your useragents string for bug tracking).  50+ tabs opened (sites tracking and "read-it-later"). +-(10-15) tabs of everyday surfing. All of this more then 3 weeks opened 1(!)time opened FF. 220MB. All this time FF is scrupulously add and release memory. No bugs. No leaks. No problem. Just block 1(!) little exploit for FF. I'm happy. Not to say about other unhappy unaware users of FF.

*** This bug has been marked as a duplicate of bug XXXXXX *** maybe?
Comment 39 Jonas Sicking (:sicking) PTO Until July 5th 2010-02-01 16:56:12 PST
Ok, so first off, there is no politics involved here. I have no idea what gave you that impression. In general in order to fix this bug we need to be able to reproduce it. Josh clearly stated that he did not see the problem and thus was unable to fix it.

This does not mean that we don't believe that the problem exists. Nor does it believe that we hold any political persuasion against fixing the bug. It simply means that he was unable to fix the bug. In fact, Josh even mentioned that he believes the bug is there, despite not being able to see it himself.


However, thanks incredibly excellent detective work by Eric Youngdale over in bug 470481 I think I can see what is going on here.

Taking this bug and marking it as a blocker to make sure that we fix it before next release.


It seems that the problem is that the category manager never frees memory from the arena it's using. See bug 470481 comment 9.

Thanks for amazing detective work Eric!
Comment 40 Josh Aas 2010-02-01 23:15:44 PST
Created attachment 424734 [details] [diff] [review]
fix v1.0

I can't think of why we'd need to do any of this category manager registration for plugins any more. I still need to do some more testing but hopefully we can just kill it.
Comment 41 Josh Aas 2010-02-01 23:20:28 PST
Hmm, we might needs those categories for full-page plugins.
Comment 42 Josh Aas 2010-02-01 23:24:25 PST
Comment on attachment 424734 [details] [diff] [review]
fix v1.0

Yeah, we can't do this.
Comment 43 Ryan Lantzer 2010-02-25 17:19:05 PST
I'm pretty sure that this memory "leak" is being introduced by CategoryNode::AddLeaf (in mozilla/xpcom/components/nsCategoryManager.cpp, line 317). It calls ArenaStrdup() to allocate a new string in an "Arena" data store to contain the new value provided by the "aValue" parameter, even when the exact same string has already been allocated in the Arena for the exact same category entry that is being re-registered. I'm pretty sure that there is no way to "free" individual strings within this "Arena" data structure, so every time navigator.plugins.refresh() tells every single firefox component to re-register, each component will allocate a new string value in the "Arena".

I am also under the impression that normal leak detection tools will not catch this as a "leak" because all of the "leaked" string values are stored in an "Arena" that will be cleanly freed when the browser is shut down and the "Arena" is un-allocated as a single entity.

A cheap fix might be to compare the old value to the new value, and only allocate a new string in the Arena if the string has actually change. This would still leak memory on the Arena every time the value was changed, but it might make a huge difference when every single component in the browser gets registered over and over again because of some frequently used JavaScript that keeps refreshing the plugin list over and over again. A better fix might be to stop re-registering every single component each time that navigator.plugins.refresh() is called (or maybe only re-register the components that really need it).

Old code:
    const char* arenaValue = ArenaStrdup(aValue, aArena);

Cheap fix:
    const char* arenaValue;
    // Only allocate a new string on the Arena if the value has actually changed
    if (((leaf->nonpValue)&&(!strcmp(leaf->nonpValue,aValue)))||
      ((leaf->pValue)&&(!strcmp(leaf->pValue,aValue))))
    {
      arenaValue = leaf->nonpValue ? leaf->nonpValue : leaf->pValue;
    }
    else
    {
      arenaValue = ArenaStrdup(aValue, aArena);
    }
Comment 44 Jonas Sicking (:sicking) PTO Until July 5th 2010-02-25 17:24:55 PST
Ryan, your analysis is completely correct.

Ideally though I really think we should write a proper fix here, rather than just using the fix you're describing. Would you be interested in writing one up?
Comment 45 Benjamin Smedberg [:bsmedberg] 2010-03-01 09:36:50 PST
The category manager is meant to store long-lived registration data associated with component/extension registration, and isn't meant to be called frequently during runtime. I'd prefer not to change its behavior (even though the arena is probably a premature optimization) and just not use it at all.
Comment 46 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-21 16:10:51 PDT
--> 1.9.3:final+, but I don't think it blocks a specific beta
Comment 47 Scott Greenlay [:sgreenlay] 2010-09-08 15:07:58 PDT
Can anyone still reproduce this?
Comment 48 Scott Greenlay [:sgreenlay] 2010-09-13 15:48:15 PDT
So I have found that this reproduces on Windows and Linux, but not on Mac.

On Mac, this leak does not occur because at /modules/plugin/base/src/nsPluginHost.cpp#2030, there are never any plug-ins marked as NS_PLUGIN_FLAG_UNWANTED.

On Linux and Windows there are some plug-ins marked, which leads to the plugins being re-added to the Category Manager.
Comment 49 Scott Greenlay [:sgreenlay] 2010-09-21 15:52:36 PDT
On further inspection, I think I have found the root cause.

Silverlight (on Windows) ships with two dlls: npctrl.ddl and npctrlui.dll. The first loads fine, but the second does not register for any MIMETypes, so it is ignored (nsPluginHost:2058). However, this check only occurs once nsPluginHost::ScanPluginsDirectory is told to create a plugin list. As a result, every time navigator.plugins.refresh() is called, npctrlui.dll is considered a 'new' plugin and everything re-registers.
Comment 50 Scott Greenlay [:sgreenlay] 2010-09-22 14:33:10 PDT
Created attachment 477694 [details] [diff] [review]
Plugin Leak Fix (v1.0)

This patch fixes the problem by keeping a second list of 'invalid' plugin tags for plugins that do not declare MIMETypes, and using that when checking for updated plugins.

A similar solution would be keeping a list of nsInvalidPluginTag (a smaller class that only stores the path and the modDate) instead of a full nsPluginTag.
Comment 51 Josh Aas 2010-09-28 00:36:07 PDT
Created attachment 478994 [details] [diff] [review]
ignore invalid dll files altogether, v1.0

I don't have a Windows box right now to test this on but it seems to me that rather than tracking invalid dll files we should just ignore them entirely. Instead of setting "*aPluginsChanged" to true as soon as we don't recognize a dll we should wait until we've actually succeeded in creating a plugin tag before setting that.

I think what I'm suggesting is probably easier to understand via code so I wrote a patch that does it. Like I said, I don't have a Windows box right now so I don't know if this actually works...
Comment 52 Josh Aas 2010-09-28 00:58:41 PDT
Created attachment 478999 [details] [diff] [review]
ignore invalid dll files altogether, v1.1

Fix a bug in my last patch.
Comment 53 Scott Greenlay [:sgreenlay] 2010-09-28 15:26:50 PDT
Created attachment 479198 [details] [diff] [review]
ignore invalid dll files altogether, v1.2
Comment 54 Josh Aas 2010-09-28 17:46:48 PDT
Comment on attachment 479198 [details] [diff] [review]
ignore invalid dll files altogether, v1.2

Looks good. Does it pass on try server?

Want to get a second opinion from jst.
Comment 55 Josh Aas 2010-10-05 11:12:46 PDT
Now that we understand what is going on and we can confirm the severity of the leak as "high" I think we should make this bug block beta 8. I'd prefer to have this fix being tested for problems in betas ASAP.
Comment 56 Johnny Stenback (:jst, jst@mozilla.com) 2010-10-12 16:52:26 PDT
Comment on attachment 479198 [details] [diff] [review]
ignore invalid dll files altogether, v1.2

Looks good! r=jst
Comment 57 Josh Aas 2010-10-13 10:04:36 PDT
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/ced813e0209d
Comment 58 Scott Greenlay [:sgreenlay] 2010-10-20 14:06:55 PDT
Created attachment 484816 [details] [diff] [review]
ignore invalid dll files altogether (1.9.2), v1.0
Comment 59 Scott Greenlay [:sgreenlay] 2010-10-20 14:07:29 PDT
Not sure if the 1.9.2 patch should be put here or in a new bug.
Comment 60 Scott Greenlay [:sgreenlay] 2010-10-22 15:51:23 PDT
Created attachment 485425 [details] [diff] [review]
ignore invalid dll files altogether (1.9.2), v1.1
Comment 61 Daniel Veditz [:dveditz] 2010-10-25 10:32:13 PDT
We're interested in this fix for the 1.9.2 branch, but we'd like some verification that this solves the problem on trunk first.
Comment 62 Josh Aas 2010-10-25 12:04:32 PDT
Can people who were able to reproduce this leak confirm that it is gone with the latest Firefox 4 nightly build?
Comment 63 Daniel Veditz [:dveditz] 2010-11-01 10:53:29 PDT
qawanted: comment 62
Comment 64 Al Billings [:abillings] 2010-12-01 10:35:59 PST
Juan, can you see if this is verified fix on trunk for beta 8?
Comment 65 Daniel Veditz [:dveditz] 2010-12-01 10:36:31 PST
Comment on attachment 485425 [details] [diff] [review]
ignore invalid dll files altogether (1.9.2), v1.1

Still waiting on QA verification of the trunk fix before approving, moving request to next release.
Comment 66 juan becerra [:juanb] 2010-12-01 11:48:42 PST
I can't reproduce the original problem in older builds yet; I don't see a large memory leak as described, but I'll leave the browser running with the reduced test case and see how it behaves after an hour or so. Then I'll check the latest nightly and compare.
Comment 67 Scott Greenlay [:sgreenlay] 2010-12-01 11:59:14 PST
Juan, do you have Silverlight installed?
Comment 68 juan becerra [:juanb] 2010-12-01 12:42:18 PST
I have Silverlight 4.0.50917.0, on a pre 10/15 version of the nightlies. In the last hour the memory usage for the firefox.exe process has doubled, to 250mb. I'll wait another hour and see where it goes, then compare with the latest nightly with a similar amount of time.
Comment 69 juan becerra [:juanb] 2010-12-01 16:50:10 PST
In a build prior to the fix the memory steadily crept up from about 150Mb to 300Mb in a 2hour period. In the latest nightly, the memory usage remained at around 150 for the same time period.

Next I tried a couple more times to see if the the leak would steadily increase in the older builds, which it did, but I only let it run for a half hour or so. The latest build's memory usage, again, remained nearly unchanged in that time period.

I say this is verified on trunk.
Comment 70 Brandon Sterne (:bsterne) 2010-12-06 10:44:57 PST
Comment on attachment 485425 [details] [diff] [review]
ignore invalid dll files altogether (1.9.2), v1.1

a=bsterne for 1.9.2.14.
Comment 71 Daniel Veditz [:dveditz] 2011-01-12 00:34:29 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/095ed971f2a4
Comment 72 Seb A 2011-03-02 13:42:15 PST
I want to (genuinely) thank everyone involved with this bug. It's taken nearly 4 years (see the duplicate bugs), caused me hundreds of crashes over that time thanks to Microsoft exploiting it, caused other people loads of crashes judging by these reports (I only just found them), but is apparently fixed in Firefox 3.6.14.

So thank you for fixing it!  But next time, can we see if we can fix these a little faster? ;)  I dare say it put a lot of people off Firefox.  People who report crashes in Firefox on Bugzilla are the tip of the iceberg.  It affects all versions of Firefox from 2007-trunk (which became 3.0 I think) to 3.6.13.  And because it is a memory leak type crash, many of the crashes were not caught by your Crash Reporter tool so I (and probably others) could not submit them.  (I think that part has been working OK recently but was an issue before.)  And because it is such an old bug, and you guys tend to try and fix newish regressions, I guess it didn't get looked at for a long time by any coders.  Although... it was first reported when it was in trunk, so it should have been fixed before 3.0 was released!

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