1770235 - DrawBlitProg::Draw non-vao path doesn't restore bindBuffer for restore of vertexAttribPointer

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Kelsey Gilbert [:jgilbert]]

We do this:

  if (mParent.mQuadVAO) {
    [...]
  } else {
    // clang-format off
        gl->fGetVertexAttribiv(0, LOCAL_GL_VERTEX_ATTRIB_ARRAY_ENABLED, &vaa0Enabled);
        gl->fGetVertexAttribiv(0, LOCAL_GL_VERTEX_ATTRIB_ARRAY_SIZE, &vaa0Size);
        gl->fGetVertexAttribiv(0, LOCAL_GL_VERTEX_ATTRIB_ARRAY_TYPE, (GLint*)&vaa0Type);
        gl->fGetVertexAttribiv(0, LOCAL_GL_VERTEX_ATTRIB_ARRAY_NORMALIZED, &vaa0Normalized);
        gl->fGetVertexAttribiv(0, LOCAL_GL_VERTEX_ATTRIB_ARRAY_STRIDE, (GLint*)&vaa0Stride);
        gl->fGetVertexAttribPointerv(0, LOCAL_GL_VERTEX_ATTRIB_ARRAY_POINTER, &vaa0Pointer);
    // clang-format on

    gl->fEnableVertexAttribArray(0);
    const ScopedBindArrayBuffer bindVBO(gl, mParent.mQuadVBO); // A
    gl->fVertexAttribPointer(0, 2, LOCAL_GL_FLOAT, false, 0, 0);           // B
  }

  gl->fDrawArrays(LOCAL_GL_TRIANGLE_STRIP, 0, 4);

  if (mParent.mQuadVAO) {
    gl->fBindVertexArray(oldVAO);
  } else {
    if (vaa0Enabled) {
      gl->fEnableVertexAttribArray(0);
    } else {
      gl->fDisableVertexAttribArray(0);
    }
    // C?
    gl->fVertexAttribPointer(0, vaa0Size, vaa0Type, bool(vaa0Normalized),
                             vaa0Stride, vaa0Pointer); // D
  }

VertexAttribPointer uses the currently bound GL_ARRAY_BUFFER, hence why we have A before B in the code above. D is missing a similar line at C.

This means that when we don't have VAOs on a driver, and we call DrawBlitProg::Draw, the state of attrib 0 isn't fully restored.
WebGL 1 is the main thing that would be affected by this bug. I think we're generally safe otherwise. Conveniently, we disabled WebGL 1 on devices that are missing VAOs in bug 1322746.

Bug 1764589 hopes to re-enable it on these devices, so we need to get this right.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1396704

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1770235 - DrawBlitProg::Draw non-vao path must restore vaa0's buffer.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b862925febc
DrawBlitProg::Draw non-vao path must restore vaa0's buffer. r=gfx-reviewers,lsalzman

Comment 4

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6b862925febc

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jgilbert, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1770235 - Add CSP tests for ShadowRealms r?evilpie

Comment 7

[Phabricator Automation]

Comment on attachment 9287295 [details]
Bug 1770235 - Add CSP tests for ShadowRealms r?evilpie

Revision D152917 was moved to bug 1770234. Setting attachment 9287295 [details] to obsolete.