Bug 106878 – [EFL][Qt][WebGL] Avoid deleting an uncreated canvas.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

Try to load https://www.khronos.org/registry/webgl/sdk/demos/webkit/SpiritBox.html

Expected Result: Spinning cube

Actual Result: MiniBrowser crashes with an Assert !m_surfaceBackingStores.contains(id);

[Kalyan]

(In reply to comment #0)
> Try to load https://www.khronos.org/registry/webgl/sdk/demos/webkit/SpiritBox.html
> 
> Expected Result: Spinning cube
> 
> Actual Result: MiniBrowser crashes with an Assert !m_surfaceBackingStores.contains(id);

sorry, the assert is in void LayerTreeRenderer::destroyCanvas(CoordinatedLayerID id)

m_surfaceBackingStores.contains(id);

[Kalyan]

Created attachment 182753 [details]
patch

[Rafael Brandao]

Comment on attachment 182753 [details]
patch

Instead of using a yet another bool, can't you use some of the existing ones? If a canvas has not been created yet, shouldn't it have "canvasNeedsCreated" turned on? If this is the case, you could simply turn that off when you're asked to delete it.

[Kalyan]

(In reply to comment #3)
> (From update of attachment 182753 [details])
> Instead of using a yet another bool, can't you use some of the existing ones? If a canvas has not been created yet, shouldn't it have "canvasNeedsCreated" turned on? If this is the case, you could simply turn that off when you're asked to delete it.

This would be fine if the canvas wasn't created. But we also need to handle the case where canvas needs to be re-created.In this case both m_canvasNeedsDestroy and m_canvasNeedsCreate are enabled in setContentsToCanvas and Destroy is handled first.

[Rafael Brandao]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 182753 [details] [details])
> > Instead of using a yet another bool, can't you use some of the existing ones? If a canvas has not been created yet, shouldn't it have "canvasNeedsCreated" turned on? If this is the case, you could simply turn that off when you're asked to delete it.
> 
> This would be fine if the canvas wasn't created. But we also need to handle the case where canvas needs to be re-created.In this case both m_canvasNeedsDestroy and m_canvasNeedsCreate are enabled in setContentsToCanvas and Destroy is handled first.

In this special case, if both are turned on, you could ignore the deletion by turning it off and early returning. Is that right that always when they both are turned on it means we expect a recreation? My feeling is that we have too many states here and maybe we could get a more understandable code if we used enums to track them accurately.

[Kalyan]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 182753 [details] [details] [details])
> > >  My feeling is that we have too many states here and maybe we could get a more understandable code if we used enums to track them accurately.

I agree, enum with some well defined states would be good here. 

On a side note when recreation is needed we need to destroy the previous canvas with that particular id and create a new one. So we cannot ignore either state in that case.

[Kalyan]

Created attachment 182829 [details]
patchv2

The patch still uses bool values but should cover all the cases now.

[Kalyan]

Created attachment 182876 [details]
patchv3

[Kalyan]

replaced the usage of boolean member variables with enum

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:170:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kalyan]

Comment on attachment 182876 [details]
patchv3

will upload a new patch with style fix

[Kalyan]

Created attachment 182881 [details]
patchv5

[Zeno Albisser]

Comment on attachment 182881 [details]
patchv5

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

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:256
> +    unsigned m_canvasState;

I'd make this of type CanvasState, to make the type explicit.

Seems much better than the booleans. :)

[Viatcheslav Ostapenko]

Comment on attachment 182881 [details]
patchv5

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

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:256
>> +    unsigned m_canvasState;
> 
> I'd make this of type CanvasState, to make the type explicit.
> 
> Seems much better than the booleans. :)

I don't think it is good idea. m_canvasState is not an enum, but bit field here.
BTW, I don't know why it is better than old bool m_var:1 . Instead of 3 bits used inside common bit space for bool vars in original proposal now it uses extra 32 bits.

