Last Comment Bug 727938 - Block McAfee Site Advisor 3.4.1 -- it leaks every content compartment forever
: Block McAfee Site Advisor 3.4.1 -- it leaks every content compartment forever
Status: RESOLVED WORKSFORME
[MemShrink:P1][3rd-party-bustage]
:
Product: Toolkit
Classification: Components
Component: Blocklisting (show other bugs)
: unspecified
: All Windows 7
: -- blocker with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Jorge Villalobos [:jorgev]
Mentors:
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2012-02-16 11:47 PST by Dietrich Ayala (:dietrich)
Modified: 2016-03-07 15:30 PST (History)
38 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Dietrich Ayala (:dietrich) 2012-02-16 11:47:03 PST
http://blog.mozilla.com/nnethercote/2012/02/16/mcafee-is-killing-us/

These add-ons are seriously harming our users. Versions that exhibit this problem should be blocked.
Comment 1 Nicholas Nethercote [:njn] 2012-02-16 11:52:38 PST
> http://blog.mozilla.com/nnethercote/2012/02/16/mcafee-is-killing-us/

The measurements are hair-raising but someone else should replicate them before we take drastic action.
Comment 2 Nicholas Nethercote [:njn] 2012-02-16 11:56:16 PST
Also, we should give McAfee some time to fix things once we've confirmed the problem.  khuey told me that someone had contacted McAfee about the blog post, so hopefully they're looking into it.
Comment 3 Nicholas Nethercote [:njn] 2012-02-16 14:11:22 PST
I see we blocklisted McAfee ScriptScan 14.4.0 due to crashiness (bug 690184).  Makes me feel better to know there's precedent here.
Comment 4 Ed Morley [:emorley] 2012-02-16 14:20:18 PST
And McAfee SiteAdvisor further back in the past too (v3.3.x, bug 637542).
Comment 5 Nicholas Nethercote [:njn] 2012-02-16 15:02:44 PST
TL;DR:  SiteAdvisor has an appalling memory leak and should be blocklisted immediately.

I spun off bug 728024 for ScriptScan;  this bug is now just about SiteAdvisor.

I installed SiteAdvisor 3.4.1 from www.siteadvisor.com.  I tested on my default Windows 7 FF profile, with Firefox 10.0.1.  (That profile also has AdBlock Plus 2.0.3 and Vertical Tabs 0.9.3, but I confirmed that SiteAdvisor is causing the problems.)

More specifically:  

  *** SiteAdvisor 3.4.1 is leaking every single user compartment that is being created. ***  

This is the worst imaginable behaviour for an add-on in terms of memory consumption.  It wouldn't surprise me if many earlier versions of SiteAdvisor also did the same thing.

Steps to reproduce:
- Open about:memory
- Open any sites you want, many of them
- Close all tabs except about:memory
- Hit "minimize memory usage" in about:memory as many times as you want, the compartments never disappear

I also ran gregor-wagner.com/tmp/mem, which is a memory stress test.  With SiteAdvisor installed, 210 user compartments remained after closing all the opened tabs and minimizing memory usage, and "resident" was 1.5GB.  With SiteAdvisor disabled, the corresponding numbers were 4 user compartments and 477MB.

My advice is that we blocklist *all versions* of SiteAdvisor (3.4.1 and earlier) immediately and contact McAfee at the same time to explain what's going on.

Do we have statistics on how many people have SiteAdvisor installed?
Comment 6 Jorge Villalobos [:jorgev] 2012-02-16 15:24:50 PST
Adding some mcafee people who have Bugzilla accounts. Kev, do you know who we should be talking to about these problems?
Comment 7 Kev Needham [:kev] 2012-02-16 16:06:29 PST
I've fwd'd the bug and comment #5 to the product team at McAfee. Marcia/Shelia, could we repro as well?


