(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);

Created attachment 182753 [details] patch

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.

(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.

(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.

(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.

Created attachment 182829 [details] patchv2 The patch still uses bool values but should cover all the cases now.

Created attachment 182876 [details] patchv3

replaced the usage of boolean member variables with enum

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.

Comment on attachment 182876 [details] patchv3 will upload a new patch with style fix

Created attachment 182881 [details] patchv5

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. :)

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.

> 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.

Comment on attachment 182881 [details] patchv5 I'm ok with this. Please ask a WebKit2 owner for review.

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.

Created attachment 183296 [details] patchv6

(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.

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.

Created attachment 183449 [details] patchv7

(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.

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"

Created attachment 183610 [details] patchv8

(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.

Created attachment 184115 [details] patchv9

(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.

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.

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

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

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

Created attachment 184386 [details] patchv10

(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.

(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.

Created attachment 185054 [details] patchv11 based on chromium test results

Created attachment 185084 [details] patchv12

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.

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

Created attachment 185413 [details] rebasedpatch

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

(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.

Comment on attachment 185413 [details] rebasedpatch Clearing flags on attachment: 185413 Committed r141247: <http://trac.webkit.org/changeset/141247>

All reviewed patches have been landed. Closing bug.