Closed Bug 1171682 Opened 9 years ago Closed 9 years ago

Disable accelerated WebGL in Safe Mode

Categories

(Core :: Graphics, defect)

Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 + fixed
firefox40 + fixed
firefox41 --- fixed

People

(Reporter: jrmuizel, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Accelerated WebGL isn't safe
If it comes in handy - there is gfxPlatform::InSafeMode() method we can use.
layers.acceleration.disabled is the preference "Use hardware acceleration if available" checkbox.  The code that checks that preference, also takes the same action if the safe mode is on.  Accelerated WebGL checks neither.
Summary: Disable accelerated WebGL in Safe Mode → Disable accelerated WebGL in Safe Mode or when layers.acceleration.disabled is set
OS: Unspecified → All
Benoit, can you get us a quick patch for this?  I'd like it uplifted to 39 and 40 and the timing is very tight.
Assignee: jmuizelaar → bgirard
Flags: needinfo?(bgirard)
So I tried the obvious thing. It doesn't work. Turns out it's because e10s content process don't currently get the safe mode state.

So in this bug we will land a patch assuming that it works fine and uplift to 39 and 40.

In a follow-up e10s bug we can look into how to handle safe mode checks for e10s content processes.
Flags: needinfo?(bgirard)
Depends on: 1174857
Ideally we want this to fallback to software webgl when that is ready: bug 1167651
Attachment #8622697 - Flags: review?(jgilbert)
Attachment #8622697 - Flags: review?(jgilbert) → review+
Turning off WebGL in safe mode is pretty simple.

What should we do when 'layers.acceleration.disabled' is set? If we have WebGL software maybe we want to fallback to that like the bug says. If we don't have software webgl then what should we do? Have no WebGL or stay on accelerated.

Keep in mind that we have a lot of users that disable OGL because of certain driver/performance issue but can still run WebGL fine, or just because it's debugging response to a bug. If we do that we run the risk of lowering our WebGL usage. That may be ok, just want us to know that going in.
Keywords: leave-open
[Tracking Requested - why for this release]: Tracking per Comment 3. This would be a low risk change and provide some slight stability benefit.
I feel like overloading layers.acceleration to also impact webgl is a bad idea. webgl.disabled already exists. Each feature has their own disable flag and it allows us to test combo that exist in the while easily like software layers and hardware WebGL.

If this is something we really want, and I'm not sure it is, we should maybe introduce something like gfx.acceleration.disabled which would disable hardware acceleration for layers, webgl and could even disable things like hardware video decoders.

TL;DR: I suggest we only disable webgl in safe mode and do nothing else.
Flags: needinfo?(jmuizelaar)
(In reply to Milan Sreckovic [:milan] from comment #3)
> Benoit, can you get us a quick patch for this?  I'd like it uplifted to 39
> and 40 and the timing is very tight.

Are you sure we want part 1 in 39/Beta? I don't see any user impact linked on this bug and we're been shipping this for years. On the other hand the patch is trivial so it's not a big deal to uplift.
Flags: needinfo?(milan)
(In reply to Benoit Girard (:BenWa) from comment #11)
> I feel like overloading layers.acceleration to also impact webgl is a bad
> idea. webgl.disabled already exists. Each feature has their own disable flag
> and it allows us to test combo that exist in the while easily like software
> layers and hardware WebGL.
> 
> If this is something we really want, and I'm not sure it is, we should maybe
> introduce something like gfx.acceleration.disabled which would disable
> hardware acceleration for layers, webgl and could even disable things like
> hardware video decoders.
> 
> TL;DR: I suggest we only disable webgl in safe mode and do nothing else.

I agree.
Flags: needinfo?(jmuizelaar)
Yes to putting this on 39.  Safemode needs to be safer.

As for the other topic - we already have a problem - we have a prominent user interface item that says "Use hardware acceleration when available", and if checked off, we use hardware acceleration for WebGL.  That doesn't feel like "don't use hardware acceleration".

I will spin off another bug for this, as it may take more discussion.
Flags: needinfo?(milan)
I imagine we don't need "leave-open" anymore?
Summary: Disable accelerated WebGL in Safe Mode or when layers.acceleration.disabled is set → Disable accelerated WebGL in Safe Mode
(In reply to Milan Sreckovic [:milan] from comment #14)
> I will spin off another bug for this, as it may take more discussion.

Please, lets decouple these decision. The safemode bit is a no brainer.
Keywords: leave-open
Comment on attachment 8622697 [details] [diff] [review]
part 1: Trival case + uplift candidate

Approval Request Comment
[Feature/regressing bug #]: This bug, disabling a feature in safe mode.
[User impact if declined]: Higher risk in safe mode
[Describe test coverage new/current, TreeHerder]: We'd need to add a 'safe' variant to our test matrix. I don't think this is realistic.
[Risks and why]: Fairly low, we already support disabling safe mode. The patch is very simple.
[String/UUID change made/needed]: none
Attachment #8622697 - Flags: approval-mozilla-beta?
Attachment #8622697 - Flags: approval-mozilla-aurora?
Adding a tracking flag for Firefox40. We should take this fix as it is making Firefox safer when running in safe mode. 

Milan, I was reading some MDN documentation on WebGL and there was a mention of webgl.disable_extensions (A Boolean property that, when true, disables all WebGL extensions. This is false by default). I am just wondering, if WebGL extensions also need to be disabled separately. Thoughts?
Flags: needinfo?(milan)
For safe mode? This patch should be sufficient.
Flags: needinfo?(milan)
Comment on attachment 8622697 [details] [diff] [review]
part 1: Trival case + uplift candidate

Approved for upplift to beta and aurora.
Attachment #8622697 - Flags: approval-mozilla-beta?
Attachment #8622697 - Flags: approval-mozilla-beta+
Attachment #8622697 - Flags: approval-mozilla-aurora?
Attachment #8622697 - Flags: approval-mozilla-aurora+
Backout: 
https://hg.mozilla.org/releases/mozilla-beta/rev/f069ca975718

I didn't notice that gfxPlatform::InSafeMode() was missing from the beta branch.

I'll do a try run
Waiting on try to open
(In reply to Carsten Book [:Tomcat] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/f4367d79d062

Let's move any follow-up work to a new bug to avoid a tracking nightmare.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.