Bug 32099 – Must fabricate GL errors instead of throwing exceptions

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

In most, if not all, cases where the WebKit WebGL implementation is raising JavaScript exceptions, for example during argument validation, it instead needs to fabricate the appropriate GL errors. Higher-level code can check the GL error state after each call and explicitly throw an exception. 

This is important to fix as soon as possible to make the implementation spec compliant and, in particular, so that the working group doesn't release bogus tests along with the first public draft of the spec.

[Kenneth Russell]

Created attachment 44278 [details]
Strawman patch

Not requesting review yet -- this is only for discussion with the WebGL working group.

[Kenneth Russell]

Created attachment 44507 [details]
Proposed patch

Changed WebGLRenderingContext to synthesize GL errors rather than raising JavaScript exceptions. Removed internal getError() calls after each graphics call. Based on idea from Ilmari Heikkinen, added create3DDebugContext() to webgl-test.js and changed the WebGL layout tests expecting error conditions to use it. The tests required no more than 2 lines of code change each and behave identically to how they did before. The changes to the expected.txt files are simply the contents of the exception messages, which implicitly test the exact GL error raised since that is part of the detail message.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44507 [details] without any errors.

[Oliver Hunt]

Comment on attachment 44507 [details]
Proposed patch

Ken, before i review this can i just get some clarification?

My recollection of glGetError behaviour is that every gl call clears the error flag, and then sets it if there's an error.  Is this correct?

Assuming it is correct it would seem that the first thing each of the WebGL functions should do is clear the error flag, this results in the following cases i believe we need to handle:

functionThatCreatesRealGLError();
functionThatDoesntTouchGLRuntimeButClearsError();
glGetError(); // should not produce an error

functionThatCreatesSynthesisedError();
functionThatDoesntProduceAnError();
glGetError(); // should not produce an error

functionThatCreatesRealGLError();
functionThatCreatesSynthesisedError();
glGetError(); // should produce an error
glGetError(); // should not produce an error

There may be a few other cases i haven't thought of

[Kenneth Russell]

The GL error state conceptually is a set of error flags. If a particular error flag is set then no further errors of that kind will be recorded. glGetError picks an arbitrary flag which is set, clears the flag, and returns the associated error to the caller. No other OpenGL API calls clear the error flags. See http://www.khronos.org/opengles/sdk/docs/man/glGetError.xml .

This implementation has one flaw, which is that if a synthetic GL error (say, INVALID_VALUE) is set and the same error happens to be set in the real GL state, both errors will be reported if glGetError is called multiple times. There are probably a couple of different ways to address this, but in the interest of simplicity I didn't attempt to fix it in this initial code. I don't think it will affect any real programs since they tend to call glGetError until it returns NO_ERROR, and this implementation will reach that fixed point.

[Kenneth Russell]

Any chance of a review of this today? I had been hoping to get this checked in for tomorrow's draft spec release, but I suppose it isn't critical, especially if there's anything controversial about these changes.

[Oliver Hunt]

Comment on attachment 44507 [details]
Proposed patch

Can you add a test for the result of context.getError() after a non-errored call? eg.

context.getParameter("blargh!");
context.getParameter(<something correct>);
context.getError();  // should be clear here

as well as a test to verify that we still provide actual errors form the gl runtime correctly as well
Can you also add a test to verify that we get GL_NO_ERROR back from getError after we have cleared the error list?

The code itself seems fine to me, i'd just like those tests to be added :-D

--Oliver

[Kenneth Russell]

