1152630 - ship a hotfix for bug 1151721 - Blacklist older intel drivers for DXVA

Status: RESOLVED WONTFIX

(Core :: Audio/Video, defect)

Description

[Chris Peterson [:cpeterson]]

The deployment steps from https://wiki.mozilla.org/Firefox/Hotfix:

1. The intent to ship a new hotfix needs to be notified to release-drivers@m.o and moc@m.c
2. Bug is filed with the unsigned hotfix file attached.
3. Bug is assigned to the Build & Release team for signing. (Ping in #releng or email release@m.c if urgent.)
4. File is signed and is attached to the bug.
5. Bug is re-assigned to Add-ons team for staging (normally :jorgev, otherwise contact ammo-team@m.c).
6. Signed file is uploaded to addons-dev.allizom.org and published.
7. Bug is reassigned to QA for testing.
8. QA signs off, hotfix owner signs off.
9. Bug is reassigned to Add-ons team for final deployment.
10. Add-ons team notifies release-drivers@m.o and moc@m.c about the deployment, including bug number.
11. Signed file is uploaded to addons.mozilla.org and published.

@Matt: please attach your unsigned addon when it's ready (step #2) and proceed to step #3.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: hotfix

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: hotfix-v20150409.01.xpi

Comment 3

[Robert Kaiser]

1) Why can we do / do we need a hotfix here? I thought the two mechanisms we had were either in-code blocklists (which would need binary changes, which a hotfix can't do) or downloadable blocklists (which don't need a hotfix but only delivering a new entry).

2) Last I heard, the downloadable blocklists for gfx stuff were something we couldn't use before a complete rewrite of the system as they would end up blocking all graphics features for older Firefox versions. Is this different here?

3) If this is needed to work around downloadable blocklist insufficiency, when will the rewrite come that will make this trivial (again) by actually using downloadable blocklists?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8590030 [details] [diff] [review]
hotfix

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

There can be only one active hotfix at a time, and the current one has been released too recently, so we can't stop shipping its fix at the moment. With that, you'll need to make this hotfix include the code of the previous one too, and decide which blocks of code to run based on the target versions. It's simple, there's a previous hotfix where we did that: https://hg.mozilla.org/releases/firefox-hotfixes/file/0ddf0fcc8006/v20150106.01/bootstrap.js#l95

The targetPlatforms and versions in the install.rdf will have to be the union of both hotfixes.



And on a more general question, is it really necessary to ship this hotfix if we also ship a chemspill for bug 1151721?

::: v20150409.01/bootstrap.js
@@ +50,4 @@
>      return false;
>    }
>  
> +  var gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);

I asked Ajones this on IRC, but I'd like to get your take on this too Matt: what's the risk of running this code to all users and causing crashes? I vaguely recall some bugs in the past about people opening about:support in the past and crashing because there were some really bad gpu/drivers out there that simply accessing them would cause problems.

Can we do a check of Windows version before running this to reduce the subset of users this runs?

And if there's any risk of crashes I'd like to see some protection for running this code only once if it crashes (i.e., set a pref before it runs, make sure the pref saves, and only then attempt to run it.. And check if the pref exists before doing that)

::: v20150409.01/install.rdf
@@ +17,5 @@
>      <em:targetApplication>
>        <Description>
>          <!-- Firefox -->
>          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> +        <em:minVersion>36.0</em:minVersion>

I thought bug 1151721 was caused by something that just shipped in 37.. If that's the case, shouldn't minVersion be 37.0?

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Robert Kaiser (:[e-mail]) from comment #3)
> 1) Why can we do / do we need a hotfix here? I thought the two mechanisms we
> had were either in-code blocklists (which would need binary changes, which a
> hotfix can't do) or downloadable blocklists (which don't need a hotfix but
> only delivering a new entry).

Instead of modifying the in-code blocklist, the hotfix just toggles off the "media.windows-media-foundation.use-dxva" pref for Firefox 37 (and 36?) users with the bad driver version.


> 3) If this is needed to work around downloadable blocklist insufficiency,
> when will the rewrite come that will make this trivial (again) by actually
> using downloadable blocklists?

jst says someone will look into the downloadable blocklist in Q2 (but not necessarily rewrite it in Q2).

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 8590030 [details] [diff] [review]
hotfix

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

::: v20150409.01/bootstrap.js
@@ -30,5 @@
> -    // (including Fx35 typo'd versions)
> -    Services.prefs.setBoolPref("network.http.atsvc.enabled", false);
> -    Services.prefs.setBoolPref("network.http.atsvc.oe", false);
> -    Services.prefs.setBoolPref("network.http.altsvc.enabled", false);
> -    Services.prefs.setBoolPref("network.http.altsvc.oe", false);

bsmedberg said we might want to keep the previous hotfix's fix (bug 1148328) because it was published recently and might not have been widely installed yet.

@@ +50,4 @@
>      return false;
>    }
>  
> +  var gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);

