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

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: vlad, Assigned: bjacob)

Tracking

unspecified
All
Mac OS X
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [sg:moderate][softblocker])

Attachments

(3 attachments)

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.
(Assignee)

Comment 1

7 years ago
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.
Sure, making the changes now works.
(Assignee)

Comment 3

7 years ago
Created attachment 509866 [details] [diff] [review]
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.
Assignee: nobody → bjacob
Whiteboard: [hardblocker]
(Assignee)

Comment 4

7 years ago
https://crash-stats.mozilla.com/report/index/bp-a948c030-742b-46d7-929f-891be2110208
Whiteboard: [hardblocker] → [softblocker]
Per Vlad this could only lead to data leakage, not arbitrary code execution. Marking sg:moderate.
Whiteboard: [softblocker] → [sg:moderate][softblocker]
(Assignee)

Comment 6

7 years ago
Created attachment 511832 [details] [diff] [review]
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().
Attachment #511832 - Flags: review?(vladimir)
(Assignee)

Comment 7

7 years ago
ping, it would be nice to get this some real world testing i.e. land it soonish
Attachment #511832 - Flags: review?(vladimir) → review+
(Assignee)

Comment 8

7 years ago
Created attachment 514249 [details] [diff] [review]
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.
Attachment #514249 - Flags: review?(vladimir)
(Assignee)

Comment 9

7 years ago
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 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.
Attachment #514249 - Flags: review?(vladimir) → review+
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.
(Assignee)

Updated

7 years ago
Summary: OS X WebGL crash with too-small attrib 0 → OS X WebGL crash with too-small attrib 0 + optimizations
http://hg.mozilla.org/mozilla-central/rev/3ac1a2ffa751
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.