(In reply to comment #7)
> (From update of attachment 44507 [details])
> Can you add a test for the result of context.getError() after a non-errored
> call? eg.
> 
> context.getParameter("blargh!");
> context.getParameter(<something correct>);
> context.getError();  // should be clear here

In this case the error flag is still supposed to be set. Is there some other situation you would like me to test?

> as well as a test to verify that we still provide actual errors form the gl
> runtime correctly as well

I'll add a new test. Looking through preexisting tests which cover this case, I just realized that getActiveAttrib is synthesizing errors in a case it shouldn't, which I'll fix in the next patch and cover in the test.

> Can you also add a test to verify that we get GL_NO_ERROR back from getError
> after we have cleared the error list?

Will add this in the new test, although it is actually already covered by preexisting tests.

[Kenneth Russell]

Created attachment 44597 [details]
Revised patch

(The ChangeLogs have been updated to reflect the comments below.)

Compared to the previous version:

 - Further testing uncovered bugs in error reporting in getActiveAttrib and getActiveUniform. These and the other bugs above highlighted the need to move the synthetic error management down into the GraphicsContext3D, because only at that level is complete information available for certain cases. Did this and changed the code in WebGLRenderingContext to call GraphicsContext3D::synthesizeGLError.
 - Testing on Chrome uncovered preexisting bugs in both WebKit's and Chrome's implementations of bindFramebuffer, framebufferRenderbuffer, and framebufferTexture2D. Fixed these bugs and the associated tests from LayoutTests/fast/canvas/webgl/null-object-behaviour.html.
 - Added LayoutTests/fast/canvas/webgl/error-reporting.html, which includes requested test cases as well as explicit regression tests for the other bugs uncovered during this exercise.

Tested on both WebKit and Chrome. All tests now pass in both browsers; the error reporting behavior is identical.

[Oliver Hunt]

Comment on attachment 44597 [details]
Revised patch

Ken, can you please not fix multiple unrelated bugs in a single patch.

The correct thing to do is to file a bug and fix them separately.  Putting multiple bug fixes into a single patch is harmful in the long term.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/GraphicsContext3D.cpp:748:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1

[Kenneth Russell]

(In reply to comment #10)
> (From update of attachment 44597 [details])
> Ken, can you please not fix multiple unrelated bugs in a single patch.
> 
> The correct thing to do is to file a bug and fix them separately.  Putting
> multiple bug fixes into a single patch is harmful in the long term.

If these issues aren't fixed then the existing tests will start to fail. What do you want me to do? Comment out the failing portions of the tests?

[Kenneth Russell]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 44597 [details] [details])
> > Ken, can you please not fix multiple unrelated bugs in a single patch.
> > 
> > The correct thing to do is to file a bug and fix them separately.  Putting
> > multiple bug fixes into a single patch is harmful in the long term.
> 
> If these issues aren't fixed then the existing tests will start to fail. What
> do you want me to do? Comment out the failing portions of the tests?

May I ask again what I should do in this case? Should I comment out the failing portions of the tests and fix both them and the associated code in another bug?

[Oliver Hunt]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > (From update of attachment 44597 [details] [details] [details])
> > > Ken, can you please not fix multiple unrelated bugs in a single patch.
> > > 
> > > The correct thing to do is to file a bug and fix them separately.  Putting
> > > multiple bug fixes into a single patch is harmful in the long term.
> > 
> > If these issues aren't fixed then the existing tests will start to fail. What
> > do you want me to do? Comment out the failing portions of the tests?
> 
> May I ask again what I should do in this case? Should I comment out the failing
> portions of the tests and fix both them and the associated code in another bug?

Comment out the framebuffer tests that are going to be broken, then re-enable them as part of the bug fix for not correctly tracking the active fbo

[Kenneth Russell]

Created attachment 44637 [details]
Revised patch

Removed changes related to default FBO and commented out affected portions of test cases. Filed https://bugs.webkit.org/show_bug.cgi?id=32391 to track this remaining issue.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44637 [details] without any errors.

[Oliver Hunt]

Comment on attachment 44637 [details]
Revised patch

r=me

[WebKit Commit Bot]

Comment on attachment 44637 [details]
Revised patch

Clearing flags on attachment: 44637

Committed r51970: <http://trac.webkit.org/changeset/51970>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.