Closed Bug 1015561 Opened 10 years ago Closed 5 years ago

"Assertion failure: false (Bad cached value.)"

Categories

(Core :: Graphics: CanvasWebGL, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected

People

(Reporter: jruderman, Assigned: wlitwinczyk)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(6 files, 10 obsolete files)

371 bytes, text/html
Details
7.36 KB, text/plain
Details
132.16 KB, text/plain
Details
14.72 KB, text/html
Details
6.91 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
8.95 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
Attached file testcase
Assertion failure: false (Bad cached value.), at content/canvas/src/WebGLContextUtils.cpp:423

The assertion added in bug 1004309 caught something!
Attached file stack
Assignee: nobody → wlitwinczyk
Attached file 1015561_crash.txt
Updated crash log, OSX
Attached patch stencil_ref_patch (obsolete) — Splinter Review
Not sure if we want to do this clamping in another area or not. For example, clamp inside StencilFuncSeparate() and save the clamped value as mStencilRef[Back|Front].

Page 95: http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf
says:

> Stencil comparison operations and queries of ref 
> clamp its value to the range [0, 2^s - 1] where s
> is the number of bits in the stencil buffer attached 
> to the framebuffer. 

More patches for new tests to come.
Attachment #8485986 - Flags: review?(jgilbert)
Attachment #8485986 - Flags: review?(jgilbert) → review+
Ugh, turns out that this breaks on Linux now because those drivers don't mask the return value with the stencil bits.
Attached patch stencil_cache_mochitests_patch (obsolete) — Splinter Review
This test currently catches the assertion bug. Using the setTimeout() function is crucial because without it everything works fine...
Attachment #8486761 - Flags: review?(jgilbert)
Comment on attachment 8486761 [details] [diff] [review]
stencil_cache_mochitests_patch

Review of attachment 8486761 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/test/webgl-mochitest/test_webgl_stencil_query.html
@@ +9,5 @@
> +        <script src="driver-info.js"></script>
> +    </head>
> +    <body onload="runTests();">
> +    <script>
> +        "use strict";

Don't indent the contents of the script tag, even if you indent the rest of the HTML tree.
Attachment #8486761 - Flags: review?(jgilbert) → review+
Attached file test_webgl_stencil_query.solo.html (obsolete) —
Standalone test case
So other platforms are actually pretty well behaved, Linux isn't.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0c5c612247ae

It looks like I need more conditional checking. I know I can get vendor + renderer from GLContext, but how can I get what OS I'm running?
Attached patch stencil_ref_patch (obsolete) — Splinter Review
Eh, I'm not too happy about it. You probably have a better idea of how to work around these problems.

getParameter(STENCIL_BITS) doesn't work correctly as the attachments aren't bound to the underlying GL context until FinalizeAttachments() is called. This leads to binding a stencil buffer and then getting 0 stencil bits.

The stencil ref/back ref code is kind of ugly.
Attachment #8485986 - Attachment is obsolete: true
Attachment #8494065 - Flags: review?(jgilbert)
Attached patch stencil_cache_mochitests_patch (obsolete) — Splinter Review
Added more test cases. I think I covered enough of the combinations of framebuffers and attachments. Although, I didn't test with textures, only renderbuffers.
Attachment #8486761 - Attachment is obsolete: true
Attachment #8494066 - Flags: review?(jgilbert)
Attached file test_webgl_stencil_query.solo.html (obsolete) —
Updated solo test
Attachment #8489662 - Attachment is obsolete: true
Comment on attachment 8494065 [details] [diff] [review]
stencil_ref_patch

Review of attachment 8494065 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for sending this back, but I think we should really have the same code to handle getting the number of stencil bits in both the query itself, and for masking in our workaround.

::: dom/canvas/WebGLContextState.cpp
@@ +244,5 @@
> +        case LOCAL_GL_STENCIL_REF:
> +        case LOCAL_GL_STENCIL_BACK_REF: {
> +            if (mBoundFramebuffer && mBoundFramebuffer->HasDepthStencilConflict()) {
> +                // Error, we don't know which stencil buffer's bits to use
> +                ErrorInvalidOperation("getParameter: framebuffer has two stencil buffers bound");

The best error here is probably INVALID_FRAMEBUFFER_OPERATION.

@@ +250,5 @@
> +            }
> +
> +            // Assuming stencils have 8 bits
> +            GLint stencilMask = (1 << 8) - 1;
> +            const bool noStencilBound = mBoundFramebuffer &&

This is probably easier to follow as:
if bound framebuffer:
  if stencil/depthStencil:
    stencilBits = 8
else:
  if options.stencil:
    stencilBits = 8

@@ +272,5 @@
> +            {
> +                mBoundFramebuffer->FinalizeAttachments();
> +            }
> +
> +            gl->fGetIntegerv(pname, &i);

Actually, pull this out into some GetStencilBits function, and use it both here and above in STENCIL_{BACK_}REF.

::: dom/canvas/WebGLContextUtils.cpp
@@ +643,5 @@
>      AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_CLEAR_VALUE, mStencilClearValue);
>  
> +    const GLRenderer renderer = gl->Renderer();
> +
> +    bool stencilRefNeedsWorkaround = renderer == GLRenderer::GalliumLlvmpipe;

This is backwards. A good implementation should do this masking for us. We should workaround on platforms that *don't* mask. (Specifically, the only workaround we need is to do the masking on platforms which don't do it for us already)
Attachment #8494065 - Flags: review?(jgilbert) → review-
Comment on attachment 8494066 [details] [diff] [review]
stencil_cache_mochitests_patch

