1179504 - Disable WARP on Windows 7 for FF40, FF41

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Disable WARP for Windows 7 on Aurora.

We think there might be some unknown issues with WARP on Windows 7 that might make some machines unusable. Let's disable it for windows 7 for now.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

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

Ok.

Comment 2

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

Thanks for the speed of this Bas; if this is Aurora only patch, it means we request uplift before we land on nightly?

Comment 3

[u279076]

Hi Bas,

Can you help me understand the anticipated user experience and any implications that has on QA? In particular, I might want to do some testing on Windows XP/Vista but I may also want to do some testing on Windows 7/8/10 to make sure nothing is inadvertently broken. And it's okay if you think this change doesn't need QA but I'd like to understand this better so I can at least keep an eye out in our typical feedback channels for issues in the wild.

Thanks

Comment 4

[Bas Schouten (:bas.schouten)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #3)
> Hi Bas,
> 
> Can you help me understand the anticipated user experience and any
> implications that has on QA? In particular, I might want to do some testing
> on Windows XP/Vista but I may also want to do some testing on Windows 7/8/10
> to make sure nothing is inadvertently broken. And it's okay if you think
> this change doesn't need QA but I'd like to understand this better so I can
> at least keep an eye out in our typical feedback channels for issues in the
> wild.
> 
> Thanks

I don't think we really need QA here in practice. The user implications will be very machine dependent. Basically we're moving a set of users on Windows 7 from their current WARP D3D11 compositor to the traditional basic compositor, which is alreadys ort of the 'safe mode' for these users.

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Approval Request Comment
[Feature/regressing bug #]: D3D11 WARP
[User impact if declined]: We suspect D3D11 WARP may not be working correctly for some users on Windows 7, resulting in a black screen.
[Describe test coverage new/current, TreeHerder]: Try
[Risks and why]: Extremely low, moving to safer configuration.
[String/UUID change made/needed]: None

Comment 6

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

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Approving for uplift to Aurora. This is a top crash and therefore the sooner we know how well the fix impacts stability the better for us.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

Given the incidence rate of crashes due to WARP I'm tracking for 40 and 41.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3cfb1740978

Comment 9

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

Release Note Request (optional, but appreciated)
[Why is this notable]: This is going to de-accelerate, but also (hopefully) stabilize a number of Windows 7 systems.
Again, unclear if this should be a release note or not, but it is noteworthy.  Also, notice we uplifted to at least 41, so if we are to release note it, it shouldn't wait for 42.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Just like Ritu said.

Comment 11

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

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Take away beta+ - this patch is not enough to disable warp.  Yes, it does in some, but not in all cases.  Bas is getting a patch.

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Ensure WARP is always unused on Windows 7.

Carrying review from bug 1159751.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Approval Request Comment
[Feature/regressing bug #]: WARP
[User impact if declined]: Some unknown amount of bugs for Win7 users
[Describe test coverage new/current, TreeHerder]: Nightly and aurora
[Risks and why]: Very few, switching users to fallback
[String/UUID change made/needed]: None

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 8633525 [details] [diff] [review]
Part 2: Ensure WARP is always unused on Windows 7.

Approval Request Comment
[Feature/regressing bug #]: WARP
[User impact if declined]: Some unknown amount of bugs for a small group of Win7 users
[Describe test coverage new/current, TreeHerder]: Try
[Risks and why]: Very few, switching users to fallback
[String/UUID change made/needed]: None

Comment 15

[u279076]

(In reply to Bas Schouten (:bas.schouten) from comment #14)
> [User impact if declined]: Some unknown amount of bugs for a small group of Win7 users

FWIW Betabreakers is doing a testrun against Dev Edition later this week which should hopefully get additional data on the impact of this change. They'll be testing a selection of 30 systems across all Windows versions with various AMD, Intel, and NVIDIA GPUs (old and new).

Comment 16

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

Bas, will this patch apply to dev edition?

Comment 17

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #16)
> Bas, will this patch apply to dev edition?

It should.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8633525 [details] [diff] [review]
Part 2: Ensure WARP is always unused on Windows 7.

Let's take it now to have it quickly in beta.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Let's take it now to have it quickly in beta.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

This caused massive failures on Aurora. I'm renomming for Beta. We shouldn't take this change until we confirm that fix on an earlier channel.

Comment 21

[Robert Kaiser]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #20)
> This caused massive failures on Aurora. I'm renomming for Beta. We shouldn't
> take this change until we confirm that fix on an earlier channel.

Well, it was the patch attached to bug 1159751 that caused the issues. The patch here might be fine.

Comment 22

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

Comment on attachment 8633525 [details] [diff] [review]
Part 2: Ensure WARP is always unused on Windows 7.

Nah, this patch is bad.  The one above is incomplete, in that it doesn't catch all the problems, but it isn't bad - just doesn't fix all the cases.  This one, on the other hand, makes things worse.  We're working on a better one, but will need separate version for aurora/beta and nightly as there were other changes in nightly in the same code.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Beta- based on the recent comments. We need a better patch for uplift.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Attachment: Part 2: Ensure WARP is always unused on Windows 7.

This is the uncrashing version of part 2 that is already on Aurora via bug 1159751

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

The problems from the other part of the patch have been fixed and landed on Aurora. This part was always safe.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Now that this fix has stabilized, we'll get it into beta7. Beta+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ad8e3522ff7c

Part 2 landed on Aurora/Beta in bug 1159751. AFAICT, nothing has landed on trunk yet. Not sure if that's intentional or not.

Comment 28

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> https://hg.mozilla.org/releases/mozilla-beta/rev/ad8e3522ff7c
> 
> Part 2 landed on Aurora/Beta in bug 1159751. AFAICT, nothing has landed on
> trunk yet. Not sure if that's intentional or not.

Intentional - there is a major rewrite of that code tracked in bug 1179051.

Comment 29

[Robert Kaiser]

This disabling of WARP on Win7 didn't only make the shutdownhangs of bug 1159751 go away, it also reduced OOMs a lot.

Comment 30

[u279076]

(In reply to Robert Kaiser (:[e-mail]) from comment #29)
> This disabling of WARP on Win7 didn't only make the shutdownhangs of bug
> 1159751 go away, it also reduced OOMs a lot.

Do you have numbers illustrating how much "a lot" is?

Comment 31

[Robert Kaiser]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #30)
> (In reply to Robert Kaiser (:[e-mail]) from comment #29)
> > This disabling of WARP on Win7 didn't only make the shutdownhangs of bug
> > 1159751 go away, it also reduced OOMs a lot.
> 
> Do you have numbers illustrating how much "a lot" is?

Not as nicely as I'd like to, I'm still working on getting a good display of OOM and other types of crashes up.

I'm already creating the dataaset for that, though, here are some raw numbers of total OOM crashes (signatures starting with "OOM | ") in the last week or so on the beta channel:

July 20  4237
July 21  4521
July 22  4506
July 23  4456
July 24  3894 (release of b7; note: we did have 5-10% crash data loss on that day)
July 25  2816 (Sat, lower numbers normal on weekend)
July 26  2013 (Sun, lower numbers normal on weekend)
July 27  3209
July 28  2917

Comment 33

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

Added to FF41 release notes in Nucleus.

Comment 34

[Benoit Girard (:BenWa)]

I renamed the title to be more accurate.

If we have no aim of fixing this on trunk because of bug 1179051, which has landed, then we should close this as FIXED.

ni? :dvander who fixed bug 1179051. Can you confirm that we don't need to disable WARP on 42+ because of the improvements made in bug 1179051? If so I think we're good to close this.

Comment 35

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Benoit Girard (:BenWa) from comment #34)
> I renamed the title to be more accurate.
> 
> If we have no aim of fixing this on trunk because of bug 1179051, which has
> landed, then we should close this as FIXED.
> 
> ni? :dvander who fixed bug 1179051. Can you confirm that we don't need to
> disable WARP on 42+ because of the improvements made in bug 1179051? If so I
> think we're good to close this.

Yup, WARP is gated on Windows 8 or later on 42+.

Comment 37

[marvinhk]

(In reply to David Anderson [:dvander] from comment #35)
> (In reply to Benoit Girard (:BenWa) from comment #34)
> > I renamed the title to be more accurate.
> > 
> > If we have no aim of fixing this on trunk because of bug 1179051, which has
> > landed, then we should close this as FIXED.
> > 
> > ni? :dvander who fixed bug 1179051. Can you confirm that we don't need to
> > disable WARP on 42+ because of the improvements made in bug 1179051? If so I
> > think we're good to close this.
> 
> Yup, WARP is gated on Windows 8 or later on 42+.

I guess "Windows 8 or later on 42+" means that we still need to have it disabled on Win7 for 42+ ...
Not sure if we should call this fixed based on this.

Comment 38

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

Actually, "WARP is gated on Windows 8 or later on 42+" meant "you need Windows 8 or later to get WARP as of 42 and subsequent releases", so we should be OK.