1005658 - Don't pass stack pointers to the GL for buffers, and have GLContext try to guard against it

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Most users pass heap pointers, not stack pointers to the GL.

So when we pass a stack pointer, we are putting ourselves in a minority case.

Moreover we then have no more alignment than what is required to store individual elements. E.g. a GLfloat vertices[] array doesn't have more than 4 byte alignment... It's not hard to imagine how that could be surprising to a GL used to heap pointers and trying to copy client-side buffers fast.

The other problem with passing stack pointers to the GL is it makes it harder to reason about crashes involving the GL and stack values.

Comment 1

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

Oh and I mean, GL entry points taking buffers, such as glBufferData, glReadPixels, glTexImage2D.

It's OK to pass stack pointers to glGetIntegerv.

Comment 2

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

Attachment: Don't pass stack arrays to the GL

This patch:

1) Removes MOZ_ENABLE_GL_TRACKING and instead uses DEBUG, which is synonym of it. Reason: comments about #else / #endif statements in GLContext.h were mentioning the wrong one, and that confused me. Since MOZ_ENABLE_GL_TRACKING is defined if and only if DEBUG is, it seems like a useless duplicate anyway.

2) Adds the assertion discussed above, with just the aggressiveness required for it to catch the CompositorOGL::Initialize case on my system.

3) Adds a  HeapCopyOfStackArray helper to be able to easily convert stack arrays into heap arrays, to be able to retain the convenience of plain old static arrays and not have to introduce more custom dangerous MOZ_ARRAY_LENGTH / sizeof usage.

4) Uses that in CompositorOGL::Initialize.

Comment 3

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

https://tbpl.mozilla.org/?tree=Try&rev=a83ae6f600ea

Comment 4

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

Attachment: additional fixups required (not asking for review...)

Comment 5

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

https://tbpl.mozilla.org/?tree=Try&rev=4f00793f49ae

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbef254f0aa5

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbef254f0aa5