Review of attachment 8494066 [details] [diff] [review]:
-----------------------------------------------------------------

Let's do another round.

::: dom/canvas/test/webgl-mochitest/test_webgl_stencil_query.html
@@ +8,5 @@
> +        <script src="webgl-util.js"></script>
> +        <script src="driver-info.js"></script>
> +    </head>
> +    <body onload="runTests();">
> +    <script type="text/javascript">

Technically, it would follow that this tag is indented one more time, since it's in the <body> scope.

@@ +9,5 @@
> +        <script src="driver-info.js"></script>
> +    </head>
> +    <body onload="runTests();">
> +    <script type="text/javascript">
> +    "use strict";

Don't indent the contents of the script tag in a file which is mostly script tag.

@@ +76,5 @@
> +            ok(gl.checkFramebufferStatus(gl.FRAMEBUFFER) != gl.FRAMEBUFFER_COMPLETE,
> +               'Framebuffer should be incomplete');
> +        }
> +
> +        if (shouldHaveStencilBits) {

If shouldError, then we shouldn't test the result of gl.getParameter.

@@ +162,5 @@
> +            document.body.appendChild(c);
> +            gl = c.getContext('experimental-webgl', {stencil:useStencil});
> +            ok(gl, 'Expected WebGL creation to succeed.');
> +
> +            var haveStencil = gl.getContextAttributes()['stencil'];

Sure, though you can just do `gl.getContextAttributes().stencil`.

@@ +167,5 @@
> +
> +            var stencilBits = gl.getParameter(gl.STENCIL_BITS);
> +            if (haveStencil) {
> +                ok(stencilBits > 0, 'If stencil creation succeeded, there should be > 0 stencil bits, got ' + stencilBits + '.');
> +                checkStencil(stencilBits, false);

Shouldn't this be checkStencil(stencilBits, true)?

@@ +177,5 @@
> +        }
> +    }
> +
> +    function runTests() {
> +        // Need to use setTimeout. Without it Firefox works fine.

You actually want to test both with and without. It exercises different codepaths because of how we switch from readback ShSurf_Basic surfaces to some accelerated backend.

@@ +182,5 @@
> +        function testTrueFalse(testFunc, next) {
> +            return function() {
> +                setTimeout(function() {
> +                    testFunc();
> +                    gl.clear(gl.COLOR_BUFFER_BIT);

A more sure-fire way to reset the canvas is to do:
gl.canvas.width += 1;
gl.canvas.width -= 1;

This triggers a canvas resize (well, two of them), which will return the canvas backbuffer to its original state.

@@ +205,5 @@
> +            if (lst.length == 0) return function() { SimpleTest.finish(); }
> +
> +            var testFunc = lst[0];
> +            var smaller_list = lst.splice(1, lst.length-1);
> +            return testTrueFalse(testFunc, buildTest(smaller_list));

This recursive function building is hard to follow. Can't we just do this iteratively?
Attachment #8494066 - Flags: review?(jgilbert) → review-
Attached patch stencil_ref_patch (obsolete) — Splinter Review
Attachment #8494065 - Attachment is obsolete: true
Attachment #8494754 - Flags: review?(jgilbert)
Attached patch stencil_cache_mochitests_patch (obsolete) — Splinter Review
Attachment #8494066 - Attachment is obsolete: true
Attachment #8494755 - Flags: review?(jgilbert)
Attachment #8494067 - Attachment is obsolete: true
Comment on attachment 8494754 [details] [diff] [review]
stencil_ref_patch

Review of attachment 8494754 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContextState.cpp
@@ +70,5 @@
>      return JS::StringValue(str);
>  }
>  
> +bool
> +WebGLContext::GetStencilBits(GLint& stencilBits)

Use pointers for out-params:
bool Foo(GLint* out_stencilBits)

::: dom/canvas/WebGLContextUtils.cpp
@@ +663,5 @@
> +        const GLuint stencilRefMask = (1 << stencilBits) - 1;
> +
> +        // Good platforms return masked values, but our cached values are not masked
> +        AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_REF,      mStencilRefFront & stencilRefMask);
> +        AssertUintParamCorrect(gl, LOCAL_GL_STENCIL_BACK_REF, mStencilRefBack & stencilRefMask);

Let's do AssertMaskedUintParamCorrect(gl, enum, mask, value).
This will work regardless of whether the underlying implementation gets it right.
Attachment #8494754 - Flags: review?(jgilbert) → review-
Comment on attachment 8494755 [details] [diff] [review]
stencil_cache_mochitests_patch

Review of attachment 8494755 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/test/webgl-mochitest/test_webgl_stencil_query.html
@@ +201,5 @@
> +            gl.canvas.width -= 1;
> +        }
> +    }
> +
> +    function runTestsWithSetTimeout(curIndex) {

Instead of chaining these setTimeouts together, you should be fine just delaying with requestAnimationFrame once, and then rerun all the tests. There shouldn't be any difference between doing that and having a setTimeout between each test.
Attachment #8494755 - Flags: review?(jgilbert) → review+
Attached patch stencil_ref_patch (obsolete) — Splinter Review
Attachment #8494754 - Attachment is obsolete: true
Attachment #8494850 - Flags: review?(jgilbert)
Attached patch stencil_cache_mochitests_patch (obsolete) — Splinter Review
Changed to use requestAnimationFrame instead
Attachment #8494755 - Attachment is obsolete: true
Attachment #8494852 - Flags: review?(jgilbert)
Attachment #8494852 - Flags: review?(jgilbert) → review+
Comment on attachment 8494850 [details] [diff] [review]
stencil_ref_patch

Review of attachment 8494850 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContext.h
@@ +823,5 @@
>  // State and State Requests (WebGLContextState.cpp)
>  public:
>      void Disable(GLenum cap);
>      void Enable(GLenum cap);
> +    bool GetStencilBits(GLint* stencilBits);

Please use the `out_` prefix, so it's clear how this will be used.
Attachment #8494850 - Flags: review?(jgilbert) → review+
I don't think this was changed, but uploading because I'm leaving
Attachment #8494850 - Attachment is obsolete: true
Attachment #8496282 - Flags: review?(jgilbert)
Fixed mochitest.

Skip running on OSX 10.6 due to driver bug with stencil ref, and skip B2G linux because the driver is blacklisted from creating a context.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=54002796830c
Attachment #8494852 - Attachment is obsolete: true
Attachment #8496285 - Flags: review?(jgilbert)
Blocks: 1072577
Attachment #8496282 - Flags: review?(jgilbert) → review+
Attachment #8496285 - Flags: review?(jgilbert) → review+
jgilbert: I hit this "Bad cached value" assertion failure when running this WebGL fuzzer on a debug build. Is this a different bug?

https://bug614678.bugzilla.mozilla.org/attachment.cgi?id=493971

Assertion failure: false (Bad cached value.), at dom/canvas/WebGLContextUtils.cpp:991
#01: mozilla::WebGLContext::ForceClearFramebufferWithDefaultValues(unsigned int, bool const*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b91780]
#02: mozilla::WebGLContext::ClearScreen()[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b90be3]
#03: mozilla::WebGLContext::ClearBackbufferIfNeeded()[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b90a9c]
#04: mozilla::WebGLContext::DrawArrays_check(int, int, int, char const*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b967c3]
#05: mozilla::WebGLContext::DrawArrays(unsigned int, int, int)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b975be]
#06: mozilla::dom::WebGLRenderingContextBinding::drawArrays(JSContext*, JS::Handle<JSObject*>, mozilla::WebGLContext*, JSJitMethodCallArgs const&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1ac17a5]
#07: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1b53982]
#08: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d1143c]
#09: Interpret(JSContext*, js::RunState&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d2e291]
#10: js::RunScript(JSContext*, js::RunState&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d21f55]
#11: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3d38baa]
#12: EvalKernel(JSContext*, JS::CallArgs const&, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3658797]
#13: js::DirectEval(JSContext*, JS::CallArgs const&)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3658fc3]
#14: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)[objdir-osx/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3833b99]
Segmentation fault: 11
Flags: needinfo?(jgilbert)
Assignee: wlitwinczyk → nobody
The leave-open keyword is there and there is no activity for 6 months.
:jgilbert, maybe it's time to close this bug?
Flags: needinfo?(jgilbert)
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jgilbert)
Resolution: --- → FIXED
Assignee: nobody → wlitwinczyk
You need to log in before you can comment on or make changes to this bug.