1402372 - (CVE-2017-7845) heap buffer overflow in VertexBuffer9 (ANGLE)

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Omair]

Attachment: D3D9.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

I have attached the same PoC (Bug 1398381) which can be used to trigger a heap overflow in ANGLE. 
(You can disable hardware acceleration and test.)

I have tested this on: 58 nightly build (should be applicable to the current stable build as well)

libglesv2.dll!memcpy + ? (the exact offset is not known) (id: 72d)
libglesv2.dll!rx::VertexBuffer9::storeVertexAttributes + 0x1D4 (id: 791) [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\vertexbuffer9.cpp @ 109]]
libglesv2.dll!rx::StreamingVertexBufferInterface::storeDynamicAttribute + 0x204 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexbuffer.cpp @ 173]]
libglesv2.dll!rx::VertexDataManager::storeDynamicAttrib + 0x240 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexdatamanager.cpp @ 480]]
libglesv2.dll!rx::VertexDataManager::storeDynamicAttribs + 0x25A [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexdatamanager.cpp @ 400]]
libglesv2.dll!rx::VertexDataManager::prepareVertexData + 0x2DA [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\vertexdatamanager.cpp @ 275]]
libglesv2.dll!rx::Renderer9::applyVertexBuffer + 0x5E [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\renderer9.cpp @ 1285]]
libglesv2.dll!rx::Renderer9::genericDrawElements + 0x1A8 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\renderer9.cpp @ 2984]]
libglesv2.dll!rx::Context9::drawElements + 0x47 [[z:\build\build\src\gfx\angle\src\libangle\renderer\d3d\d3d9\context9.cpp @ 157]]
libglesv2.dll!gl::Context::drawElements + 0x33 (inlined function) [[z:\build\build\src\gfx\angle\src\libangle\context.cpp @ 1654]]
libglesv2.dll!gl::DrawElements + 0xB2 [[z:\build\build\src\gfx\angle\src\libglesv2\entry_points_gles_2_0.cpp @ 791]]
xul.dll!mozilla::gl::GLContext::raw_fDrawElements + 0x1A (inlined function) [[z:\build\build\src\gfx\gl\glcontext.h @ 1101]]
xul.dll!mozilla::gl::GLContext::fDrawElements + 0x26 [[z:\build\build\src\gfx\gl\glcontext.h @ 1115]]
xul.dll!mozilla::WebGLContext::DrawElements + 0x13F [[z:\build\build\src\dom\canvas\webglcontextdraw.cpp @ 811]]
xul.dll!mozilla::dom::WebGLRenderingContextBinding::drawElements + 0xB6 [[z:\build\build\src\obj-firefox\dom\bindings\webglrenderingcontextbinding.cpp @ 16256]]
xul.dll!mozilla::dom::GenericBindingMethod + 0x12A [[z:\build\build\src\dom\bindings\bindingutils.cpp @ 3061]]
xul.dll!js::CallJSNative + 0xA3 (inlined function) [[z:\build\build\src\js\src\jscntxtinlines.h @ 293]]
xul.dll!InternalCall + ? (the exact offset is not known) [[z:\build\build\src\js\src\vm\interpreter.cpp @ 495]]
xul.dll!js::CallFromStack + 0xD (inlined function) [[z:\build\build\src\js\src\vm\interpreter.cpp @ 546]]
xul.dll!js::jit::DoCallFallback + 0x24A [[z:\build\build\src\js\src\jit\baselineic.cpp @ 2589]]
0x2E8E572D08E (no function symbol available)
0x18453FC480 (no function symbol available)
0x18453FC1B0 (no function symbol available)
0xFFFE01216EA6D940 (no function symbol available)
0xEEE4421FD838 (no function symbol available)
0x18453FC430 (no function symbol available)
0x18453FC3E8 (no function symbol available)
0x4 (no function symbol available)
0x18453FC3E8 (no function symbol available)

Comment 1

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

Peter, can you find somebody to take a look?

Comment 2

