https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-depth-texture.html Device ID: 0x fd5 GPU Accelerated Windows: 1/1 OpenGL Vendor ID: 0x10de WebGL Renderer: NVIDIA Corporation -- NVIDIA GeForce GT 650M OpenGL Engine

What OS and/or driver version?

Also, comment 0 says it's webgl-depth-texture.html but the callstack attachment has a different URL at the top. Which is it?

ProductName: Mac OS X ProductVersion: 10.8.2 BuildVersion: 12C3006 It's webgl-depth-texture.html

Driver Version: NVIDIA-8.6.22

Assigning to Milan for now, maybe bjacob will steal it though :)

Safari on this test: FAIL Unable to fetch WebGL rendering context for Canvas FAIL successfullyParsed should be true. Threw exception ReferenceError: Can't find variable: successfullyParsed

In Safari, WebGL isn't enabled by default. You have to go to the developer menu to enable it. It would be very interesting to know if it reproduces there. Another thing to do is APItrace it.

Right - with WebGL enabled, it admits it doesn't support that extension: PASS WebGL context exists Testing binding enum with extension disabled PASS gl.texImage2D(gl.TEXTURE_2D, 0, gl.DEPTH_COMPONENT, 1, 1, 0, gl.DEPTH_COMPONENT, gl.UNSIGNED_SHORT, null) generated expected GL error: INVALID_ENUM. PASS gl.texImage2D(gl.TEXTURE_2D, 0, gl.DEPTH_COMPONENT, 1, 1, 0, gl.DEPTH_COMPONENT, gl.UNSIGNED_INT, null) generated expected GL error: INVALID_ENUM. PASS No WEBGL_depth_texture support -- this is legal PASS WEBGL_depth_texture not listed as supported and getExtension failed -- this is legal PASS successfullyParsed is true TEST COMPLETE

Do we know if this is crash happens outside of OSX?

And, on OSX, is it just Nvidia graphics?

Benoit/Jeff, why is gl->IsExtensionSupported(GLContext::EXT_packed_depth_stencil) and indication that WEBGL_depth_texture is supported? Is there a better test, before we start special casing OSX (and Nvidia?)

Option 1, suggested by bjacob

Is this the right place instead?

