Last Comment Bug 1152630 - ship a hotfix for bug 1151721 - Blacklist older intel drivers for DXVA
: ship a hotfix for bug 1151721 - Blacklist older intel drivers for DXVA
Status: RESOLVED WONTFIX
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: ---
Assigned To: Matt Woodrow (:mattwoodrow)
:
: Maire Reavy [:mreavy] Please needinfo me
Mentors:
Depends on: 1151721
Blocks: MSE
  Show dependency treegraph
 
Reported: 2015-04-08 19:35 PDT by Chris Peterson [:cpeterson]
Modified: 2015-08-25 01:57 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
unaffected
unaffected
unaffected
unaffected


Attachments
hotfix (4.59 KB, patch)
2015-04-08 19:40 PDT, Matt Woodrow (:mattwoodrow)
ajones: review+
felipc: feedback+
Details | Diff | Splinter Review
hotfix-v20150409.01.xpi (1.68 KB, application/x-xpinstall)
2015-04-08 19:45 PDT, Matt Woodrow (:mattwoodrow)
no flags Details

Description User image Chris Peterson [:cpeterson] 2015-04-08 19:35:23 PDT
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 User image Matt Woodrow (:mattwoodrow) 2015-04-08 19:40:06 PDT
Created attachment 8590030 [details] [diff] [review]
hotfix
Comment 2 User image Matt Woodrow (:mattwoodrow) 2015-04-08 19:45:54 PDT
Created attachment 8590032 [details]
hotfix-v20150409.01.xpi
Comment 3 User image Robert Kaiser 2015-04-09 05:40:52 PDT
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 User image :Felipe Gomes (needinfo me!) 2015-04-09 06:40:37 PDT
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 User image Chris Peterson [:cpeterson] 2015-04-09 17:41:04 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 User image Chris Peterson [:cpeterson] 2015-04-09 17:47:03 PDT
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 User image Matt Woodrow (:mattwoodrow) 2015-04-09 18:10:49 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 User image Anthony Jones (:kentuckyfriedtakahe, :k17e) 2015-04-10 10:58:23 PDT
(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 User image :Felipe Gomes (needinfo me!) 2015-04-10 11:07:22 PDT
(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 User image Jeff Muizelaar [:jrmuizel] 2015-04-10 15:42:17 PDT
FWIW, I'm interested I'm also interested in a 37.0.2 for bug 1153381
Comment 11 User image Jeff Muizelaar [:jrmuizel] 2015-04-10 15:43:10 PDT Comment hidden (off-topic)
Comment 12 User image Jeff Muizelaar [:jrmuizel] 2015-04-10 15:43:24 PDT Comment hidden (off-topic)
Comment 13 User image Chris Peterson [:cpeterson] 2015-04-13 14:46:59 PDT
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 User image Matt Woodrow (:mattwoodrow) 2015-04-13 16:33:48 PDT
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 User image Chris Peterson [:cpeterson] 2015-04-13 16:44:30 PDT
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 User image Chris Peterson [:cpeterson] 2015-04-14 08:54:24 PDT
Blocklist bug 1151721 has been approved for 37.0.2 chemspill so we don't need this hotfix.

Note You need to log in before you can comment on or make changes to this bug.