1084286 - Need documentation on the blocklist push to production

Status: RESOLVED WONTFIX

(Toolkit :: Blocklist Policy Requests, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

On the page https://wiki.mozilla.org/Blocklisting/Graphics
the item "The blocklist item will be pushed to production."
should point to a detailed documentation.

We, European Mozilla, are stucked until someone knows how this works.
Having a documentation would help if that happens again.

Comment 1

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

Not sure what you want from the docs. We track blocklist entries via bugs and we decide a block should be pushed to production, I do it. It's a matter of filing a form in the AMO admin tools, and at the moment only AMO admins have access to it.

There are some instructions here: https://wiki.mozilla.org/Blocklisting/Admin#Graphics_card_blocks, but they won't be useful to anyone other than AMO admins.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

There seems to be some limitations that were unknown to some/most of us, and maybe that's worth recording.  For instance, it seems that the downloadable blocklist only works on known device IDs, and those device IDs need to be defined in the build?  Benoit, is that the case, or something similar to that?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

I am not aware of a limitation on device IDs, and the build doesn't really define lists of device IDs outside of the "device families" used AFAIK solely for convenience for writing built-in blacklist rules and not relevant to downloadable blacklist rules.

There are other limitations though. In particular, one that's bitten us repeatedly is that downloadable blacklist entries must only refer to FEATURES known to all Firefoxes in circulation, including old ones. Otherwise, if an old Firefox comes across a new blacklist entry referring to a FEATURE unknown to it, it will interprete it as "blacklist all features".

There are probably (certainly) other similar gotchas that I don't know about. I could perhaps find some of them by inspecting our code manually for an hour, but I haven't done that (recently).

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> On the page https://wiki.mozilla.org/Blocklisting/Graphics
> the item "The blocklist item will be pushed to production."
> should point to a detailed documentation.
> 
> We, European Mozilla, are stucked until someone knows how this works.
> Having a documentation would help if that happens again.

The problem is not that there is some sane piece of software that some people understand and just need to document.

The problem is that the blocklisting code is insane and not really understood by anyone - it's not a very big piece of code, but it's sufficiently quirky that only manual line by line inspection of code can reveal issues such as the one described in comment 3.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Case in point:
http://dxr.mozilla.org/mozilla-central/source/widget/xpwidgets/GfxInfoBase.cpp#266

static int32_t
BlacklistFeatureToGfxFeature(const nsAString& aFeature)
{
  if (aFeature.EqualsLiteral("DIRECT2D"))
    return nsIGfxInfo::FEATURE_DIRECT2D;
  else if (aFeature.EqualsLiteral("DIRECT3D_9_LAYERS"))
    return nsIGfxInfo::FEATURE_DIRECT3D_9_LAYERS;
  else if (aFeature.EqualsLiteral("DIRECT3D_10_LAYERS"))
    return nsIGfxInfo::FEATURE_DIRECT3D_10_LAYERS;
  else if (aFeature.EqualsLiteral("DIRECT3D_10_1_LAYERS"))
    return nsIGfxInfo::FEATURE_DIRECT3D_10_1_LAYERS;
  else if (aFeature.EqualsLiteral("DIRECT3D_11_LAYERS"))
    return nsIGfxInfo::FEATURE_DIRECT3D_11_LAYERS;
  else if (aFeature.EqualsLiteral("OPENGL_LAYERS"))
    return nsIGfxInfo::FEATURE_OPENGL_LAYERS;
  else if (aFeature.EqualsLiteral("WEBGL_OPENGL"))
    return nsIGfxInfo::FEATURE_WEBGL_OPENGL;
  else if (aFeature.EqualsLiteral("WEBGL_ANGLE"))
    return nsIGfxInfo::FEATURE_WEBGL_ANGLE;
  else if (aFeature.EqualsLiteral("WEBGL_MSAA"))
    return nsIGfxInfo::FEATURE_WEBGL_MSAA;
  else if (aFeature.EqualsLiteral("STAGEFRIGHT"))
    return nsIGfxInfo::FEATURE_STAGEFRIGHT;
  return 0;
}

Looks sane? Until you realize that the return 0 at the end means "return allFeatures" since allFeatures == 0, so if the feature string doesn't match a known feature, it will be interpreted as "blacklist all features". And since this string comes from a single blocklist.xml file used by all Gecko versions in circulations...

This is just an example to show the level of detail in which code would have to be audited, knowing exactly how it's used in conjunction with AMO blocklisting, in order to answer questions like Milan's above question.

Comment 6

[Robert Kaiser]

(In reply to Benoit Jacob [:bjacob] from comment #5)
> And since this string comes from a single blocklist.xml file used by all Gecko
> versions in circulations...

Well, that would make sense if, like for add-ons/plugins, the code would actually only apply rules in that file that have a Gecko version range matching the running build. But since the graphics code does not have that kind of filtering of which rules apply, it tries to apply rules that only newer versions can reasonably understand. That of course helps making any changes more dangerous.

Comment 7

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

We have talked about completely rewriting the gfx blocklist (bug 838845), but maybe we should begin with a simpler, short-term solution. We could just fork both server and client code and use something like <gfxBlacklistEntry2> to define the new entries, and then modify the forked client code so it at least has more sensible defaults than "Apply to all".

Comment 8

[Robert Kaiser]

(In reply to Jorge Villalobos [:jorgev] from comment #7)
> We have talked about completely rewriting the gfx blocklist (bug 838845),
> but maybe we should begin with a simpler, short-term solution. We could just
> fork both server and client code and use something like <gfxBlacklistEntry2>
> to define the new entries, and then modify the forked client code so it at
> least has more sensible defaults than "Apply to all".

Could we at the same time make it respect Gecko version ranges? That would make it so much easier to apply things only where we want them (and lift blocks when we implement proper fixes).

Comment 9

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

I wouldn't block on that for the short-term solution, but it's certainly something that should be included eventually.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Robert Kaiser (:[e-mail]) from comment #6)
> (In reply to Benoit Jacob [:bjacob] from comment #5)
> > And since this string comes from a single blocklist.xml file used by all Gecko
> > versions in circulations...

This is bug 1162530.


(In reply to Benoit Jacob [:bjacob] (mostly away) from comment #3)
> ...
> There are other limitations though. In particular, one that's bitten us
> repeatedly is that downloadable blacklist entries must only refer to
> FEATURES known to all Firefoxes in circulation, including old ones.
> Otherwise, if an old Firefox comes across a new blacklist entry referring to
> a FEATURE unknown to it, it will interprete it as "blacklist all features".
> 

This is bug 1162299.

I updated the wiki page a bit.

Comment 11

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Still existing but WONTFIXING it