(In reply to Milan Sreckovic from comment #11) > Benoit/Jeff, why is > gl->IsExtensionSupported(GLContext::EXT_packed_depth_stencil) and indication > that WEBGL_depth_texture is supported? Is there a better test, before we > start special casing OSX (and Nvidia?) Hm, so the test we have here is if (gl->IsGLES2() && gl->IsExtensionSupported(GLContext::OES_packed_depth_stencil) && gl->IsExtensionSupported(GLContext::OES_depth_texture)) So it is testing for OES_depth_texture as it should, but you're right, it also tests for OES_packed_depth_stencil and I have no idea why it does that. Jeff reviewed that patch (bug 738866) so he should know. Anyhow, this is only adding a _restriction_ on the condition to enable this extension, so it can't be responsible for enabling it when it shouldn't.

Comment on attachment 692433 [details] [diff] [review] Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported Review of attachment 692433 [details] [diff] [review]: ----------------------------------------------------------------- That is the right place.

Comment on attachment 692434 [details] [diff] [review] Blacklist OSX & Nvidia in WebGLContext::InitAndValidateGL Review of attachment 692434 [details] [diff] [review]: ----------------------------------------------------------------- The interesting --- I didn't know that we had MarkExtensionUnsupported. As said above, the extension to disable here would be the depth_texture extension since we do check for it too; but if we were to take that route, I'd rather do it directly in gfx/gl/GLContext.* since that isn't WebGL specific.

I meant: "Ah, Interesting!"

Wait a minute, what tree does your patch apply to ? In all the trees I looked at, the code looks like in comment 14, with the gl->IsExtensionSupported(GLContext::OES_depth_texture).

Comment on attachment 692433 [details] [diff] [review] Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported Sorry -- I was confused above, was looking at the wrong if(). Jeff, please comment on the rationale for this EXT_packed_depth_stencil test?

(In reply to Milan Sreckovic from comment #10) > And, on OSX, is it just Nvidia graphics? Tested on a Mac with AMD graphics, works fine. Crash is NVIDIA specific.

Removing qawanted, bjacob confirmed it runs on AMD.

(In reply to Benoit Jacob [:bjacob] from comment #19) > Comment on attachment 692433 [details] [diff] [review] > Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported > > Sorry -- I was confused above, was looking at the wrong if(). > > Jeff, please comment on the rationale for this EXT_packed_depth_stencil test? WEBGL_depth_texture requires both depth and depth-stencil textures. That said, WebGL actually requires depth-stencil in the core spec, so technically we need to blocklist WebGL on systems where this is not supported, until we can emulate it. This means that while we shouldn't check for {OES,EXT}_packed_depth_stencil, it's not because we don't need it, but because this functionality is required already by WebGL.

Comment on attachment 692433 [details] [diff] [review] Blacklist OSX & Nvidia in WebGLContext::IsExtensionSupported Review of attachment 692433 [details] [diff] [review]: ----------------------------------------------------------------- This should be done at the end of GLContext::InitExtensions.

Option 3, suggested by jgilbert

Comment on attachment 693020 [details] [diff] [review] Blacklist OSX & Nvidia in GLContext::InitExtensions Review of attachment 693020 [details] [diff] [review]: ----------------------------------------------------------------- Note: as I have never used MarkExtensionUnsupported, I suggest that you run this locally to verify that it does take effect. ::: gfx/gl/GLContext.cpp @@ +599,5 @@ > + // to properly support this. See 814839 > + if (WorkAroundDriverBugs() && > + Vendor() == gl::GLContext::VendorNVIDIA) { > + MarkExtensionUnsupported(gl::GLContext::EXT_packed_depth_stencil); > + } Just a style nit: after a multi-line if() condition, the opening curly brace '{' should be on a new line.

Applying nit comments to the r+ patch.

Comment on attachment 693086 [details] [diff] [review] Updated with code review comments (trivial) [Security approval request comment] How easily can the security issue be deduced from the patch? Nothing specific, aside from "there is a security problem with depth textures on Mac NVIDIA", can be deduced from the patch, as this patch just disables this functionality on this driver. I think it's safe to land now. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, they just show that there exists a security issue with depth textures on this driver. Which older supported branches are affected by this flaw? All branches >= 17 when bug 738866 landed, exposing this functionality. If not all supported branches, which bug introduced the flaw? Bug 738866. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be simple enough to backport, not necessarily trivial if MarkExtensionUnsupported wasn't available yet in some branch, but still simple. How likely is this patch to cause regressions; how much testing does it need? Safe, won't cause regressions. Only testing it needs is verify that it does disable depth textures on Mac NVIDIA.

Adding release management so we can discuss when we want to get this in since it affects 17 and later.

As long as there won't be a major perf hit, we're good to land on all branches given the low risk of regression. Please prepare an ESR17 patch as well.

I'll put together the backports.

In comments 28 and 29 did you mean to grant sec-approval? Or should we wait a bit longer before landing on central? (My recommendation is not to wait as this fix is not giving away a precise vulnerability).

Comment on attachment 693086 [details] [diff] [review] Updated with code review comments (trivial) Giving explicit sec-approval+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f731147e6c https://hg.mozilla.org/releases/mozilla-aurora/rev/3d1343748fc7 https://hg.mozilla.org/releases/mozilla-beta/rev/f2fcf38b2b56 https://hg.mozilla.org/releases/mozilla-esr17/rev/5e7c575794d8

Confirmed crash on 17.0.1 ESR, confirmed fix on 17.0.2 ESR.

Confirmed crash on trunk, 2012-11-24 Confirmed fixed on trunk, 2013-01-09 Confirmed fixed on Aurora, 2013-01-09 Confirmed fixed on beta, 2013-01-09

This does not reproduce for me on 10.8.3 with a GeForce 9400M. Also, I believe this fix was over-cautious, and we should have only blocked depth_texture on the affected machines. I filed bug 908905 for this.