Last Comment Bug 715221 - Modify AMO review checklist to disallow add-ons which disable the XUL cache
: Modify AMO review checklist to disallow add-ons which disable the XUL cache
Product: Graveyard
Classification: Graveyard
Component: Policy (show other bugs)
: unspecified
: All All
: -- major
: Q1 2012
Assigned To: Andrew Williamson [:eviljeff]
Depends on: 725465
  Show dependency treegraph
Reported: 2012-01-04 11:01 PST by :Ehsan Akhgari
Modified: 2016-02-04 14:51 PST (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Description :Ehsan Akhgari 2012-01-04 11:01:20 PST
So I discovered today that I have XUL cache disabled.  I'm seeing extremely high memory usage and extremely long GC/CC pauses.  I know that I have not set the nglayout.debug.disable_xul_cache pref myself, so this got me curious.  Look and behold:

Any of these add-ons which is not focused on debugging XUL is broken and should be disabled.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-04 13:16:43 PST
This is a major degradation of product quality and needs to be addressed.
Comment 2 Justin Lebar (not reading bugmail) 2012-01-04 13:25:33 PST
For those of us not in the know, what does the XUL cache do?
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2012-01-04 13:31:37 PST
It's an in-memory cache of chrome stylesheets, XUL prototype documents, and XBL prototype bindings.

The main effect is that you don't have to keep loading/parsing these things; you just use the existing in-memory representation every time they're referenced.  This saves both CPU time and memory.
Comment 4 Andrew Williamson [:eviljeff] 2012-01-06 04:09:08 PST
I've not checked every match but a lot of these addons are old (max 3.6) or the preference is commented out so it might not be as widespread as it appears.  The validator now flags every preference set outside the "extensions." branch so we should catch any updates also.
Comment 5 Nicholas Nethercote [:njn] 2012-01-10 15:43:00 PST
In today's MemShrink meeting we talked with the AMO guys about ways to flag add-ons as causing poor performance.  Combine that with comment 4, this seems like a low priority thing.  If we can identify any particular popular add-ons that disable the XUL cache, that might make it higher priority.
Comment 6 Jorge Villalobos [:jorgev] 2012-02-08 14:09:46 PST
We won't blocklist add-ons based on this, but we will adjust our policies so that we check for this preference more attentively. I filed bug 725465 to add a validator check for this preference, and we'll investigate all add-ons that modify this preference.
Comment 7 Nicholas Nethercote [:njn] 2012-02-08 14:27:56 PST
I think we can WONTFIX this -- bug 725465 sounds good, but this bug doesn't really depend on it.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-08 15:22:40 PST
No, we still want this.
Comment 9 Nicholas Nethercote [:njn] 2012-02-08 15:32:00 PST
Sorry, I misunderstood.  To clarify:

- This bug is about changing the AMO policy to disallow add-ons that disable the XUL cache.

- Bug 725465 is about adding an automated check for this in the add-on validation tool.
Comment 10 Jorge Villalobos [:jorgev] 2012-02-14 14:30:53 PST
Andrew: please look into all MXR results for this preference and send me a list of the add-ons that use it that are compatible with 4 and above.
Comment 11 Andrew Williamson [:eviljeff] 2012-02-15 04:37:55 PST
okay (I thought I'd post here so everyone is aware of the scale). Fully reviewed indicated with -F, Preliminary with -P.

The following add-ons could be classed as 'specialist', in that they are either designed to set up a development environment or expose 'hidden' Firefox prefs with a UI.  Unless we want to get really strict I'd say these are valid use-cases: -F -F -F -F -F -P -F -F

The following don't have any reason to set the preference: -P -P -P -F

Only the last once, clickclean, is fully reviewed so requires action.

(Technically, does it too but only in installed apps.)
Comment 12 Andrew Williamson [:eviljeff] 2012-02-15 08:17:29 PST
I've contacted the developers of the four addons that set the preference without any need to requesting they remove XUL cache disable preference.  I've additionally downgraded the fully reviewed addon, clickclean, to preliminary reviewed.
Comment 13 Justin Lebar (not reading bugmail) 2012-02-15 08:18:44 PST
From comment 9:
> - This bug is about changing the AMO policy to disallow add-ons that disable the XUL cache.

Has the AMO policy been updated as well?
Comment 14 Jorge Villalobos [:jorgev] 2012-02-15 08:35:36 PST
The validation in bug 725465 should be enough for reviewers to do the right thing.

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