Bug 59109 – Extra GraphicsContext::save() in InlineTextBox::paintDecoration()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Looks like there might be an unbalanced context->save() in InlineTextBox::paintDecoration(). We should have assertions to check that this never happens.

[Simon Fraser (smfr)]

Created attachment 90565 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 90565 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:105
> +    // Deal with mismatched save/restore calls from content.
> +    if (GraphicsContext* context = drawingContext()) {
> +        while (context->stackDepth())
> +            context->restore();
> +    }

I take it tests hit your new ASSERT w/o this?

[Simon Fraser (smfr)]

(In reply to comment #2)
> (From update of attachment 90565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90565&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:105
> > +    // Deal with mismatched save/restore calls from content.
> > +    if (GraphicsContext* context = drawingContext()) {
> > +        while (context->stackDepth())
> > +            context->restore();
> > +    }
> 
> I take it tests hit your new ASSERT w/o this?

Without this, DRT crashes on fast/dom/gc-something if run after fast/canvas, because some canvas test has issues.

mitz thinks that this whole block could be #if !ASSERTS_DISABLED and I agree.

[Simon Fraser (smfr)]

Created attachment 90663 [details]
Patch

Comment on attachment 90663 [details]
Patch

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

r=me but please consider my comments

> Source/WebCore/html/HTMLCanvasElement.cpp:107
> +#if !ASSERT_DISABLED
> +    // Deal with mismatched save/restore calls from content.
> +    if (GraphicsContext* context = drawingContext()) {
> +        while (context->stackDepth())
> +            context->restore();
> +    }
> +#endif

Why can’t CanvasRenderingContext2D (and other CanvasRenderingContexts if necessary) handle this by popping its internal stack?

> Source/WebCore/platform/graphics/GraphicsContext.h:283
> +        size_t stackDepth() const { return m_stack.size(); }

It’s kind of ugly that this is a public member. Please consider guarding this with #if !ASSERT_DISABLED too.

[Simon Fraser (smfr)]

Some canvas tests are still causing this assertion to fire:

run-webkit-tests --debug --gc-between-tests LayoutTests/fast/canvas/change-context.html LayoutTests/fast/canvas/create-pattern-does-not-crash.html LayoutTests/fast/canvas/drawImage-with-globalAlpha.html

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/84631