Ensure symbol shader attrib bound at 0 is always valid and used

[lbud]

Fixes #4607.
@jfirebaugh found the culprit in Firefox: binding an attrib to position 0 without using it "forces the browser to do expensive emulation work when running on desktop OpenGL platforms, for example on Mac." In this case, a_size was frequently being bound to position 0 and was not always used. This PR manually binds a_data (which is always used — either this or a_pos_offset would work) to position 0, and then lets the program dynamically bind the rest.

[jfirebaugh]

Ah, the old "don't disable attribute 0" gotcha. We've been bitten several times in the past by this. It would be good to come up with a test that fails if this regresses again. Would this have been hitting

mapbox-gl-js/src/render/vertex_array_object.js

Line 63 in 13363a2

assert(i !== 0);

but we just never caught it in a debug build?

In native our strategy is to explicitly bind everything with bindAttribLocation and a fixed order of attributes.

[lbud]

@jfirebaugh I haven't dug too far into it yet, but it appears it's not entering that branch because the OES_vertex_array_object (gl.extVertexArrayObject here) is present. Moreover, I'm not 100% clear on how the shader binding works but it appears that attrib positions are already determined by this point, having been dynamically/automatically assigned in painter.js.

It also appears that in Chrome on master when we rely on webgl to dynamically assign attribute locations (I think this happens in gl.linkProgram…?), a_data already happens to be assigned to position 0, whereas on Safari a_size is assigned to position 0, so I'm not sure that we'd even be able to reliably create a test for this, since we can't control browser environments on CI 🤕

It'd probably be a good idea to eventually take the same approach as native with explicit bindings.

[kkaefer]

I think this happens in gl.linkProgram…?

Yeah, if you don't define explicit bindings, it auto-assigns them when linking.

I'm not sure that we'd even be able to reliably create a test for this, since we can't control browser environments on CI

This is pretty hard to test. It only happens for me every 5-10 reloads of the map. When the labels are vanishing, the map is also really slow. Since this is a race condition we're trying to remove, I don't think there's a good way to test this. Even if we had a test, we'd only catch the error in a very limited number of test runs.

Longer term, it might make more sense to force explicit bindings, similar to how GL native does it.

[dagjomar]

Anybody have time to review this? Waiting for the specific bugfix so we can upgrade the version :)

[anandthakker]

Sheesh. What a weird issue, nice catch.