Bug 94036 – [WebGL] program should not be able to link if a bad shader is attached

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Dean Jackson]

WebGL conformance test suite build: 5a7a067ead6 Tue Aug 7 14:34:42 2012 -0700
WebKit: r125123 on Mountain Lion 12A269
GPU: AMD Radeon HD 6750M 1024 MB (yes, fairly old)

[Radar WebKit Bug Importer]

<rdar://problem/12099740>

[Roger Fong]

Khronos WebGL test fails as a result:
conformance/programs/program-test.html failing on Apple Mountain Lion

[Roger Fong]

Created attachment 168122 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 168122 [details]
Patch

Attachment 168122 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14254399

[WebKit Review Bot]

Comment on attachment 168122 [details]
Patch

Attachment 168122 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14255431

[Dean Jackson]

Comment on attachment 168122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168122&action=review

> Source/WebCore/html/canvas/WebGLShader.h:46
> +    bool isValid() const { return m_valid; }

I think this should be just valid(), but see below about avoiding this.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:677
> -    void compileShader(Platform3DObject);
> +    bool compileShader(Platform3DObject);

Unfortunately this change will require a fair amount of coordination with other platforms, in particular Chromium (who have a bunch of proxies implementing this API, some in the Chromium project).

Let's see if we can come up with another way to do this. ShaderSourceEntry already has a "valid" flag that doesn't seem to be used.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:449
>      if (!translatedShaderSource.length())
> -        return;
> +        return false;

OK. Maybe we can do it here! If the validation failed, then get the entry in the Shader Source map and set it to invalid. (In fact, why not set it true/false for every shader?). Hmm... nope, then we'd still need to either update GC3D to have API testing for valid shaders, or have linkProgram repeat the test and communicate the failure.

How does Chrome pass this? Do they detach the shader if it fails validation? I'll need to think about this for a moment.

[Dean Jackson]

Ken, can you give advice here?

[Kenneth Russell]

I don't know off the top of my head how this is implemented in Chrome, but Gregg's command buffer implementation tracks a lot of information about shader and program objects, and is probably caching the state so that it looks like a compilation failure and can be queried as such through getShaderParameter.

[Roger Fong]

Something else I was unsure about was m_linkStatus.
Right it gets set to false when either the vertex or fragment shader is null before the linking happens.

Should it also be false if the the shader source doesn't compile?

I guess the question boils down to:
Should m_linkStatus be false when the state of the attached shaders is bad in general (either null or does not compile) or just when a shader is missing?

If it's the former then I'll need to find a way to access the m_shaderSourceMap from WebGLRenderingContext.

[Gregg Tavares]

not looking at the code, only responding the comment.

The only thing that effects link status is glLinkProgram

You can attach shaders, delete shaders, they can fail to compile, whatever but until you call glLinkProgram again the link status will not change. At that time, it will try to link the 2 shaders. If one of them is bad or missing it will not link and link status will be set to failed.  

Note that the failing for a missing shader has to be emulated on desktop GL as missing shaders fall back to fixed function behavior on desktop GL.

[Roger Fong]

I'm gonna throw out some thoughts about this and maybe someone can tell me if what I'm thinking is wrong:

The code for linkProgram in WebGLRC looks like this:

    if (!isGLES2Compliant()) {
        if (!program->getAttachedShader(GraphicsContext3D::VERTEX_SHADER) || !program->getAttachedShader(GraphicsContext3D::FRAGMENT_SHADER)) {
            program->setLinkStatus(false);
            return;
        }
    }

(At least the part we care about).

With a bad shader, it must get to the program->setLinkStatus(false), but check that's performed on the shader is the !program->getAttachedShader, which just returns the shader itself on the WebGLProgram.

The shader is not going to be null if it's bad...so how does this test pass anywhere?

[Roger Fong]

Just noticed there's a method I can use from WebGLRC that you can look for shader compile status with...so I don't need GC3D's compileShader method to return anything...so doing that and using the extra flag in the WebGLShader I can make it work properly...

Still probably not the best solution though...

[Roger Fong]

(In reply to comment #11)
> I'm gonna throw out some thoughts about this and maybe someone can tell me if what I'm thinking is wrong:
> 
> The code for linkProgram in WebGLRC looks like this:
> 
>     if (!isGLES2Compliant()) {
>         if (!program->getAttachedShader(GraphicsContext3D::VERTEX_SHADER) || !program->getAttachedShader(GraphicsContext3D::FRAGMENT_SHADER)) {
>             program->setLinkStatus(false);
>             return;
>         }
>     }
> 
> (At least the part we care about).
> 
> With a bad shader, it must get to the program->setLinkStatus(false), but check that's performed on the shader is the !program->getAttachedShader, which just returns the shader itself on the WebGLProgram.
> 
> The shader is not going to be null if it's bad...so how does this test pass anywhere?

O also, I tried not attaching the shader if the compiling failed, leaving the shader detached.
That causes another test to fail. The rule is apparently that the shader should get attached even if it's bad.

[Dean Jackson]

(In reply to comment #10)
> not looking at the code, only responding the comment.
> 
> The only thing that effects link status is glLinkProgram
> 
> You can attach shaders, delete shaders, they can fail to compile, whatever but until you call glLinkProgram again the link status will not change. At that time, it will try to link the 2 shaders. If one of them is bad or missing it will not link and link status will be set to failed.  

Thanks Gregg. We're a little confused here because the test takes a working and linked program, attaches a bad shader, then attempts to link again. The link fails, as expected.

The bit we're breaking on is that the subsequent drawArrays call is producing INVALID_OPERATION, when the test expects it to work. The OpenGL spec says "If the program object currently in use is relinked unsuccessfully, its link status will be set to GL_FALSE , but the executables and associated state will remain part of the current state until a subsequent call to glUseProgram removes it from use. "

I assume this is why the test expects the drawArrays to pass, even though the linkProgram failed.

Since we're calling our platform glLinkProgram, I'm confused as to how Chromium is passing this test.  Do you actually call glLinkProgram when you have a shader attached that didn't compile?

[Gregg Tavares]

No, we don't call LinkProgram if the shader didn't compile. We just synthesize a failed link status

[Gregg Tavares]

The check you mentioned above that just checks that shaders are attached works around the difference mentioned above between ES and Desktop GL in that desktop GL will succeed even if no shaders are attached since it falls back  to fixed function operations in that case.

[Dean Jackson]

(In reply to comment #16)
> The check you mentioned above that just checks that shaders are attached works around the difference mentioned above between ES and Desktop GL in that desktop GL will succeed even if no shaders are attached since it falls back  to fixed function operations in that case.

Got it, thanks.

Our problem is that we don't keep any program state in GC3D, the way Chromium does in your GPU command buffer implementation. Even if we tested the attached shaders to make sure they are valid, we have no way of propagating the link status.

This will be an easy addition into our port's version of GC3D.

[Dean Jackson]

Created attachment 213429 [details]
Patch

[Kenneth Russell]

Comment on attachment 213429 [details]
Patch

LGTM FWIW.

[Darin Adler]

Comment on attachment 213429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213429&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1301
> +    shader->setValid(!!value);

Don't need the !! with C++ bool; just "value" will work fine.

[Dean Jackson]

Committed r156971: <http://trac.webkit.org/changeset/156971>