[Peter Chang[:pchang]]

Michael, please check this with ANGEL update.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

 1:50.79 INFO: Last good revision: 099f695d31326c39595264c34988a0f4b7cbc698 (2015-11-25)
 1:50.79 INFO: First bad revision: c321d84038519dcf1670d59fd2c5c00ad8a85a55 (2015-11-26)
 1:50.80 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=099f695d31326c39595264c34988a0f4b7cbc698&tochange=c321d84038519dcf1670d59fd2c5c00ad8a85a55

Two WebGL-related pushes in that range:
https://hg.mozilla.org/mozilla-central/rev/bd2c8c547222
https://hg.mozilla.org/mozilla-central/rev/7d1c223f397c

Comment 4

[Michael Leu[:Lenzak](UTC+8)]

Updating ANGLE to newer version is still affected by this issue.

And Chrome does not crash, so there must be something wrong in our Webgl context.

Comment 5

[Michael Leu[:Lenzak](UTC+8)]

Attachment: (WIP)Bug 1402372 - Block illegal DrawElements

I observed what chrome do when they load the same webpage, it seems  that the DrawElements is not legal and will be blocked as INVALID_OPERATION, I add same validation code in our WebGL code, the crash does disappear after this modification.

Comment 6

[Michael Leu[:Lenzak](UTC+8)]

This is the validation code which blocks the DrawElements command in Chrome.

https://cs.chromium.org/chromium/src/gpu/command_buffer/service/vertex_attrib_manager.cc?type=cs&q=attempt+to+draw+with+all+attributes+having+non-zero+divisors&sq=package:chromium&l=282

Comment 7

