1161590 - Create a pref (not in about:config) to allow working around graphics blacklisting for testing purposes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[u279076]

Milan and I were discussing our graphics regression testruns with Betabreakers today. We realized that some of them might be testing on blacklisted systems since some of their test systems use older hardware and drivers by design. We thought it might be a good idea to provide them a way to work around the blacklist and re-enable features on their systems for testing purposes. 

Milan's idea was to create a hidden pref that disables the blacklist. For security purposes this pref would have to be manually entered in about:config and only work in daily builds (Nightly & Aurora).

Making this a confidential bug since we want to limit exposure of this pref to the public.

Comment 1

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

We can eventually (perhaps as a part of the meta bug 838845) cover the scenario where we want to ignore/force blocklisting for individual features - this one is the big hammer that just does everything wholesale.

Comment 2

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

This is not particularly secret, we just don't want people to start using it outside of particular testing functionality we need.  Perhaps it may be worth doing telemetry on this.

Comment 3

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

Attachment: Option to force or ignore blocklisting. r=kats

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8602348 [details] [diff] [review]
Option to force or ignore blocklisting. r=kats

Review of attachment 8602348 [details] [diff] [review]:
-----------------------------------------------------------------

The gfxPrefs bit looks sane to me but I don't really know the blocklisting code so somebody else should probably look over that and make sure it's sane.

::: widget/GfxInfoBase.cpp
@@ +1009,5 @@
> +  static int32_t blocklistAll = 0;
> +  if (!blocklistAllInit) {
> +    MOZ_ASSERT(gfxPrefs::SingletonExists());
> +    blocklistAll = gfxPrefs::BlocklistAll();
> +    blocklistAllInit = true;

Is there a point to this cache? I thought gfxPrefs already did this caching internally and was cheap to call.

Comment 5

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8602348 [details] [diff] [review]
> Option to force or ignore blocklisting. r=kats
> 
> Review of attachment 8602348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The gfxPrefs bit looks sane to me but I don't really know the blocklisting
> code so somebody else should probably look over that and make sure it's sane.
> 
> ::: widget/GfxInfoBase.cpp
> @@ +1009,5 @@
> > +  static int32_t blocklistAll = 0;
> > +  if (!blocklistAllInit) {
> > +    MOZ_ASSERT(gfxPrefs::SingletonExists());
> > +    blocklistAll = gfxPrefs::BlocklistAll();
> > +    blocklistAllInit = true;
> 
> Is there a point to this cache? I thought gfxPrefs already did this caching
> internally and was cheap to call.

Valid point; I can probably just remove that whole thing.

Comment 6

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

(In reply to Milan Sreckovic [:milan] from comment #5)
> ...
> > 
> > ::: widget/GfxInfoBase.cpp
> > @@ +1009,5 @@
> > > +  static int32_t blocklistAll = 0;
> > > +  if (!blocklistAllInit) {
> > > +    MOZ_ASSERT(gfxPrefs::SingletonExists());
> > > +    blocklistAll = gfxPrefs::BlocklistAll();
> > > +    blocklistAllInit = true;
> > 
> > Is there a point to this cache? I thought gfxPrefs already did this caching
> > internally and was cheap to call.
> 
> Valid point; I can probably just remove that whole thing.

Just to document - I originally put this in anticipating there will be "many" places (at least platform specific, and perhaps in more than one place per platform) where we had to check this value.  From what I can tell, it is only needed in one place, so just checking the pref directly should be fine.

Comment 7

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

Attachment: Part 2. Option to force or ignore blocklisting. r=jmuizelaar

Use pref directly, it's only checked in one place in the code.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24e2b063e06f - need to initialize graphics preferences so that xpcshell tests can get to them.  Will get a pre-patch for that.

Comment 9

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

Attachment: Part 1. xpcshell to initialize graphics prefs as xpcshell tests are hitting this path before the other initialization. r=ehsan

Comment 10

[(no longer active)]

This bug should be opened up.  We have many other secret prefs, and I don't remember ever landing the code for them in a confidential bug.  In fact the only places where I've seen us land code in a confidential bug for have been stuff that we couldn't do publicly because of NDAs.  Milan, do you agree?

Comment 11

[(no longer active)]

Comment on attachment 8602746 [details] [diff] [review]
Part 2. Option to force or ignore blocklisting. r=jmuizelaar

Review of attachment 8602746 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPrefs.h
@@ +200,5 @@
>    DECL_GFX_PREF(Once, "gfx.android.rgb16.force",               AndroidRGB16Force, bool, false);
>  #if defined(ANDROID)
>    DECL_GFX_PREF(Once, "gfx.apitrace.enabled",                  UseApitrace, bool, false);
>  #endif
> +#if defined(NIGHTLY_BUILD) || defined(MOZ_DEV_EDITION)

Nit: please use RELEASE_BUILD instead.  It should be functionally equivalent to this.

Comment 12

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

Attachment: Part 2. Option to force or ignore blocklisting. Carry r=jmuizelaar

Use defined(RELEASE_BUILD) instead.

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95cea19a80e5

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2df087f15d6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd5921f4b3b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2df087f15d6e
https://hg.mozilla.org/mozilla-central/rev/5bd5921f4b3b