729171 - Modify AMO review policy to check for addons that disable incremental GC

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Incremental GC just landed in Firefox 13.  It is possible for an addon to be incompatible with incremental GC.  If the GC detects that, it will shut off incremental GC, which will degrade the user experience quite a bit.  For this reason, roc suggested that the AMO review process check for this.  Basically, after you use the addon for a bit, look in about:support to see if "Incremental GC" is set to 1 (as opposed to 0).

Here are some comments from the incremental GC bug about this:

billm: "It adds a new flag on the JSClass that should only be set if the class implements write barriers correctly. If we ever create an object whose class has a trace hook and doesn't have this flag set, then we permanently disable incremental GC as long as the browser is running."

khuey: "Yes, but the only addons that can disable IGC are ones that implement their own custom JSAPI stuff (a custom JSClass with a trace hook, to be precise), so this should be very rare."

There are some big caveats here:

- Currently, the browser itself does not work with incremental GC properly all of the time, so you can't start checking it right away.  At a minimum, bug 728686 will have to be fixed.  Possibly there will be other cases where a browser without addons will cause incremental GC to be disabled.

- There should probably be some documentation on how to fix this.  I don't know enough about this to do that.

- One problem is that the check is fairly crude: it just checks that a certain flag is set, it does not actually check that the hook is implemented correctly.  It would be possible for an addon author to blindly set this flag without fixing the problem, to pass the test.  I don't know if that means some kind of basic code inspection should be done, too.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

If I got this right, this flag would only be switched when add-ons "implement their own custom JSAPI stuff", which Kyle correctly indicates is extremely rare. I wouldn't expect more than a handful add-ons doing this, most of them being Firebug and its extensions. I don't think that's a test we can ask all editors to do. It'll pass more than 99% of the time and they'll just begin ignoring it.

However, there's an Extension Test add-on that many of us use, built by Kris (CCd), that could throw an error if the pref is flipped. I think that's the best we can offer in this case.

@Andrew: what is the preference name?

@Kris: this is easy to include in Extension Test, right?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That sounds good, Jorge. This is the code I used in about:support to test whether incremental is enabled. I think it should work in an add-on as well:

  let enabled = window.QueryInterface(Ci.nsIInterfaceRequestor)
        .getInterface(Ci.nsIDOMWindowUtils)
        .isIncrementalGCEnabled();

Comment 3

[Kris Maglione [:kmag]]

I can add the check to log a message to the Error Console when incremental GC becomes disabled. I think the policy issues are less black an white, though. Most of the times add-ons trigger this behavior will be because of core bugs, as mentioned in the description. If bug 641025 comment 117 is correct, then only add-ons with binary components can trigger this. I don't think I've ever seen an add-on that interacts with JSAPI this way (they usually use XPCOM for those purposes).

I think that the check is a good idea, but we should probably report it to the add-on author and file a bug rather than taking action against the add-on until we can establish that the add-on is at fault.

Comment 4

[Kris Maglione [:kmag]]

Added a test that prints an error to the Error Console when incgc becomes disabled:

https://addons.mozilla.org/en-US/firefox/addon/extension-test/versions/#version-2.7.6

It may be better to just add the message to the core, though.

Comment 5

[Kris Maglione [:kmag]]

Incidentally, this can be tested with something like the following at the moment:

let a=[];
a[1<<26] = 2;
a.length = 1;

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

I sent a message to the review team so that they keep an eye on these errors.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to Andrew McCreight [:mccr8] from comment #0)

> khuey: "Yes, but the only addons that can disable IGC are ones that
> implement their own custom JSAPI stuff (a custom JSClass with a trace hook,
> to be precise), so this should be very rare."

This would imply it's a binary addon (or possibly jsctypes), I assume? Or can a pure JS addon do this?

Comment 8

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

(In reply to Justin Dolske [:Dolske] from comment #7)
> (In reply to Andrew McCreight [:mccr8] from comment #0)
> 
> > khuey: "Yes, but the only addons that can disable IGC are ones that
> > implement their own custom JSAPI stuff (a custom JSClass with a trace hook,
> > to be precise), so this should be very rare."
> 
> This would imply it's a binary addon (or possibly jsctypes), I assume? Or
> can a pure JS addon do this?

No.