Closed Bug 729608 Opened 12 years ago Closed 12 years ago

McAfee Site Advisor 3.4.1.195 leaks compartments while browsing

Categories

(WebExtensions :: General, defect)

All
Windows 7
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jorgev, Unassigned)

References

Details

(Whiteboard: [MemShrink:P1][3rd-party-bustage])

Attachments

(1 file)

9.13 KB, application/octet-stream
Details
Bug 727938 pointed out major leaks in McAfee Site Advisor, some of which have been fixed in version 3.4.1.195. This bug is intended to track and fix the remaining leaks in the add-on.
I have not updated to the new version yet but I certainly was having the issue prior to learning it was causing a memory leak and disabling it. I have Windows Vista Home Premium. Probably would be good to update it and enable it to see if it still leaks memory huh? Was trying to wait until it was entirely fixed.
Well I tried to download the latest version and install it but the version number is 3.4.1.123 and it should be 3.4.1.193 right? I don't understand what is going on... maybe I should just give up on this add-on as I only have memory leaks when it is enabled. I actually began using chrome because of the memory leak caused by it.
Excuse me... it should be 3.4.1.195
zeonis:  did you install Site Advisor in Chrome?  If not, it sounds like it's not an important add-on for you, in which case disabling it in Firefox is a reasonable choice.
Yes I installed Site Advisor in Chrome. So yes I like having it but this whole mess makes me question whether I should just give up on it...
Are you checking for updates in the Firefox Add-ons Manager or in the McAfee software? It should be the latter, I think.
I did both... first I had the add-ons manager check and it said no updates found. Then I tried downloading it from siteadvisor.com, installed it, and restarted my computer as it says is necessary. Version still reads 3.4.1.123...
The version in RDF has changed. In addition the about box of the plugin identifies version xx.195.
Maybe the new version is only on Windows 7? I don't know what the deal is but my version still reads xx.123.
I have attached a sample plug-in (helloworld) which demonstrates there is a leak in FF even without SiteAdvisor plug-in. Please remove our plug-in and install this xpi to demonstrate the issue.

 Please see the following steps and repeat the steps with commenting one line of code to see the issue going away. Please send us any feedback you have.

 As always, thanks much and looking forward to your feedback.

 - After you install the XPI, follow the steps  below:

 *	Navigate to about:memory
 *	In a new tab, navigate to mcafee.com
 o	This is important -- mcafee.com is actually hardcoded in the plugin. It simulates us injecting our annotations code for some domains, but not others. The issue is still intermittently seen if you run the code in all tabs, but this is one way we found if seeing it 100% of the time. This isn't dependent on the mcafee.com domain, it'll work if you change the javascript to check for google, etc.
 *	In about:memory, observe that there is now a compartment for the mcafee.com tab
 *	Close the mcafee.com tab
 *	In about:memory, observe that the mcafee.com tab remains after hitting GC/Minimize Memory Usage/F5

 The issue goes away if you unzip the plugin, go content/overlay.js and uncomment the line in SampleCode1 that sets the function pointer to null:
 // g_pfnCreateEngine1 = null;
 You can then zip it back up, rename it to .xpi and install it in FF. Repeating the above steps should now NOT yield a leaked compartment.
Attached file Hello world plugin
I tested this XPI and could reproduce the leak on Mac OS.

The relevant code is here: http://pastebin.com/EtxYNcw9.