(In reply to Jorge Villalobos [:jorgev] from comment #6)
> Adding some mcafee people who have Bugzilla accounts. Kev, do you know who
> we should be talking to about these problems?
Comment 8 Nicholas Nethercote [:njn] 2012-02-16 16:44:38 PST
If anyone can reproduce with older versions of the add-on, that'd be good to know as well.

https://developer.mozilla.org/en/Zombie_Compartments#Reactive_checking has detailed instructions on reproducing zombie compartments.
Comment 9 Nicholas Nethercote [:njn] 2012-02-16 20:21:52 PST
I just tried reproducing this on Mac (Firefox 11.0 beta) and failed to do so.  So it appears to be a Windows-only problem.
Comment 10 Justin Lebar (not reading bugmail) 2012-02-16 22:17:59 PST
Jorge,

Nick says on his blog that you don't think we should immediately block this add-on:

"In the bug report I recommended that we block-list it immediately and contact McAfee, but Jorge Villalobos told me that block-listing is considered an option of last resort."

This add-on causes about 75% of the memory used by a page to never be reclaimed.  This hurts our reputation, it hurts our users, and on Windows machines with 2GB of RAM, it will quickly make Firefox unusable.

If this is not cause for an immediate block under the addons team's policies, I'd like to start a serious discussion about revising those policies.  Standing up for our users here is paramount, in my view.  Please let me know.
Comment 11 Nicholas Nethercote [:njn] 2012-02-16 22:23:04 PST
Furthermore, even if McAfee fixes this very quickly, that fix will presumably only be available in version 3.4.2.  Anyone still stuck on version 3.4.1 will still suffer terrible performance.  So delaying the block-listing of 3.4.1 doesn't achieve anything.
Comment 12 Justin Lebar (not reading bugmail) 2012-02-16 22:58:02 PST
Put another way, we would, without hesitation, skip a release cycle before releasing a version of Firefox which we were aware contained a similar bug.

If we would never release a version of Firefox which we were aware had this bug, how can we justify letting this bug persist on users' machines for even one day longer than it has to?
Comment 13 Scoobidiver (away) 2012-02-17 02:22:39 PST
(In reply to Nicholas Nethercote [:njn] from comment #5)
> Do we have statistics on how many people have SiteAdvisor installed?
If you consider that each ADU has the same crash daily rate, which is wrong, few people have it based on the correlations of the day: 4/54820.

(In reply to Justin Lebar [:jlebar] from comment #10)
> If this is not cause for an immediate block under the addons team's
> policies, I'd like to start a serious discussion about revising those
> policies.
The add-on blocklisting policy is explained here:
https://wiki.mozilla.org/Blocklisting#A_High_Bar
Comment 14 Gervase Markham [:gerv] 2012-02-17 03:16:55 PST
The policy talks about:
"Severe performance impact (e.g. adds more than 75% to start-up time)"

What sort of additional memory use is severe, if this isn't?

We might want to give them a few days to release a fixed version, but we should soft-block the broken versions fairly soon after that. McAfee's stuff is popular - who knows how many people have had and are having terrible Firefox experiences because of this?

Gerv
Comment 15 Nicholas Nethercote [:njn] 2012-02-17 03:51:48 PST
> > Do we have statistics on how many people have SiteAdvisor installed?
>
> If you consider that each ADU has the same crash daily rate, which is wrong,
> few people have it based on the correlations of the day: 4/54820.

Sorry, I don't understand that.  Can you explain what 4/54820 means?
Comment 16 Scoobidiver (away) 2012-02-17 04:22:27 PST
(In reply to Nicholas Nethercote [:njn] from comment #15)
> Sorry, I don't understand that.  Can you explain what 4/54820 means?
* Open the add-on correlation file of the day: https://crash-analysis.mozilla.com/crash_analysis/20120217/20120217_Firefox_10.0.1-interesting-addons.txt.gz
* Search McAfee.
* It's the second column. It means amongst the 54820 crashes of the day, 4 have McAfee add-on installed. So 0.007% of Firefox users have the McAfee add-on installed.
Note that Breakpad misses extensions in startup crashes.
Comment 17 Mike Kaply [:mkaply] 2012-02-17 06:39:40 PST
* It's the second column. It means amongst the 54820 crashes of the day, 4 have McAfee add-on installed. So 0.007% of Firefox users have the McAfee add-on installed.

I'm pretty sure a statistician would call BS on that statement.

All that says is that 0.007% of the people that crashed has McAfee installed.
Comment 18 Andrew McCreight [:mccr8] 2012-02-17 06:42:11 PST
Yes, that's why Scoobidiver gave this disclaimer:
> If you consider that each ADU has the same crash daily rate, which is wrong,
> few people have it based on the correlations of the day: 4/54820.
Comment 19 Jorge Villalobos [:jorgev] 2012-02-17 06:56:59 PST
(In reply to Gervase Markham [:gerv] from comment #14)
> The policy talks about:
> "Severe performance impact (e.g. adds more than 75% to start-up time)"
> 
> What sort of additional memory use is severe, if this isn't?

It may be severe compared to other memory problems, but that doesn't mean it's severe enough for blocklisting. In the blog post itself the user claims there's no performance loss and he investigated out of curiosity of the high memory usage.

The blocklisting page is fairly clear about what we consider severe, and this doesn't even come close, unless there's evidence that hasn't been presented here.

> We might want to give them a few days to release a fixed version, but we
> should soft-block the broken versions fairly soon after that.

Yes, we should give them time to fix it, and we can soft-block the old versions once we have given them enough time to publish the update.

> McAfee's stuff
> is popular - who knows how many people have had and are having terrible
> Firefox experiences because of this?

It is very popular, all the more reason to not make rushed decisions and cause anger and confusion in (possibly) millions of users. What evidence is there that this is causing a bad experience to the significant portion of users that have this add-on installed? It's very likely that this bug has been present in that product for years, so why is there an urgency to fix this overnight by blocklisting? Why would we want to damage our relationship with McAfee by shooting first and asking later?

If you want to discuss the blocklisting process, we can do that in a different place (dev.planning, maybe?). As it stands, I don't expect this blocklist to happen before the fix is published or we know that McAfee has no intention on fixing this problem.
Comment 20 Mike Kaply [:mkaply] 2012-02-17 07:02:07 PST
Sorry, missed that. Wasn't in the second comment.

I have another question on this issue.

Do McAfee users receive their updates via updateURL or via updates to the antivirus themselves?
Comment 21 Justin Lebar (not reading bugmail) 2012-02-17 07:27:51 PST
> It may be severe compared to other memory problems, but that doesn't mean it's severe enough for 
> blocklisting. In the blog post itself the user claims there's no performance loss and he 
> investigated out of curiosity of the high memory usage.

It's true that this particular user did not notice performance problems.  I wonder how many people on fast machines would notice Firefox starting up 75% slower.

If Firefox never frees compartments, as it will if you have this add-on installed, then consider the performance of the whole machine.  If you have 2GB of RAM, as is common, and Firefox takes up 1.6GB, then every time you try to do *anything* else on your machine, it will page.  If you have 1GB of RAM, Firefox will be unusable.

So if you want a severe performance impact, it is:

 * On a 2GB machine, the whole machine locks up for tens of seconds (paging) while switching between Firefox and a memory-intensive application, such as Photoshop, another web browser, a game, a MS Office product, etc.

 * On a 1GB machine, Firefox and the whole machine becomes completely unusable after a day's browsing, even if you only ever open one site at a time, until the user ditches Firefox and switches to a working web browser.

For someone who uses Firefox on a machine with 1GB of RAM, or for someone who uses more than one program at once, this will be a *huge* performance problem.

> Why would we want to damage our relationship with McAfee by shooting first and asking later?

This is a key point of disagreement, I think.  Let me ask: Why would we want to damage our relationship with our users by not taking action immediately when we discover that a third party has injected brain-dead code into their web browser?  What's more important here?

McAfee should be worried about their relationship with *us*.  Indeed, I'm going to propose in a separate bug that we require specifically theirs, but also all third party add-ons, be reviewed and signed *before* we allow them to run in the browser.

Consider the relative severity of this issue by asking: Would an informed user choose this leak over 75% slower startup?  It's laughable, honestly, even to ask this; this kind of leak is many classes more severe than a mere startup regression.
Comment 22 Justin Lebar (not reading bugmail) 2012-02-17 07:47:52 PST
>  Indeed, I'm going to propose in a separate bug that we require specifically theirs, but also all 
> third party add-ons, be reviewed and signed *before* we allow them to run in the browser.

I've filed bug 728227 for this.
Comment 23 davoudm 2012-02-17 08:17:53 PST
The issue with SiteAdvisor has been fixed and is in QA now. It should be rolled out to all user base in a week. Thanks for reaching out to McAfee and letting us know.

McAfee has, and always will want to have a great relationship with FireFox and other browser vendors and that is why Kev and I and other FF engineers are in constant contact.

Please remember any issues effecting FireFox users also effect our users so we have the best of interest to make sure issues are resolved in a timely manner.

Thanks again for all your help.
Comment 24 Justin Lebar (not reading bugmail) 2012-02-17 08:20:26 PST
> It should be rolled out to all user base in a week.

So we can block all old versions of the extension one week from today?

> The issue with SiteAdvisor has been fixed and is in QA now.

It would be fantastic if you could provide us with this pre-release version so we can verify that the issue is fixed.
Comment 25 Jorge Villalobos [:jorgev] 2012-02-17 08:47:41 PST
(In reply to davoudm from comment #23)
> The issue with SiteAdvisor has been fixed and is in QA now. It should be
> rolled out to all user base in a week. Thanks for reaching out to McAfee and
> letting us know.

Thank you for the quick response. Please note that there's also bug 728024 filed for the ScriptScan add-on.

Let us know when the new version is available, so we can time soft-blocking the old versions of the add-on.
Comment 26 davoudm 2012-02-17 08:54:23 PST
>>So we can block all old versions of the extension one week from today?
Most users will be updated in one week. Although there maybe others who lag behind. It would be good if we can do that in 2 weeks. The version will still be 3.4.1.xx. It is only the build number that will be higher than 193 (current build). Can you block per build version since the main version will still be 3.4.1?

>>It would be fantastic if you could provide us with this pre-release version so we can verify that the issue is fixed.

We would love to do that. I can send a copy to Kev next Tuesday and he can send it to you. He is traveling today and we are out on Monday due to holiday.
Comment 27 Jorge Villalobos [:jorgev] 2012-02-17 09:28:03 PST
(In reply to davoudm from comment #26)
> >>So we can block all old versions of the extension one week from today?
> Most users will be updated in one week. Although there maybe others who lag
> behind. It would be good if we can do that in 2 weeks. The version will
> still be 3.4.1.xx. It is only the build number that will be higher than 193
> (current build). Can you block per build version since the main version will
> still be 3.4.1?

If the version string in install.rdf is different, we can make that distinction. If it isn't, then we can't. If the build number is part of the version string, then there's no problem. Otherwise I recommend you bump up the version number to 3.4.1.1 or something like that.
Comment 28 Mike Kaply [:mkaply] 2012-02-17 09:37:59 PST
It would also be nice to know a general idea of what the problem was so that it can be documented not to do that.
Comment 29 Paul Wright 2012-02-17 12:02:57 PST
Regarding the question "to blocklist or not to blocklist", my take on it is that Firefox has lost many users because of this issue.  Here is my reasoning:

1.  Until relatively recently, Firefox used more resources than it should have, causing slowdowns on the "average" machine
2.  This addon made the issue an order of magnitude (at least) worse than it already was.
3.  A user who's program is killing his/her machine will not keep using said program, and since this addon (I think?) was being installed with little user interaction, the full blame was put on Firefox.

At the end of the day, there may be a large install-base for this addon, but I doubt the percentage of active users is extremely high.

It doesn't take long to lose a customer, but it sure does take a lot of effort to win one.
Comment 30 Naer Chang 2012-02-17 12:39:49 PST
I updated 728024. It appears that Script Scan has nothing to do with this issue.
Comment 31 Scoobidiver (away) 2012-02-17 13:25:50 PST
(In reply to Michael Kaply (mkaply) from comment #17)
> I'm pretty sure a statistician would call BS on that statement.
Based on comment 29, I am not sure that it's so statistically biased. Either users made the correlation between the high memory usage and the McAfee extension and they disabled it, or they switched to another browser blaming Mozilla for that.
Comment 32 Scoobidiver (away) 2012-02-18 07:27:16 PST
(In reply to Scoobidiver from comment #13)
> > Do we have statistics on how many people have SiteAdvisor installed?
> If you consider that each ADU has the same crash daily rate, which is wrong,
> few people have it based on the correlations of the day: 4/54820.
It seems that the extension count is under-estimated in crash reports, maybe because of startup crashes where extensions are not fully loaded. Indeed, if I check the sahook.dll McAfee Site Advisor binary, the crash ratio is 1972/54820, i.e. 3.6% which is more realistic.

Anyway, I compared the McAfee Site Advisor crash ratio between Fx 7 and Fx 8 (third party add-on prompt on startup) on Nov 11, 2011 when there were as many users in each version:
* Fx 7.0.1: 6338/95883 i.e. 6.6%
* Fx 8: 4145/95929 i.e. 4.3%
So one third of McAfee Site Advisor users let it disabled.
Three months later, it has decreased additionally from one sixth. It's this part that matches the extension disabling or the browser switching I described in comment 31.
Comment 33 Justin Lebar (not reading bugmail) 2012-02-22 08:19:54 PST
Note that the new version, which is slated for release today (?) is a huge improvement, but based on our testing of the pre-release version, it still leaks some compartments.
Comment 34 Kev Needham [:kev] 2012-02-22 10:04:32 PST
(In reply to Justin Lebar [:jlebar] from comment #33)
> Note that the new version, which is slated for release today (?) is a huge
> improvement, but based on our testing of the pre-release version, it still
> leaks some compartments.

...and we're working with McAfee to try to determine if the problem is with their addon or with Firefox, because it's not clear at this point. McAfee also uses a separate update service, so these changes should be pushed pretty quickly to the large majority of users who have this installed. I'll discuss the softblock witht hem for older versions after some time for uptake of the update.
Comment 35 Jorge Villalobos [:jorgev] 2012-02-22 10:32:57 PST
* I filed bug 729608 to track the leaks in the newer versions.
* I also filed bug 729614 for softblocking the older versions once the uptake of the latest update is sufficient. Since users are pointed to the block bug when they want to read more info about it, I'd rather not use this bug which is very noisy at this point.

I'll close this bug once we have confirmation that the new version is available. It should have been released about an hour ago, I was told.
Comment 36 davoudm 2012-02-22 10:51:50 PST
The new version is available on siteadvisor.com and we have started rolling out to all user base. It will take several days before entire user base is populated.
Comment 37 Jorge Villalobos [:jorgev] 2012-02-22 10:56:20 PST
Closing as WORKSFORME, though this is being followed up on other bugs, as explained in comment #35.
Comment 38 Nicholas Nethercote [:njn] 2012-02-22 13:48:08 PST
(In reply to Jorge Villalobos [:jorgev] from comment #35)
> * I filed bug 729608 to track the leaks in the newer versions.
> * I also filed bug 729614 for softblocking the older versions once the
> uptake of the latest update is sufficient. Since users are pointed to the
> block bug when they want to read more info about it, I'd rather not use this
> bug which is very noisy at this point.

Good idea!  Thanks, Jorge.
Comment 39 Justin Lebar (not reading bugmail) 2012-03-14 16:27:33 PDT
Marking with [3rd-party-bustage], a new whiteboard tag I'm using to track problems caused by non-AMO add-ons.
Comment 40 Nicholas Nethercote [:njn] 2012-03-14 17:08:06 PDT
(In reply to Justin Lebar [:jlebar] from comment #39)
> Marking with [3rd-party-bustage], a new whiteboard tag I'm using to track
> problems caused by non-AMO add-ons.

You're conflating "third-party" with "non-AMO" again...
Comment 41 Justin Lebar (not reading bugmail) 2012-03-14 17:38:51 PDT
Just to confuse the heck out of everyone, I thought I'd use "third-party" to mean "non-AMO", here.  I think "drive-by add-ons" is an un-ambiguous descriptor to contrast with "third-party add-ons", which, in its demonstrated ambiguity, can refer to both drive-by and user-installed non-AMO add-ons.

But if you want to change the whiteboard name, feel free.

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