Bug 37118 – WebGLUniformLocation objects must be invalidated during linkProgram

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Conceptually, when a WebGLProgram is relinked, any WebGLUniformLocation objects that were previously queried are no longer valid. Code needs to be added to the WebKit WebGL implementation to prevent invalid WebGLUniformLocation objects from being passed in. This can be as simple as keeping a link count in both the WebGLProgram and WebGLUniformLocation objects, incrementing the link count during linkProgram, and verifying equality of the link count when taking in WebGLUniformLocation objects.

[Zhenyao Mo]

Created attachment 75838 [details]
patch

Test will be synced back to khronos once reviewed.

[WebKit Review Bot]

Attachment 75838 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 75838 [details]
patch

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

Aside from the comments above the logic looks okay, but I think the term "link signature" is confusing. I'd suggest "link count" or "numberOfLinkProgramCalls".

> WebCore/html/canvas/WebGLUniformLocation.cpp:52
> +    if (!m_program || m_program->getLinkSignature() != m_linkSignature)

Since the constructor asserts that m_program is non-NULL, checking against !m_program is unnecessary.

> WebCore/html/canvas/WebGLUniformLocation.cpp:62
> +        return -1;

I don't think silently returning -1 here is a good idea. The location -1 is treated specially by OpenGL -- any call like glUniform3f taking a location of -1 is silently ignored. If the WebGLRenderingContext ends up calling this then it will likely produce wrong results instead of synthesizing the error it should. I think this entry point should probably ASSERT that the link signatures match.

[Zhenyao Mo]

Created attachment 75984 [details]
revised patch: responding to kbr's review

[Kenneth Russell]

Comment on attachment 75984 [details]
revised patch: responding to kbr's review

Looks good. Assuming that all layout tests were run in debug mode and that the new assertion wasn't triggered.

[Zhenyao Mo]

Committed r73573: <http://trac.webkit.org/changeset/73573>

[WebKit Review Bot]

http://trac.webkit.org/changeset/73573 might have broken GTK Linux 32-bit Release