We could also check whether the "media.windows-media-foundation.use-dxva" pref is true before we start poking the GPU. If the pref is already false, then the hotfix doesn't need to do anything.

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert Kaiser (:[e-mail]) from comment #3)
> 1) Why can we do / do we need a hotfix here? I thought the two mechanisms we
> had were either in-code blocklists (which would need binary changes, which a
> hotfix can't do) or downloadable blocklists (which don't need a hotfix but
> only delivering a new entry).

The binary change would need a chemspill release (which I assume we want to avoid), and the downloadable  blocklist doesn't support disabling DXVA as per 2).

> 
> 2) Last I heard, the downloadable blocklists for gfx stuff were something we
> couldn't use before a complete rewrite of the system as they would end up
> blocking all graphics features for older Firefox versions. Is this different
> here?

This is correct, and why we need a hotfix instead of using it.


(In reply to :Felipe Gomes from comment #4)
> 
> There can be only one active hotfix at a time, and the current one has been
> released too recently, so we can't stop shipping its fix at the moment. With
> that, you'll need to make this hotfix include the code of the previous one
> too, and decide which blocks of code to run based on the target versions.
> It's simple, there's a previous hotfix where we did that:
> https://hg.mozilla.org/releases/firefox-hotfixes/file/0ddf0fcc8006/v20150106.
> 01/bootstrap.js#l95
> 
> The targetPlatforms and versions in the install.rdf will have to be the
> union of both hotfixes.

Ok, I can look into doing this. My JS isn't great though, will need to be tested thoroughly.

> 
> And on a more general question, is it really necessary to ship this hotfix
> if we also ship a chemspill for bug 1151721?

No, a chemspill release would work too, is that an option?



> 
> I asked Ajones this on IRC, but I'd like to get your take on this too Matt:
> what's the risk of running this code to all users and causing crashes? I
> vaguely recall some bugs in the past about people opening about:support in
> the past and crashing because there were some really bad gpu/drivers out
> there that simply accessing them would cause problems.
> 
> Can we do a check of Windows version before running this to reduce the
> subset of users this runs?
> 
> And if there's any risk of crashes I'd like to see some protection for
> running this code only once if it crashes (i.e., set a pref before it runs,
> make sure the pref saves, and only then attempt to run it.. And check if the
> pref exists before doing that)

I'm not aware of any crashes just from querying gfxInfo. I believe the previous crashes were from other things about:support does (like creating a webgl context).

> 
> ::: v20150409.01/install.rdf
> @@ +17,5 @@
> >      <em:targetApplication>
> >        <Description>
> >          <!-- Firefox -->
> >          <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> > +        <em:minVersion>36.0</em:minVersion>
> 
> I thought bug 1151721 was caused by something that just shipped in 37.. If
> that's the case, shouldn't minVersion be 37.0?

The blacklist relaxing change that broke HTML5 video went out earlier, I believe it was 36 so I wanted to cover that too.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> > I thought bug 1151721 was caused by something that just shipped in 37.. If
> > that's the case, shouldn't minVersion be 37.0?
> 
> The blacklist relaxing change that broke HTML5 video went out earlier, I
> believe it was 36 so I wanted to cover that too.

I tested 36 and it works because layers acceleration is disabled. The bug was introduced in Firefox 37.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #8)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> > > I thought bug 1151721 was caused by something that just shipped in 37.. If
> > > that's the case, shouldn't minVersion be 37.0?
> > 
> > The blacklist relaxing change that broke HTML5 video went out earlier, I
> > believe it was 36 so I wanted to cover that too.
> 
> I tested 36 and it works because layers acceleration is disabled. The bug
> was introduced in Firefox 37.

If there's no chemspill for 37, it's actually a better idea to run on some previous versions too, because then the pref is flipped ahead of time and the users won't be affected when they upgrade.
Since the previous hotfix went for 35, and this one would need to be merged with that one, I believe that's the range we should target (35 - 37).

However, if the default value for the pref on versions previous than 37 was false, then this flipping-pref-in-advance strategy won't work.


I see that bug 1151721 has an approval-mozilla-release requested. Are there ongoing conversations about doing that? I do think that's the best option.
But Matt's comment about no known crashes from querying gfxInfo makes me a lot more comfortable for the hotfix.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

FWIW, I'm interested I'm also interested in a 37.0.2 for bug 1153381

Comment 11

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/726812ce0633

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/726812ce0633

Wrong bug

Comment 13

[Chris Peterson [:cpeterson]]

Matt: what are the next steps to complete the hotfix add-on? Release Management says there is a good chance we will release a 37.0.2 chemspill. If so, then we can just uplift bug 1151721 and forget about the hotfix.

Comment 14

[Matt Woodrow (:mattwoodrow)]

I'd need to merge the two hotfixes and upload the new version. I'd much prefer to just uplift bug 1151721 and ship 37.0.2 though, any idea when we'll know if that's happening?

Comment 15

[Chris Peterson [:cpeterson]]

Uplifting just the blocklist change sounds much easier. :)

lmandel says "I think we're going to do a 37.0.2."  I will follow with Release Management about the 37.0.2 schedule. You can probably just put the hotfix on hold for now.

Comment 16

[Chris Peterson [:cpeterson]]

Blocklist bug 1151721 has been approved for 37.0.2 chemspill so we don't need this hotfix.