What the code does is add a DOMContentLoaded event listener to the "appcontent" element (this is in chrome code). The listener code then evals a function and stores it in a global variable. For some reason, this global variable is the one causing the content leak.
(In reply to Jorge Villalobos [:jorgev] from comment #12)
> The relevant code is here: http://pastebin.com/EtxYNcw9.
> 
> What the code does is add a DOMContentLoaded event listener to the
> "appcontent" element (this is in chrome code). The listener code then evals
> a function and stores it in a global variable. For some reason, this global
> variable is the one causing the content leak.

That's because you're dealing with a closure. Setting domdoc to null should prevent keeping it alive.
(In reply to Dão Gottwald [:dao] from comment #13)
> That's because you're dealing with a closure. Setting domdoc to null should
> prevent keeping it alive.

Can you please elaborate? Where is the reference to domdoc being held, and why? Is this because a global function (SampleCode1) is being called and passed the document as an argument? Does the eval have anything to do with the leak?
domdoc is part of _SampleEvalFnc's scope, which equals g_pfnCreateEngine1's scope as you set g_pfnCreateEngine1 = _SampleEvalFnc. If g_pfnCreateEngine1 is called at some later point, it could potentially access domdoc, so this needs to be kept alive.
OK, so if domdoc were to be used in the eval'd function, this function shouldn't be stored globally, or at least it should be nulled when the document expires.

If domdoc isn't being used in the function, there's no reason to be passing domdoc as a parameter to that function anyway and should be removed.
(In reply to Jorge Villalobos [:jorgev] from comment #16)
> OK, so if domdoc were to be used in the eval'd function, this function
> shouldn't be stored globally, or at least it should be nulled when the
> document expires.
> 
> If domdoc isn't being used in the function, there's no reason to be passing
> domdoc as a parameter to that function anyway and should be removed.

Right.  Or you shouldn't use eval.  In some cases the JS engine can optimize the unused away, but once you put an eval in here all bets are off.  There are various other cases that can cause this too (anything that makes the function "heavyweight" in our engine) but there's no definite set of rules.  Restricting what is in the scope of the closure and avoiding crazy things like eval is the best way to handle this.
Whiteboard: [MemShrink] → [MemShrink:P1]
Is SiteAdvisor a JavaScript add-on?  Sounds like it is.  If McAfee is willing to privately send a copy of the add-on code to Mozilla, I'm sure someone at Mozilla could provide more help with the leak.
It is not purely a JS plug-in. It has JS components. We just did this sample JS to demonstrate there is a leak . BTW, the same sample JS does not leak in IE or Google Chrome.
> BTW, the same sample JS does not leak in IE or Google Chrome.

Well, you're not running this sample JS as an extension in IE or Chrome, are you?  (I suppose even if you were, it's possible that the code would do something different, because extensions are different in different browsers.)

AIUI this leaks because |var g_pfnCreateEngine1| is *global to the entire browser*, so it never goes away.  If instead one loaded this same JS in a webpage, it shouldn't leak in any browser, because the variable would go away when the window closed.

This is a classic JS gotcha, that functions keep their scope alive.  There are lots of ways to work around it, but not using a global variable seems best to me.
What is the status of this bug?

We are happy to provide assistance; is there anything further we can do to help you track down the problem?
Sorry, we have been swamped with a bunch of stuff. We will respond in the next several days. Obviously we have to find a fix for this one way or another.
What's the status on this bug?
We have found a solution to this issue.  A patch is currently scheduled to be pushed early April.  I will post a follow-up once this patch is posted.
> A patch is currently scheduled to be pushed early April.  I will post a follow-up once this patch is 
> posted.

We'd very much like to test this add-on before it's released, if that's possible.
Marking with [3rd-party-bustage], a new whiteboard tag I'm using to track problems caused by third-party add-ons.
Whiteboard: [MemShrink:P1] → [MemShrink:P1][3rd-party-bustage]
kev, can you please follow up re comment 25?
(In reply to Justin Lebar [:jlebar] from comment #27)
> kev, can you please follow up re comment 25?

request sent.
Component: General → Add-ons
Product: Core → Tech Evangelism
> We'd very much like to test this add-on before it's released, if that's
> possible.

I tested a pre-release version on April 4 that didn't create any zombies.  Has that fix made it into a released version?  Will existing users have been updated?
This fix has been released to production. Most of the SA user base should have already received this update.
If it is released than why when I check for updates does it say I am running the latest version (3.4.1.195)?
This fix was included in a content update, not a client binary update.  You can confirm you have received the fix by opening: %programfiles%\mcafee\siteadvisor\scrips\safe.js
The first line of this file specifies the version.  The fix was included in version 1.0.196.  Please confirm you have at least this version.
Okay I checked and it does say 1.0.196 on the first line of that file. Thank you for that information.
Calling this fixed per comment #30 and comment #33.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to Geoff from comment #32)
> This fix was included in a content update, not a client binary update.  You
> can confirm you have received the fix by opening:
> %programfiles%\mcafee\siteadvisor\scrips\safe.js
> The first line of this file specifies the version.  The fix was included in
> version 1.0.196.  Please confirm you have at least this version.

In the future, please update the version number when you release a fix like this.

As is, it's difficult for us to measure uptake of this new version and determine how many users are stuck on the old version.  It's also difficult for us to eventually block the old, leaky version.
Component: Add-ons → General
Product: Tech Evangelism → WebExtensions
You need to log in before you can comment on or make changes to this bug.