Bug 66710 – Assertion fires if canvas is resized while save() active

• Component: Canvas
• Status: RESOLVED FIXED

[Stephen White]

Created attachment 104734 [details]
Test case

If a 2D canvas is resized (via canvas.width = foo) while a save() is active, GraphicsContext's destructor asserts:

ASSERT(m_stack.isEmpty());

[Matthew Delaney]

(In reply to comment #0)
> Created an attachment (id=104734) [details]
> Test case
> 
> If a 2D canvas is resized (via canvas.width = foo) while a save() is active, GraphicsContext's destructor asserts:
> 
> ASSERT(m_stack.isEmpty());

IIRC, this ASSERT was added primarily for helping with an issue Simon discovered in the layout tests. Certain canvas tests would use save() without a corresponding restore() - which in general, is totally fine for a page's script to do - but it had some side effect in the layout test environment that caused other subsequent tests in the chain of layout tests to act up or fail altogether. So, adding the assert helped to find those canvas tests that didn't restore, so that we could add restores to them, and avoid the issue.

Surprisingly, I haven't run into many (if any at all - can't think of any at the moment) real world examples of saves not matched with restores. I assume this is because almost all use cases for save() and restore() are animations, and it's crucial for them to match saves and restores in order to work properly. Though, I always thought that I'd find more pages that would have behavior that might bail out in the middle of some animation, recreate the canvas, and hit this assert. I suppose this bug is pretty much that case. OR even more simply: have an extra save or two in the beginning that wouldn't affect them but would hit the assert when we tear down the page.

Do we have anything instead of ASSERT like TESTING that would only enable certain "testing" asserts when we're running tests? I always considered ASSERT to be a way to declare an invariant in the code, such that if it fails, it signifies a bug in our code or our understanding of our code. This ASSERT is not an invariant since it's perfectly acceptable for a context to die with a non-zero size state stack.

[Stephen White]

(In reply to comment #1)
> (In reply to comment #0)
> > Created an attachment (id=104734) [details] [details]
> > Test case
> > 
> > If a 2D canvas is resized (via canvas.width = foo) while a save() is active, GraphicsContext's destructor asserts:
> > 
> > ASSERT(m_stack.isEmpty());
> 
> IIRC, this ASSERT was added primarily for helping with an issue Simon discovered in the layout tests. Certain canvas tests would use save() without a corresponding restore() - which in general, is totally fine for a page's script to do - but it had some side effect in the layout test environment that caused other subsequent tests in the chain of layout tests to act up or fail altogether. So, adding the assert helped to find those canvas tests that didn't restore, so that we could add restores to them, and avoid the issue.
> 
> Surprisingly, I haven't run into many (if any at all - can't think of any at the moment) real world examples of saves not matched with restores.

This assertion fires in Chrome if I visit http://ie.microsoft.com/testdrive/Performance/Fireflies/Default.html.

 I assume this is because almost all use cases for save() and restore() are animations, and it's crucial for them to match saves and restores in order to work properly. Though, I always thought that I'd find more pages that would have behavior that might bail out in the middle of some animation, recreate the canvas, and hit this assert. I suppose this bug is pretty much that case. OR even more simply: have an extra save or two in the beginning that wouldn't affect them but would hit the assert when we tear down the page.
> 
> Do we have anything instead of ASSERT like TESTING that would only enable certain "testing" asserts when we're running tests? I always considered ASSERT to be a way to declare an invariant in the code, such that if it fails, it signifies a bug in our code or our understanding of our code. This ASSERT is not an invariant since it's perfectly acceptable for a context to die with a non-zero size state stack.

If we did so, it would be impossible to add the attached test case as a layout test, for example.

It should be fairly straightforward to fix this bug by copying the #if !ASSERT_DISABLED code that's currently in the CanvasRenderingContext2D destructor to HTMLCanvasElement::setSurfaceSize(), just before m_imageBuffer is cleared.   Or alternatively, to the ImageBuffer destructor, although that would have to be done in every port.

[Stephen White]

Created attachment 104853 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 104853 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:328
> +#if !ASSERT_DISABLED
> +    if (m_context)
> +        m_context->clearStateStack();
> +#endif

I don't think state stack behavior should change between debug and non-debug builds. This could lead to hard-to-find bugs.

[Stephen White]

(In reply to comment #4)
> (From update of attachment 104853 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104853&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:328
> > +#if !ASSERT_DISABLED
> > +    if (m_context)
> > +        m_context->clearStateStack();
> > +#endif
> 
> I don't think state stack behavior should change between debug and non-debug builds. This could lead to hard-to-find bugs.

I was just following the pattern of the CanvasRenderingContext2D destructor, which has the same #if !ASSERT_DISABLED.

Would you prefer we clear the state stack unconditionally?  I'm fine with that.

[Stephen White]

Created attachment 104859 [details]
Same as above with #if !ASSERT_DISABLED removed

[Simon Fraser (smfr)]

Another way to fix this would be to move the  context2D->reset(); before the setSurfaceSize() in HTMLCanvasElement::reset().

[Stephen White]

(In reply to comment #7)
> Another way to fix this would be to move the  context2D->reset(); before the setSurfaceSize() in HTMLCanvasElement::reset().

Doesn't work.  The state stack has one element after CanvasRenderingContext2D::reset(), which still causes the assert to fire.

[Stephen White]

(In reply to comment #8)
> (In reply to comment #7)
> > Another way to fix this would be to move the  context2D->reset(); before the setSurfaceSize() in HTMLCanvasElement::reset().
> 
> Doesn't work.  The state stack has one element after CanvasRenderingContext2D::reset(), which still causes the assert to fire.

Can be made to work by clearing the (GraphicsContext) state stack in CanvasRenderingContext2D::reset(), and putting the reset() call ahead of the setSurfaceSize() call in HTMLCanvasElement as you suggest.

But this starts to feel a bit fragile -- the real problem occurs when the ImageBuffer is destroyed, so it makes more sense to me to put the call near the point where it is freed. Still, I'll upload this version for your consideration.

[Stephen White]

Created attachment 104865 [details]
Make CanvasRenderingContext2D::reset() clear the state stack

[Simon Fraser (smfr)]

Comment on attachment 104865 [details]
Make CanvasRenderingContext2D::reset() clear the state stack

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:178
> +    if (size_t stackSize = m_stateStack.size()) {
> +        if (GraphicsContext* context = canvas()->existingDrawingContext()) {
> +            while (--stackSize)
> +                context->restore();
> +        }
> +    }

WOuld be nice to share this code with the dtor. Maybe a new "unwindStateStack()" method or something.

[Stephen White]

Created attachment 104873 [details]
Patch

[Stephen White]

(In reply to comment #11)
> (From update of attachment 104865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104865&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:178
> > +    if (size_t stackSize = m_stateStack.size()) {
> > +        if (GraphicsContext* context = canvas()->existingDrawingContext()) {
> > +            while (--stackSize)
> > +                context->restore();
> > +        }
> > +    }
> 
> WOuld be nice to share this code with the dtor. Maybe a new "unwindStateStack()" method or something.

OK, done.  Let me know what you think.

[Stephen White]

Committed r93901: <http://trac.webkit.org/changeset/93901>