715221 - Modify AMO review checklist to disallow add-ons which disable the XUL cache

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Policy, defect)

Description

[(no longer active)]

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:

https://mxr.mozilla.org/addons/search?string=disable_xul_cache

Any of these add-ons which is not focused on debugging XUL is broken and should be disabled.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is a major degradation of product quality and needs to be addressed.

Comment 2

[Justin Lebar (not reading bugmail)]

For those of us not in the know, what does the XUL cache do?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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]]

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 [inactive]]

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] (he/him)]

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 [inactive]]

I think we can WONTFIX this -- bug 725465 sounds good, but this bug doesn't really depend on it.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, we still want this.

Comment 9

[Nicholas Nethercote [inactive]]

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] (he/him)]

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]]

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:
https://addons.mozilla.org/en-US/firefox/addon/developer-profile/ -F
https://addons.mozilla.org/en-US/firefox/addon/featurefix/ -F
https://addons.mozilla.org/en-US/seamonkey/addon/seamonkey-debug-and-qa-ui/ -F
https://addons.mozilla.org/en-US/firefox/addon/extdev-preferences/ -F
https://addons.mozilla.org/en-US/firefox/addon/guiconfig/ -F
https://addons.mozilla.org/en-US/firefox/addon/mozrepl/ -P
https://addons.mozilla.org/en-US/firefox/addon/extension-developer/ -F
https://addons.mozilla.org/en-US/firefox/addon/configuration-mania-4420/ -F

The following don't have any reason to set the preference:
https://addons.mozilla.org/en-US/firefox/addon/kuby/ -P
https://addons.mozilla.org/en-US/firefox/addon/bookie-for-firefox/ -P
https://addons.mozilla.org/en-US/firefox/addon/mytabs/ -P
https://addons.mozilla.org/en-US/firefox/addon/clickclean/ -F

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

(Technically, https://addons.mozilla.org/en-US/firefox/addon/app-runtime/ does it too but only in installed apps.)

Comment 12

[Andrew Williamson [:eviljeff]]

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)]

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] (he/him)]

The validation in bug 725465 should be enough for reviewers to do the right thing.