631420 - OS X WebGL crash with too-small attrib 0 + optimizations

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

From Gregg on the webgl mailing list; we'll have to do the same:

The steps to reproduce are:

1) assign a small buffer to attrib 0
2) assign indices for the small buffer to ELEMENT_ARRAY_ATTRIB
3) assign a large buffer to attrib 1
4) enable attrib 0
5) drawElements for small buffer with program that only uses attrib 0
6) enable attrib 1
7) drawArray for large buffer with program that only uses attrib 1

I checked in a conformance test to reproduce it
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/gl-vertex-attrib-zero-issues.html

Run it on OSX and at least for me (10.6.4) it crashes or renders garbage.

If we understand the bug correctly it's that the second draw has a large count and even though attrib 0 is not being consumed by the program something in the OSX drivers require it to be large enough for the draw.

This only seems to effect attrib 0. Switch to attribs 1 and 2 and the problem goes away.

The solution we are planning to implement is to take advantage of the Attrib 0 simulation code already in our implementations to provide a buffer large enough for Attrib 0 at all times. A minor optimization is that it doesn't matter what's in the buffer since it's not actually being used.  That seems to fix the issue.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

OK, we had to make a change in the attrib 0 simulation code anyway, I guess this is the occasion.

Remember how the webkit implementations uses a persistent VBO while we recreate the array on every draw call. Andor showed me a demo that was using two constant (not changing over time) VBOs: attrib 1 for vertex positions, and attrib 0 for vertex colors, and had a checkbox to enable/disable colors. With colors disabled, it killed our performance because we had to do the simulation on every draw call, while webkit was 2x faster because it just reused its VBO.

Since the change that you discuss above is also very prone to using a VBO (since there we don't care about the contents), maybe it's a good idea to do the two changes at once.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Sure, making the changes now works.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: trivial patch to confirm theory (do not check in, performance disaster)

If you want to validate your theory, check if this patch makes crashes go away on OSX.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

https://crash-stats.mozilla.com/report/index/bp-a948c030-742b-46d7-929f-891be2110208

Comment 5

[Johnny Stenback (:jst)]

Per Vlad this could only lead to data leakage, not arbitrary code execution. Marking sg:moderate.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: work around osx bug + optimizations to avoid doing work when possible

There are 3 cases:
 * default case: nothing special needs to be done about attrib 0. This patch calls that VertexAttrib0Status::Default
 * case 2: attrib 0 needs to be emulated with a fully initialized array. This patch calls that VertexAttrib0Status::EmulatedInitializedArray
 * case 3: attrib 0 needs a big enough array, but doesn't actually needs its contents to be initialized. This patch calls that VertexAttrib0Status::EmulatedUninitializedArray

The new method WebGLContext::WhatDoesVertexAttrib0Need() determines what case we're in.

The actual emulation work stays in WebGLContext::DoFakeVertexAttrib0().

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

ping, it would be nice to get this some real world testing i.e. land it soonish

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: more aggressively optimized variant

How about this instead. See the interdiff. This variant basically keeps the existing VBO alive when it's not needed or bigger than needed, instead of destroying/recreating it. So there is a (gpu) memory usage penalty, but it's only for the lifetime of the WebGL context anyway, and in exchange for that we get both performance, and less (gpu) memory allocation churning.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

The rationale here is that in typical cases where this makes any difference, this replaces 'intermittent' memory usage (allocation and deallocation in every frame rendering) by 'steady' memory usage, so this isn't a real memory usage increase.

The only case where this would have a great cost is if some WebGL app initially required some big emulated attrib 0, and then kept running for a long time no longer requiring a big attrib 0.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 514249 [details] [diff] [review]
more aggressively optimized variant

Hmm, any reason for doing this?  At the very least we should delete if the existing buffer is larger than, say, 2x the needed one.  Otherwise one bad draw could cause a large buffer to get allocated and stick around forever.

Then again... most draws are in loops, so we'll likely end up needing this anyway.  OK.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Yes, that's basically my rationale. Very few WebGL apps do rendering with a big attrib once and never after, while still keeping the WebGL context around.

Also, regarding the '2x larger' suggestion, the problem is that a typical WebGL app (say a game) will be doing rendering with many different VBOs of very diverse sizes at every frame, so a '2x larger' trick would typically not prevent this VBO from being recreated very frequently.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/3ac1a2ffa751