[Noam Rosenthal]

> I don't think it is good idea. m_canvasState is not an enum, but bit field here.
You can typedef it to unsigned.

> BTW, I don't know why it is better than old bool m_var:1 . Instead of 3 bits used inside common bit space for bool vars in original proposal now it uses extra 32 bits.
Either is fine.

[Noam Rosenthal]

Comment on attachment 182881 [details]
patchv5

I'm ok with this. Please ask a WebKit2 owner for review.

[Benjamin Poulain]

Comment on attachment 182881 [details]
patchv5

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

You maintain m_canvasState even if USE(GRAPHICS_SURFACE) is false. You never use the value in that case.
It looks like something is wrong with the #ifdef.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:334
> +        }  else if ((m_canvasSize != platformLayer->platformLayerSize()) || (m_canvasToken != platformLayer->graphicsSurfaceToken()))
>              // m_canvasToken can be different to platformLayer->graphicsSurfaceToken(), even if m_canvasPlatformLayer equals platformLayer.
> -            m_canvasNeedsDestroy = true;
> -            m_canvasNeedsCreate = true;
> -        }
> +            m_canvasState |= RecreateCanvas;

I would use a bracket here for clarity.

[Kalyan]

Created attachment 183296 [details]
patchv6

[Kalyan]

(In reply to comment #17)
> (From update of attachment 182881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182881&action=review
> 
> You maintain m_canvasState even if USE(GRAPHICS_SURFACE) is false. You never use the value in that case.
> It looks like something is wrong with the #ifdef.

fixed.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics
> > +            m_canvasState |= RecreateCanvas;
> 
> I would use a bracket here for clarity.

done.

[Benjamin Poulain]

Comment on attachment 183296 [details]
patchv6

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

Better but you still have an unused variable without the USE()

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:127
> +    , m_canvasState(0)

This should be in a #ifdef block for USE(GRAPHICS_SURFACE)

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:256
> +    unsigned m_canvasState;

This should be in the block USE(GRAPHICS_SURFACE) above.

[Kalyan]

Created attachment 183449 [details]
patchv7

[Kalyan]

(In reply to comment #20)
> (From update of attachment 183296 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183296&action=review
> 
> This should be in the block USE(GRAPHICS_SURFACE) above.

done.

[Kenneth Rohde Christiansen]

Comment on attachment 183449 [details]
patchv7

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

> Source/WebKit2/ChangeLog:11
> +        is later used to either create or delete the canvas. The issue seems
> +        to be that we mark a canvas for deletion even though it has not

"seems" that doesn't make me confident in the patch

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:333
> +        if (!platformLayer) {
> +            m_canvasState |= DestroyCanvas;
> +            m_canvasState &= ~CreateCanvas;

this sounds more like a pending operation to me than a state

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:174
> +    enum CanvasState {
> +        CreateCanvas = 0x01,
> +        ValidCanvas = 0x02,
> +        DestroyCanvas = 0x04,
> +        RecreateCanvas = CreateCanvas | DestroyCanvas
> +    };

This seems a bit confusing to me... like mixing "create" with "valid"

[Kalyan]

Created attachment 183610 [details]
patchv8

[Kalyan]

(In reply to comment #23)
> (From update of attachment 183449 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183449&action=review
> "seems" that doesn't make me confident in the patch

fixed the changelog.

/CoordinatedGraphicsLayer.h:174
> > +    enum CanvasState {
> > +        CreateCanvas = 0x01,
> > +        ValidCanvas = 0x02,
> > +        DestroyCanvas = 0x04,
> > +        RecreateCanvas = CreateCanvas | DestroyCanvas
> > +    };
> 
> This seems a bit confusing to me... like mixing "create" with "valid"

Renamed the enum to PendingCanvasOperation, this would keep track of any pending canvas operations. A valid canvas (i.e Canvas creation request has been issued) is tracked separately.

[Kalyan]

Created attachment 184115 [details]
patchv9

[Kalyan]

(In reply to comment #26)
> Created an attachment (id=184115) [details]
> patchv9

Added a layout test to reproduce the issue. For completeness, removed m_canvasNeedsDisplay usage and made it part of PendingCanvasOperation.

[Benjamin Poulain]

Comment on attachment 184115 [details]
patchv9

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

> LayoutTests/fast/canvas/webgl/canvas-resize-crash.html:12
> +description('Test Canvas resize.');

I'd be a little more descriptive.

> LayoutTests/fast/canvas/webgl/canvas-resize-crash.html:20
> +    // change the size of the canvas's backing store to match the size it is displayed.

Uppercase C.

> LayoutTests/fast/canvas/webgl/canvas-resize-crash.html:25
> +    canvas.width = canvas.clientWidth;
> +    canvas.height = canvas.clientHeight;

No need to force a relayout here?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:122
> +    , m_validCanvas(false)

This is not a great name for a boolean.

[WebKit Review Bot]

Comment on attachment 184115 [details]
patchv9

Attachment 184115 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16039937

New failing tests:
fast/canvas/webgl/canvas-resize-crash.html
platform/chromium/virtual/gpu/fast/canvas/webgl/canvas-resize-crash.html

[Build Bot]

Comment on attachment 184115 [details]
patchv9

Attachment 184115 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16063394

New failing tests:
fast/canvas/webgl/canvas-resize-crash.html

[WebKit Review Bot]

Comment on attachment 184115 [details]
patchv9

Attachment 184115 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16038948

New failing tests:
fast/canvas/webgl/canvas-resize-crash.html
platform/chromium/virtual/gpu/fast/canvas/webgl/canvas-resize-crash.html

[Kalyan]

Created attachment 184386 [details]
patchv10

[Kalyan]

(In reply to comment #28)
> (From update of attachment 184115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184115&action=review
> 
> I'd be a little more descriptive.
> 
> Uppercase C.

done

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:122
> > +    , m_validCanvas(false)
> 
> This is not a great name for a boolean.

renamed it as m_isValidCanvas.

[Kalyan]

(In reply to comment #32)
> Created an attachment (id=184386) [details]
> patchv10

Test seems to fail with texdiff on some of the bots. Will update a new patch after looking into this issue.

[Kalyan]

Created attachment 185054 [details]
patchv11

based on chromium test results

[Kalyan]

Created attachment 185084 [details]
patchv12

[Benjamin Poulain]

Comment on attachment 185084 [details]
patchv12

Okay, everything looks good to me. I sign off on this for WebKit2.

Noam already r+ed on his side so let's go with this.

[WebKit Review Bot]

Comment on attachment 185084 [details]
patchv12

Rejecting attachment 185084 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 185084, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
oordinatedGraphics/CoordinatedGraphicsLayer.h
Hunk #1 FAILED at 166.
Hunk #2 succeeded at 190 (offset 6 lines).
Hunk #3 succeeded at 198 (offset 6 lines).
Hunk #4 succeeded at 227 (offset 6 lines).
1 out of 4 hunks FAILED -- saving rejects to file Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Benjamin Poulain']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16218340

[Kalyan]

Created attachment 185413 [details]
rebasedpatch

[Noam Rosenthal]

(In reply to comment #39)
> Created an attachment (id=185413) [details]
> rebasedpatch
Is this up for review?

[Kalyan]

(In reply to comment #40)
> (In reply to comment #39)
> > Created an attachment (id=185413) [details] [details]
> > rebasedpatch
> Is this up for review?

Put it up for cq. Patch has been reviewed already. Had a discussion with noam and he was k with it.

[WebKit Review Bot]

Comment on attachment 185413 [details]
rebasedpatch

Clearing flags on attachment: 185413

Committed r141247: <http://trac.webkit.org/changeset/141247>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.