Disable WARP on Windows 7 for FF40, FF41

RESOLVED FIXED

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: bas, Assigned: bas)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 wontfix, firefox40+ fixed, firefox41+ fixed, firefox42 affected, relnote-firefox 41+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8628514 [details] [diff] [review]
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.
Attachment #8628514 - Flags: review?(jmuizelaar)
Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

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

Ok.
Attachment #8628514 - Flags: review?(jmuizelaar) → review+
Thanks for the speed of this Bas; if this is Aurora only patch, it means we request uplift before we land on nightly?
Flags: needinfo?(bas)
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
(Assignee)

Comment 4

2 years ago
(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.
Flags: needinfo?(bas)
(Assignee)

Comment 5

2 years ago
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
Attachment #8628514 - Flags: approval-mozilla-beta?
Attachment #8628514 - Flags: approval-mozilla-aurora?
Blocks: 1159751

Comment 6

2 years ago
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.
Attachment #8628514 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Given the incidence rate of crashes due to WARP I'm tracking for 40 and 41.
status-firefox39: --- → wontfix
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox42: --- → affected
tracking-firefox40: --- → +
tracking-firefox41: --- → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3cfb1740978
status-firefox41: affected → fixed
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.
relnote-firefox: --- → ?
Comment on attachment 8628514 [details] [diff] [review]
Disable WARP for Windows 7 on Aurora.

Just like Ritu said.
Attachment #8628514 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8628514 - Flags: approval-mozilla-beta+
(Assignee)

Comment 12

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

Carrying review from bug 1159751.
Attachment #8633525 - Flags: review+
(Assignee)

Comment 13

2 years ago
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
Attachment #8628514 - Flags: approval-mozilla-beta?
(Assignee)

Comment 14

2 years ago
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
Attachment #8633525 - Flags: approval-mozilla-beta?
Attachment #8633525 - Flags: approval-mozilla-aurora?
(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).
Bas, will this patch apply to dev edition?
Flags: needinfo?(bas)
(Assignee)

Comment 17

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

It should.
Flags: needinfo?(bas)
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.
Attachment #8633525 - Flags: approval-mozilla-beta?
Attachment #8633525 - Flags: approval-mozilla-beta+
Attachment #8633525 - Flags: approval-mozilla-aurora?
Attachment #8633525 - Flags: approval-mozilla-aurora+
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.
Attachment #8628514 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8628514 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Attachment #8633525 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?

Comment 21

2 years ago
(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 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.
Attachment #8633525 - Flags: approval-mozilla-beta?
Attachment #8633525 - Flags: approval-mozilla-aurora+
Blocks: 1159139
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.
Attachment #8628514 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Created attachment 8636600 [details] [diff] [review]
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
Attachment #8633525 - Attachment is obsolete: true
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.
Attachment #8628514 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
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+
Attachment #8628514 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
status-firefox40: affected → fixed
Blocks: 1183534
(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

2 years ago
This disabling of WARP on Win7 didn't only make the shutdownhangs of bug 1159751 go away, it also reduced OOMs a lot.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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

2 years ago
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #30)
> (In reply to Robert Kaiser (:kairo@mozilla.com) 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
Duplicate of this bug: 1188831
Added to FF41 release notes in Nucleus.
relnote-firefox: ? → 41+
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.
Flags: needinfo?(dvander)
Summary: Do not use WARP on Windows 7 → Disable WARP on Windows 7 for FF40, FF41
(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+.
Flags: needinfo?(dvander)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Duplicate of this bug: 1200537

Comment 37

2 years ago
(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.
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.
See Also: → bug 1186755
You need to log in before you can comment on or make changes to this bug.