Bug 37118 - WebGLUniformLocation objects must be invalidated during linkProgram
Summary: WebGLUniformLocation objects must be invalidated during linkProgram
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
Depends on:
Reported: 2010-04-05 15:32 PDT by Kenneth Russell
Modified: 2010-12-09 02:22 PST (History)
8 users (show)

See Also:

patch (11.69 KB, patch)
2010-12-07 13:45 PST, Zhenyao Mo
kbr: review-
zmo: commit-queue-
Details | Formatted Diff | Diff
revised patch: responding to kbr's review (11.56 KB, patch)
2010-12-08 16:44 PST, Zhenyao Mo
kbr: review+
zmo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-04-05 15:32:47 PDT
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.
Comment 1 Zhenyao Mo 2010-12-07 13:45:09 PST
Created attachment 75838 [details]

Test will be synced back to khronos once reviewed.
Comment 2 WebKit Review Bot 2010-12-07 21:51:37 PST
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.
Comment 3 Kenneth Russell 2010-12-08 11:47:33 PST
Comment on attachment 75838 [details]

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.
Comment 4 Zhenyao Mo 2010-12-08 16:44:31 PST
Created attachment 75984 [details]
revised patch: responding to kbr's review
Comment 5 Kenneth Russell 2010-12-08 16:47:54 PST
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.
Comment 6 Zhenyao Mo 2010-12-08 17:40:37 PST
Committed r73573: <http://trac.webkit.org/changeset/73573>
Comment 7 WebKit Review Bot 2010-12-09 02:22:29 PST
http://trac.webkit.org/changeset/73573 might have broken GTK Linux 32-bit Release