[Michael Leu[:Lenzak](UTC+8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62f1311c0f367eb473c9af590d6a5f80b1c339f5&selectedJob=133796181

Try looks good!

Comment 8

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

How about this?

Comment 9

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8913506 [details] [diff] [review]
0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

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

LGTM, this should block illegal Draw calls.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8913506 [details] [diff] [review]
0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

This has issues still.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Easy enough to construct a non-security bug in parallel: bug 1404196

We can do sec approval for uplift and what-not in this bug.

Comment 12

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8913506 [details] [diff] [review]
0001-Bug-1402372-Always-check-that-we-have-a-non-zero-ver.patch

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

LGTM, this should block illegal Draw calls.

Comment 13

[Kelsey Gilbert [:jgilbert]]

So the parallel bug is leaving open a potential case: instanced attribs with generic (non-array) attrib values.
I'll make a testcase for this.

Comment 14

[Julien Cristau [:jcristau]]

tracking as sec-critical.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Fixed in 58: https://bugzilla.mozilla.org/show_bug.cgi?id=1404196

Comment 16

[Kelsey Gilbert [:jgilbert]]

This was actually the d3d9 version of bug 1376399.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

Comment 18

[Kelsey Gilbert [:jgilbert]]

We're going to want to fix this in esr52 and probably release, when we get a chemspill opportunity.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this affected for 57 so that it will show up on relman and security triage queries. 

We also could uplift now, for an RC2 build later this week. 

Since I haven't started the ESR build yet, it could make ESR as well if we land patches tomorrow morning. 
Ritu, dveditz, fyi, depending on sec-approval and how risky this looks for 57 we can either plan this for dot release, or get it in this week.

Comment 20

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

Without sec-approval, risk assessment and how easily this can be exploited, my guess is this will likely not be in 57 RC. Let's wait to hear back from Dan and the other info from the uplift request.

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to Jeff Gilbert [:jgilbert] from comment #16)
> This was actually the d3d9 version of bug 1376399.

Should I worry that the ANGLE update in bug 1371190 seemed to revert the d3d11 fix in that bug back to passing '0' instead of '1'? It's calling a different set of methods now, though:

  gl::Error Context11::drawArrays(const gl::Context *context, GLenum mode, GLint first, GLsizei count)
  {
      ANGLE_TRY(prepareForDrawCall(context, mode));
      return mRenderer->drawArrays(context, mode, first, count, 0);
  }

(Similar in the other places we changed.)

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to Ritu Kothari (:ritu) from comment #20)
> Without sec-approval, risk assessment and how easily this can be exploited,
> my guess is this will likely not be in 57 RC. Let's wait to hear back from
> Dan and the other info from the uplift request.

I'm not at all worried about taking this fix in 57 -- it's identical to the patch we took in bug 1376399 except for d3d9 instead of d3d11. I am a little worried about whether we regressed that other fix (see comment21) and might have to fix that, too, but I don't have the hardware to test it right now.

Comment 23

[Daniel Veditz [:dveditz]]

This hurts. We initially found and fixed the d3d11 version on our own, but didn't identify it as a security bug. That means we didn't think about backporting it to ESR-52 (bug 1398381) and we didn't look at the parallel code for d3d9 (this bug).

Comment 24

[Kelsey Gilbert [:jgilbert]]

(In reply to Daniel Veditz [:dveditz] from comment #21)
> (In reply to Jeff Gilbert [:jgilbert] from comment #16)
> > This was actually the d3d9 version of bug 1376399.
> 
> Should I worry that the ANGLE update in bug 1371190 seemed to revert the
> d3d11 fix in that bug back to passing '0' instead of '1'? It's calling a
> different set of methods now, though:
> 
>   gl::Error Context11::drawArrays(const gl::Context *context, GLenum mode,
> GLint first, GLsizei count)
>   {
>       ANGLE_TRY(prepareForDrawCall(context, mode));
>       return mRenderer->drawArrays(context, mode, first, count, 0);
>   }
> 
> (Similar in the other places we changed.)

Yes, my god. New rule: No ANGLE patches without regression tests.

Comment 25

[Kelsey Gilbert [:jgilbert]]

Though actually I believe we're safe now that we skip making the actual call if `!vertCount||!instanceCount`, and always validate non-instanced draw calls as instanceCount=1 in FF58.

Comment 26

[Kelsey Gilbert [:jgilbert]]

(In reply to Daniel Veditz [:dveditz] from comment #22)
> (In reply to Ritu Kothari (:ritu) from comment #20)
> > Without sec-approval, risk assessment and how easily this can be exploited,
> > my guess is this will likely not be in 57 RC. Let's wait to hear back from
> > Dan and the other info from the uplift request.
> 
> I'm not at all worried about taking this fix in 57 -- it's identical to the
> patch we took in bug 1376399 except for d3d9 instead of d3d11. I am a little
> worried about whether we regressed that other fix (see comment21) and might
> have to fix that, too, but I don't have the hardware to test it right now.

I do confirm that the d3d11 bug remains fixed in FF58, and the ANGLE patch(es) are not needed there, because of our new validation. The D3D11 bug is also fixed in FF57 release build 4, but this D3D9 version still repros there, as expected.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit
User impact if declined: Poorly written WebGL code might crash.
Fix Landed on Version: 58+
Risk to taking this patch (and alternatives if risky): Very low, since we've taken this already for d3d11.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This is more of a vector than a vuln, so it'd probably still be tough. The vector here is clear though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
ESR52, 57

If not all supported branches, which bug introduced the flaw?
ANGLE update

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes.

How likely is this patch to cause regressions; how much testing does it need?
Our CI tests cover this.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

57 already shipped. Unless this is chemspill worthy, we're not going to take it on 57 and re-release.

This is fixed in 58 and, therefore, fixed in 59, right? So we're only talking about ESR52 for 52.6 (next release) at this point?

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Al Billings [:abillings] from comment #28)
> 57 already shipped. Unless this is chemspill worthy, we're not going to take
> it on 57 and re-release.
> 
> This is fixed in 58 and, therefore, fixed in 59, right? So we're only
> talking about ESR52 for 52.6 (next release) at this point?

I'm theoretically OK with letting this go unfixed in 57, but it would be nice to deploy this fix if we chemspill for another reason.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Ritu - FYI as 57 release owner, something we may want to consider for a dot release.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Note also we support ESR52 through the end of June 2018 (52.8.0). So that may weigh more heavily in favor of taking this in ESR at some point, whether that's for 52.5.1 or 52.6.0.

Comment 32

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

Thanks Liz! I'll add this to my 57.x dot release ride-alongs though Al's comment 28 makes me feel it might be a wontfix for all of 57. Let's review risks, reward, security team's opinion when we are closer to doing a dot release for 57.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

Clearing sec-approval since this is really a branch issue at this point.

Comment 34

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

Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

DVeditz agrees that this would be a low-risk ride-along for 57.0.1. Release57+

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/eb55b678d70e

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8925741 [details] [diff] [review]
0001-Bug-1402372-Fix-instancing-count-for-non-instanced-c.patch

Fix for sec-critical issue, let's take this for ESR 52.5.1.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e4d839e583f88e318e24fef8e6efa7ff18d7b565

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Jeff, this is hitting WinXP debug asserts on ESR52.
https://treeherder.mozilla.org/logviewer.html#?job_id=148283375&repo=mozilla-esr52

Is this something that should block the 52.5.1 release?

Comment 39

[Andrei Vaida [:avaida]]

(In reply to Jeff Gilbert [:jgilbert] from comment #27)
> How likely is this patch to cause regressions; how much testing does it need?
> Our CI tests cover this.

Since this patch has automated coverage, I'm flagging it as not needing manual testing.

Jeff, if you think otherwise, please ni? me.

Comment 40

[Georg Koppen]

Looking at the PoC I got wondering whether one actually hits the problematic code path if one just allows WebGL in minimum capability mode (by setting `webgl.min_capability_mode` to `true`) and disables things like extensions (by setting `webgl.disable-extensions`, to `true`). Or is the bug in question generic enough that it hits the users as soon as any kind of WebGL is allowed?

Comment 41

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

We should probably block 52.5.1 while we sort this out; may need to back out some stuff out.

Comment 42

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

We will not push ESR52.5.1 until the test failure is all clear, I just emailed r-d.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Note that we're also shipping this in 57.0.1 and I believe Al's intending to publish an advisory for it. We should probably delay that then too.

To recap, Windows XP debug (and *only* Windows XP debug) is hitting asserts in the mochitest-gl test suite. We aren't seeing them on opt builds or on other Windows platforms. What isn't clear to me is what test coverage we have for this code path anywhere outside of ESR52 - are our Win7 machines (which have SP1 and no platform update) also hitting the d3d9 path, or are they on d3d11?

It's really unclear what the severity is here, especially with opt tests passing. FWIW, when I run 52.5.1 locally on my Win10 machine with webgl.angle.try-d3d11 set to false (which IIUC forces d3d9 mode on ANGLE), I was able to run various WebGL demos on https://experiments.withgoogle.com/chrome?tag=WebGL without any obvious issues.

Comment 44

[Kelsey Gilbert [:jgilbert]]

I'm trying solutions, but having a hell of a time building esr52 or pushing to try over git.

Comment 45

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1402372-Always-use-instanced-drawing-when-enable.patch

I'll be on a flight, and can't build esr52 anyways at the moment.
Can someone build and test this?

Comment 46

[Ryan VanderMeulen [:RyanVM]]

I'll attempt to run this through Try. Jeff, can you please summarize what the issue is here so we have a better idea of what your patch is fixing? Does it affect all branches? Is it a real user-facing issue?

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8933419 [details] [diff] [review]
0001-Bug-1402372-Always-use-instanced-drawing-when-enable.patch

Try says this doesn't work :(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c21121e38f60df9f13ba73e6228a68c4037e0e55&group_state=expanded

Assertions are still there on XP and it actually causes new failures in /test_2_conformance__rendering__point-no-attributes.html on Win7/Win8.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8933419 [details] [diff] [review]
0001-Bug-1402372-Always-use-instanced-drawing-when-enable.patch

Sorry, I misunderstood that this patch depends on the earlier patch being backed out. Here's a new Try run for that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2a2d8bec27350c016debfcf0a7a42520822f07&group_state=expanded

Comment 49

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch

This should fix the no-attrib drawing tests.

Comment 50

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8933467 [details] [diff] [review]
0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch

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

::: dom/canvas/WebGLContextDraw.cpp
@@ +998,5 @@
>      }
>  
> +    if (mActiveProgramLinkInfo->attribs.size() && !hasPerVertex) {
> +        /* http://www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt
> +         *  If all of the enabled vertex attribute arrays that are bound to active

The reasoning doesn't seem strong to me: one could fetch per-vertex positions from textures or generate in code with `gl_VertexID`, so the case should be valid.

Comment 51

[Kelsey Gilbert [:jgilbert]]

(In reply to Dzmitry Malyshau [:kvark] from comment #50)
> Comment on attachment 8933467 [details] [diff] [review]
> 0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch
> 
> Review of attachment 8933467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextDraw.cpp
> @@ +998,5 @@
> >      }
> >  
> > +    if (mActiveProgramLinkInfo->attribs.size() && !hasPerVertex) {
> > +        /* http://www.khronos.org/registry/gles/extensions/ANGLE/ANGLE_instanced_arrays.txt
> > +         *  If all of the enabled vertex attribute arrays that are bound to active
> 
> The reasoning doesn't seem strong to me: one could fetch per-vertex
> positions from textures or generate in code with `gl_VertexID`, so the case
> should be valid.

The logic for the restrictions comes pretty directly from that spec. It's to appease D3D9 (I believe) which requires at least one non-instanced vertex attrib.

Comment 52

[Kelsey Gilbert [:jgilbert]]

Most recent:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6d89b7abd2c44a17070428a981fe905c675ccfa

Comment 53

[Ryan VanderMeulen [:RyanVM]]

That push won't give you the buildbot jobs you need for WinXP. Here's a push that has those:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00b6f7856f21783fd70f66452d9e06f4ef8ad7bd

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Except that only has the second patch. Here's one that has both on top of a backout of the first.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=865188c01c4d10a7fc6c551b5543014fff97a0b6

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8933467 [details] [diff] [review]
0002-Bug-1402372-Always-check-for-active-vertex-attrib-di.patch

Failing on all platforms:
https://treeherder.mozilla.org/logviewer.html#?job_id=148977700&repo=try

Failing on Win7+:
https://treeherder.mozilla.org/logviewer.html#?job_id=148977484&repo=try

The Try push was ESR52 tip with a backout of comment 37, attachment 8933419 [details] [diff] [review], and attachment 8933467 [details] [diff] [review] applied on top.

Comment 56

[Kelsey Gilbert [:jgilbert]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=e22b18ba10ad945b10ab6611859d21a829094d18

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Ryan, from the email thread, we want to backout the fix. 
If you agree, could you please do that?

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Backed out from release and ESR52. We'll target Fx58 and ESR 52.6.0 for shipping this fix instead.

https://hg.mozilla.org/releases/mozilla-release/rev/15bb6c3fb5875c7f39aef036dd161df1407b5ee3
https://hg.mozilla.org/releases/mozilla-esr52/rev/c05ea3d4321bce6c8d6ab8460eec44b277710f1b

Comment 59

[Georg Koppen]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #58)
> Backed out from release and ESR52. We'll target Fx58 and ESR 52.6.0 for
> shipping this fix instead.
> 
> https://hg.mozilla.org/releases/mozilla-release/rev/
> 15bb6c3fb5875c7f39aef036dd161df1407b5ee3
> https://hg.mozilla.org/releases/mozilla-esr52/rev/
> c05ea3d4321bce6c8d6ab8460eec44b277710f1b

I guess I am confused but it seems 57.0.1 got out and includes the fix. So, "wontfix" seems wrong. Or am I missing something?

Comment 60

[Ryan VanderMeulen [:RyanVM]]

We're building 57.0.2 with it backed out.

Comment 61

[Al Billings [:abillings - ex-MoCo]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #60)
> We're building 57.0.2 with it backed out.

I argued against doing this.

Comment 62

[Kelsey Gilbert [:jgilbert]]

Attachment: reduced-testcase.html

Comment 63

[Kelsey Gilbert [:jgilbert]]

Of note is that we updated ANGLE in 51, 55, and 58. The update in 55 is a point of uncertainty between release57 and esr52.

Comment 64

[Kelsey Gilbert [:jgilbert]]

Attachment: webgl-torture-attribs.html

Here's a testcase that'll show whether 0-attrib rendering works. (or asserts)

Comment 65

[Kelsey Gilbert [:jgilbert]]

FWIW, release57 asserts in my local build, running webgl-torture-attribs. After finishing here, I will look at why release57 wasn't seeing asserts in our CI testcases for this.

Comment 66

[Kelsey Gilbert [:jgilbert]]

As such, I'm encouraged to ship the old fix with a fix-up of fixing the assert.

Comment 67

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Bug-1402372-DrawArrays-is-DrawArraysInstanced-1-in-D.patch

r+'d previously

Comment 68

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd949c0e37468c809297dbd5cbbe655208f050e&selectedJob=150099454

Comment 69

[Kelsey Gilbert [:jgilbert]]

This fix doesn't seem to regress on webgl-torture-attribs compared to esr52.5.0.

Comment 70

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/releases/mozilla-release/rev/04a4d110a17ef89b3d329eb517fdc9bbc5509fd3
 https://hg.mozilla.org/releases/mozilla-esr52/rev/af2615af2b2ef52c7b97c426277511b1f22e021a

Not sure how we annotate the bug for dot releases. :ritu?

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Do 58/59 need this revised patch still?

Comment 72

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

Updating the tracking flag to blocking since we held back 57.0.2/esr52.5.1 on this one. Thanks Jeff for pushing these fixes to m-r/m-esr52.

Comment 73

[Kelsey Gilbert [:jgilbert]]

ANGLE rejects the bad call in 58+.

Comment 74

[Georg Koppen]

Jeff, is the code path for this bug generic enough that users are affected as soon as they have any WebGL enabled or would it be fine for those that only have minimum capability mode on and no extensions and WebGL2 allowed? From looking at the PoC and various patch proposals in this bug it seems to me having all those things set/disabled would mitigate the problem. I am still trying to understand the impact of this bug for Tor Browser users. (see my comment 40)

Comment 75

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Our team can confirm that the initial issue is not reproducible anymore using 57.0.2-build2 (20171206182557)and 52.5.2esr-build2 (20171206101620) on Windows 10 x64, Windows 7 x64 and Ubuntu 16.04 x64. 
On macOS 10.13	(and Mac OS X 10.11.6) the 2 above mentioned builds trigger @ GLEngine@0x147a9d and @ mozilla::WebGLContext::GetError crashes (with and without hardware acceleration enabled) when using the testcase from comment 64 (the one from comment 62 is only opening a blank page). The 52.5.0esr (20171107091003) build has the same behavior. 
Any thoughts about this?

Comment 76

[Sylvestre Ledru [:Sylvestre]]

Added the follow up in the release notes with "Fix a regression with WebGL and D3D9 - Windows only" as wording.

Comment 77

[Kelsey Gilbert [:jgilbert]]

(In reply to Georg Koppen from comment #74)
> Jeff, is the code path for this bug generic enough that users are affected
> as soon as they have any WebGL enabled or would it be fine for those that
> only have minimum capability mode on and no extensions and WebGL2 allowed?
> From looking at the PoC and various patch proposals in this bug it seems to
> me having all those things set/disabled would mitigate the problem. I am
> still trying to understand the impact of this bug for Tor Browser users.
> (see my comment 40)

All users who are using instancing. I don't remember if instancing is supported by min-cap mode. (Instancing has ~100% support, otherwise)