Bug 208876 – [GPU Process] RemoteImageBufferProxy should ensure the state stack of the context is cleared before destruction

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Said Abou-Hallawa]

For any reason, if the GraphicsContext::save() did not get a matching GraphicsContext::restore(), the destructor of GraphicsContext will assert: the state stack is not empty. In some cases, the GPU Process may terminate before receiving the matching restore() for the RemoteImageBufferProxy. This can depend on when CanvasRenderingContext2D.save() and its matching restore() are called.

This causes many failures in the canvas layout tests. Currently running 'run-webkit-tests --debug --no-retry LayoutTests/canvas/ LayoutTests/fast/canvas  --internal-feature RenderCanvasInGPUProcessEnabled' results 255 failures.

[Said Abou-Hallawa]

Created attachment 393173 [details]
Patch

[Said Abou-Hallawa]

This patch lowers the number of failures in the above command from 255 to 100 failures.

[Myles C. Maxfield]

Comment on attachment 393173 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:58
> +        // destruction, to avoid assertions in the GraphicsContext dtor.

Does it make sense to delete the assertion instead? The purpose of assertions is to find bugs, not to force us to do busywork.

[Said Abou-Hallawa]

Comment on attachment 393173 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:58
>> +        // destruction, to avoid assertions in the GraphicsContext dtor.
> 
> Does it make sense to delete the assertion instead? The purpose of assertions is to find bugs, not to force us to do busywork.

I think the assertion is important to have. I hit this assertion only when running layout tests. When reloading the same page or load different pages, I see we are doing the right thing: the restore commands are pushed from the WebProcess to the GPUProcess and the loop below, does not execute even once. This is the same we do in CanvasRenderingContext2DBase::unwindStateStack().

I can do the same thing that other function is doing:

#if ASSERT_ENABLED
    while (context().stackSize())
        context().restore();
#endif

[Simon Fraser (smfr)]

Comment on attachment 393173 [details]
Patch

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

>>> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:58
>>> +        // destruction, to avoid assertions in the GraphicsContext dtor.
>> 
>> Does it make sense to delete the assertion instead? The purpose of assertions is to find bugs, not to force us to do busywork.
> 
> I think the assertion is important to have. I hit this assertion only when running layout tests. When reloading the same page or load different pages, I see we are doing the right thing: the restore commands are pushed from the WebProcess to the GPUProcess and the loop below, does not execute even once. This is the same we do in CanvasRenderingContext2DBase::unwindStateStack().
> 
> I can do the same thing that other function is doing:
> 
> #if ASSERT_ENABLED
>     while (context().stackSize())
>         context().restore();
> #endif

I don't think we should have different behavior in debug builds vs. release. Either always clear the stack, or remove the assertion.

[Said Abou-Hallawa]

Created attachment 393389 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 393389 [details]
Patch

We should not have different behavior in debug.

[Said Abou-Hallawa]

Comment on attachment 393173 [details]
Patch

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

>>>> Source/WebKit/GPUProcess/graphics/RemoteImageBufferProxy.h:58
>>>> +        // destruction, to avoid assertions in the GraphicsContext dtor.
>>> 
>>> Does it make sense to delete the assertion instead? The purpose of assertions is to find bugs, not to force us to do busywork.
>> 
>> I think the assertion is important to have. I hit this assertion only when running layout tests. When reloading the same page or load different pages, I see we are doing the right thing: the restore commands are pushed from the WebProcess to the GPUProcess and the loop below, does not execute even once. This is the same we do in CanvasRenderingContext2DBase::unwindStateStack().
>> 
>> I can do the same thing that other function is doing:
>> 
>> #if ASSERT_ENABLED
>>     while (context().stackSize())
>>         context().restore();
>> #endif
> 
> I don't think we should have different behavior in debug builds vs. release. Either always clear the stack, or remove the assertion.

-- I think the assertion is very useful for the render tree rendering where we control the save/restore. This assertion will detect if we miss a restore.
-- In CanvasRenderingContext2DBase, the save and restore are controlled by the javascript. If the javascript missed a restore, the assertion should not fire.
-- In RemoteImageBufferProxy, the restore might be lost because the connection between the GPUProcess and the WebProcess is terminated. In this case the assertion should not fire.

In the three cases the code to unwind the state stack and check the state stack is empty is only under #if ASSERT_ENABLED.

If this is not the right approach, I think we can do one of the following:

1. Remove the assertion and CanvasRenderingContext2DBase::unwindStateStack() also.
2. Add a flag to GraphicsContext like m_externalSaveRestoreControl. It will be true for the ImageBuffer context of CanvasRenderingContext2DBase and for the graphics context of RemoteImageBufferProxy. In the destructor of GraphicsContext. We can assert

ASSERT_IMPLIES(!m_externalSaveRestoreControl, m_stack.isEmpty());

[Wenson Hsieh]

Created attachment 410054 [details]
Reupload Said’s patch

[Tim Horton]

Comment on attachment 410054 [details]
Reupload Said’s patch

As I said on Slack... "seems a little funky, but OK".

[EWS]

Committed r267772: <https://trac.webkit.org/changeset/267772>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410054 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69769208>