1309330 - Change the D3D9 default fallback preference on Firefox 49 using a system add-on

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #1306465 +++

Spin-off from bug 1304360.

This is for the system add-on to change the preference layers.allow-d3d9-fallback to false.  We have the preference flip handled manually in 50.

I expect to have the actual fix in 51, so this system add-on for the preference flip should be 49 only.

Comment 1

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

Attachment: System add-on for changing a D3D9 fallback preference.

This particular patch should be r-, as it doesn't have an ID - where does that come from?
Also, it wasn't clear where to specify that this should be version 49.* only.

Hopefully, it's otherwise moving in the right direction.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

That ID is Firefox's ID, so you don't need to change it. It should be equal to all other system add-ons.

The 49.* will come from the @MOZ_MAX_APPVERSION@ replacement in install.rdf.in. You'll get 49.* if you build it using the mozilla-release tree. Or you can manually create a correct install.rdf.

If you build from the tree, your final files will end up in objdir/dist/bin/browser/features/

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8799915 [details] [diff] [review]
System add-on for changing a D3D9 fallback preference.

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

::: browser/extensions/d3d9fallback/bootstrap.js
@@ +16,5 @@
> +}
> +
> +function startup() {
> +  let defaults = new Preferences({defaultBranch: true})
> +  defaults.set(PREF_D3D9_FALLBACK, false);

this will dynamically change the pref at some point after startup, on every startup, because prefs on the default branch don't stick after the browser quits. Which is fine, and the recommended way to do.

But will this pref take effect dynamically, or whatever was read by the graphics code on startup will stick for the session?

Comment 4

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

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Comment on attachment 8799915 [details] [diff] [review]
> System add-on for changing a D3D9 fallback preference.
> 
> Review of attachment 8799915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/d3d9fallback/bootstrap.js
> @@ +16,5 @@
> > +}
> > +
> > +function startup() {
> > +  let defaults = new Preferences({defaultBranch: true})
> > +  defaults.set(PREF_D3D9_FALLBACK, false);
> 
> this will dynamically change the pref at some point after startup, on every
> startup, because prefs on the default branch don't stick after the browser
> quits. Which is fine, and the recommended way to do.
> 
> But will this pref take effect dynamically, or whatever was read by the
> graphics code on startup will stick for the session?

This preference is "read once use after that", so it would stick for the session.  Is there a way to change the add-on so that this gets set earlier?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

No, the order of the startup process has no guarantees, and it's likely that the platform code will win over the add-on. So you should just use a regular pref that will get saved to the user's profile. Has the value of this pref changed in Firefox 50, so that `false` will become the default once they upgrade?

Comment 6

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

Yes, this pref is false in 50.

Comment 7

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

Attachment: System add-on for changing a D3D9 fallback preference.

So, like this?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8799984 [details] [diff] [review]
System add-on for changing a D3D9 fallback preference.

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

Yeah. One thing to note is that this code will keep forcing the pref to false (on startup) even if the user had switched it back to true. If you're fine with this, this version is fine. Otherwise you can move the code to install() which will run only once.

::: browser/extensions/moz.build
@@ +8,5 @@
>      'e10srollout',
>      'pdfjs',
>      'pocket',
>      'webcompat',
> +    'd3d9fallback',

this list needs to be sorted alphabetically

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Milan, what do you think? Seems like switching the pref only once on first install should be sufficient.

Comment 10

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

Attachment: System add-on for changing a D3D9 fallback preference

Updated patch with latest round of feedback, since Milan is busy today. r? to felipe.

I assume that once this is reviewed, it will need approval for, and land directly on, mozilla-release?

Comment 11

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

This version of the patch flips the pref on first install only ^. I figured it shouldn't make too much difference either way, but this way allows a user who legitimately needs to flip the pref back to true, to do so.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8800288 [details] [diff] [review]
System add-on for changing a D3D9 fallback preference

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

Yeah, this looks good.

Next steps: 

- build this so that you get a post-processed install.rdf
- zip the bootstrap.js and install.rdf files into an .xpi (I think if you do a make package that will also happen, but I'm not sure)
- get it signed
- QA it and announce it to gofaster@ and release-drivers@

Comment 13

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

Attachment: [e-mail]

Hi Jason, could you sign this add-on please? We are planning to ship it as a system addon in 49 to fix a regression there, and need it to be signed for QA and eventual deployment.

Comment 14

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> Milan, what do you think? Seems like switching the pref only once on first
> install should be sufficient.

Milan said on IRC that he agrees with moving the code from startup() to install().

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

We should test and ship the system add-on, then we should also land a patch on m-r afterwards, in case we do a dot release for 49 this change can go along with it. 

Here's the process so far as we have it documented: https://wiki.mozilla.org/Firefox/Go_Faster/Process#Process_to_Push_updates_to_release_channel

Comment 16

[Jason Thomas [:jason]]

Attachment: [e-mail] signed

Please see attached signed add-on.

Comment 17

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

Thanks!

Does anybody know which QA team is supposed to sign off on the system addon? Starting with RyanVM since his team handles testing telemetry experiment add-ons and I'm hoping the process is similar.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Yeah, it's Ryan's team. To set up the experiment for them, please take the current update.xml for 49 and include your new add-on on it. You can host a new file and the signed add-on at people.mozilla.org to let them test it.

This is the current update.xml in 49:

https://aus5.mozilla.org/update/3/SystemAddons/49.0/20160310153207/Darwin_x86_64-gcc3-u-i386-x86_64/en-GB/release/Darwin%2014.5.0/default/default/update.xml

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

We have another system add-on coming too, the async rendering one in bug 1307108.  It makes sense to me to keep them separate add-ons, but maybe test and ship them together.

Comment 20

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

Cool, I uploaded the modified update.xml to here:

https://people.mozilla.org/~kgupta/bug/1309330/update.xml

and it points to the signed XPI at this URL: https://people.mozilla.org/~kgupta/bug/1309330/[e-mail]

Comment 21

[Michelle Funches - QA]

QA Update: yes the SV-Vegas team can handle the testing of this system addon. I will contact "felipe" & "kats" for information.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Changing a pref for users on release 49.
[Suggested wording]:   Setting the default preference layers.allow-d3d9-fallback to false, to fix graphics glitches for some users (Bug 1306465)
[Links (documentation, blog post, etc)]:

Comment 23

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

Comment on attachment 8800288 [details] [diff] [review]
System add-on for changing a D3D9 fallback preference

Approval Request Comment
[Feature/regressing bug #]: bug 1262187
[User impact if declined]: Windows users with HWA experience graphics artifacts
[Describe test coverage new/current, TreeHerder]: that flipping the pref fixes the issues was done in bug 1304360
[Risks and why]: I guess there's a risk that the pref flip doesn't fix it for all users?
[String/UUID change made/needed]: none

Comment 24

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

NI Standard8 and CC a couple other folks. It may make sense to send this and bug 1307108 out tomorrow AM if possible.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

This still needs testing before we ship it.

Comment 26

[Mark Banner (:standard8)]

I've put this on the release-sysaddon channel along with the one from bug 1307108. They will be installed for 49.0 and 49.0.1 only. The complete set that should be installed with the update are:

- d3d9 1.0
- async plugin rendering 1.0
- Multi-process 1.3

Comment 27

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

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> This still needs testing before we ship it.

Michelle's QA team did run through a test cycle for it.

Comment 28

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #27)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> > This still needs testing before we ship it.
> 
> Michelle's QA team did run through a test cycle for it.

After a discussion with felipe on IRC we realized that we should probably wait until both system addons have been tested together, to make sure there's no badness that results from both pref flips together. Although the other pref has been on beta for 3 weeks, this one has only been on beta 9 days which may not be enough time to really flush out bugs. Felipe said he'd talk to Michelle to get some additional QA on the combined update.

Comment 29

[Michelle Funches - QA]

QA Update: Thanks Felipe, we are prepping to tested combined.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800288 [details] [diff] [review]
System add-on for changing a D3D9 fallback preference

We need this patch to be included in 49.0.2 as the system add-on update is only configured to flip this pref on 49 and 49.0.1.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

This has been included in 49.0.2 as a direct pref flip (bug 1306465 comment 19). So the system add-on itself is not necessary

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8800288 [details] [diff] [review]
System add-on for changing a D3D9 fallback preference

Felipe mentioned that this has already landed on m-r. See https://hg.mozilla.org/releases/mozilla-release/rev/25e8a5c6be62b96ede0993fd78ddb0c90dc6343f

Comment 33

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

Makes sense, thanks.

Has the system addon been pushed yet?

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Yeah, it was pushed with 10% throttling on Monday (Oct 17) and unthrottled yesterday (Oct 18).

This has now been shipped to 49.0 and 49.0.1 as a system add-on, and landed as a pref flip on 49.0.2 (see comment 31).