Early deallocation of canvas X11 window causes random crashes on UI side in webgl layout tests. Have to make sure that UI doesn't use canvas X11 window before it is deleted on webprocess side.
Created attachment 182523 [details] Patch
Comment on attachment 182523 [details] Patch Attachment 182523 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15873378
Comment on attachment 182523 [details] Patch Attachment 182523 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15853444
Comment on attachment 182523 [details] Patch Attachment 182523 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15867394
Comment on attachment 182523 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182523&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:1098 > friend class GraphicsContext3DPrivate; > - OwnPtr<GraphicsContext3DPrivate> m_private; > + RefPtr<GraphicsContext3DPrivate> m_private; This looks wrong. Why have a shared ownership of a Private object? Please explain this in the ChangeLog.
Created attachment 182696 [details] Patch
Created attachment 182709 [details] Patch
(In reply to comment #5) > (From update of attachment 182523 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182523&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1098 > > friend class GraphicsContext3DPrivate; > > - OwnPtr<GraphicsContext3DPrivate> m_private; > > + RefPtr<GraphicsContext3DPrivate> m_private; > > This looks wrong. Why have a shared ownership of a Private object? > > Please explain this in the ChangeLog. No more shared GraphicsContext3DPrivate. It was wrong idea ;)
When Window is used we use XCompositeWindowPixmap in UI Process side, from the docs: XCompositeNameWindowPixmap creates and returns a pixmap id that serves as a reference to the off-screen storage for window. This pixmap will remain allocated until freed, even if the window is unmapped, reconfigured or destroyed. However, the window will get a new pixmap allocated each time it is mapped or resized, so this function will need to be reinvoked for the client to continue to refer to the storage holding the current window contents. Generates a BadMatch error if window is not redirected or is not visible. What additional benefits do we get from this change, just wondering if this is still required??
(In reply to comment #9) > > When Window is used we use XCompositeWindowPixmap in UI Process side, from the docs: > > XCompositeNameWindowPixmap creates and returns a pixmap id that serves as a reference to the off-screen storage for window. This pixmap will remain allocated until freed, even if the window is unmapped, reconfigured or destroyed. However, the window will get a new pixmap allocated each time it is mapped or resized, so this function will need to be reinvoked for the client to continue to refer to the storage holding the current window contents. Generates a BadMatch error if window is not redirected or is not visible. > > What additional benefits do we get from this change, just wondering if this is still required?? Without this I'm still getting crashes in mesa in glXReleaseTexImageEXT/glXBindTextImageEXT if window is deleted already. I also understood, that is should be NoOp if window becomes invalid, but it doesn't work. May be just mesa bug.
(In reply to comment #10) > (In reply to comment #9) > > glXReleaseTexImageEXT/glXBindTextImageEXT if window is deleted already. I also understood, that is should be NoOp if window becomes invalid, but it doesn't work. May be just mesa bug. Trying to identify the root cause would be beneficial before making this kind of change. I am also wondering if swapbuffers is the right place for bind/release instead of platformPaintToTextureMapper. As the contents of texture after drawable has been bound is the result of all rendering that has completed before the call to glXBindTexImageEXT. But I guess this is not related to the proposed change and could be handled separately.
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > > glXReleaseTexImageEXT/glXBindTextImageEXT if window is deleted already. I also understood, that is should be NoOp if window becomes invalid, but it doesn't work. May be just mesa bug. > > Trying to identify the root cause would be beneficial before making this kind of change. > > I am also wondering if swapbuffers is the right place for bind/release instead of platformPaintToTextureMapper. As the contents of texture after drawable has been bound is the result of all rendering that has completed before the call to glXBindTexImageEXT. But I guess this is not related to the proposed change and could be handled separately. It is definitely not, and it is what currently breaks any WebGL on GLX for Qt!
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #9) > > > > > > glXReleaseTexImageEXT/glXBindTextImageEXT if window is deleted already. I also understood, that is should be NoOp if window becomes invalid, but it doesn't work. May be just mesa bug. > > > > Trying to identify the root cause would be beneficial before making this kind of change. > > > > I am also wondering if swapbuffers is the right place for bind/release instead of platformPaintToTextureMapper. As the contents of texture after drawable has been bound is the result of all rendering that has completed before the call to glXBindTexImageEXT. But I guess this is not related to the proposed change and could be handled separately. > > It is definitely not, and it is what currently breaks any WebGL on GLX for Qt! How does it break? Could you write how to reproduce the break? I'm running Qt build and don't see any problem with it. And about "not the right place". We have do swapbuffers on webprocess and have to do matching operation on the UI process to re-bind texture to the new front buffer. We could do it during painting, but this will delay re-bind and will have greater chance of executing re-bind in parallel with swapbuffers on webprocess side.
Created attachment 183812 [details] Patch
Uploaded rebased patch.
(In reply to comment #13) > (In reply to comment #12) > How does it break? Could you write how to reproduce the break? > I'm running Qt build and don't see any problem with it. > And about "not the right place". > We have do swapbuffers on webprocess and have to do matching operation on the UI process to re-bind texture to the new front buffer. We could do it during painting, but this will delay re-bind and will have greater chance of executing re-bind in parallel with swapbuffers on webprocess side. GLX pixmap references the contents of the original pixmap. The texture may or may not use the same storage (entirely implementation dependent). If the texture uses a different storage, it needs to be updated with a call to glxBindTexImageExt. One way to ensure synchronisation is to have a glxwaitgl after swapbuffers call in webprocess side. This would ensure that all gl calls are handled before X rendering calls. Have a GLXWaitX before bind call which ensures that all X rendering calls are done prior to GL calls. Both of these dont involve round trips to XServer. Directly from TextureFromPixmap extension: To gurantee tear-free rendering, a composite manager would run as follows: -receive request for compositing: XGrabServer() glXWaitX() or XSync() glXBindTexImageEXT() <Do rendering/compositing> glXReleaseTexImageEXT() XUngrabServer() Apps that don't synchronize like this would get what's available, and that may or may not be what they expect.
(In reply to comment #13) > How does it break? Could you write how to reproduce the break? > I'm running Qt build and don't see any problem with it. > Have you been trying MiniBrowser? Any webgl page i currently open in MiniBrowser just stays black. This regression was introduced in r138327. Or to be more specific: removing the portion that was added to GraphicsSurface::platformSwapBuffers fixes the issue.
(In reply to comment #17) > (In reply to comment #13) > > > How does it break? Could you write how to reproduce the break? > > I'm running Qt build and don't see any problem with it. > > > > Have you been trying MiniBrowser? > > Any webgl page i currently open in MiniBrowser just stays black. > This regression was introduced in r138327. > Or to be more specific: removing the portion that was added to GraphicsSurface::platformSwapBuffers fixes the issue. Yes, I did run MiniBrowser. No, I don't have this issue. What graphics card do you have, ubuntu version and etc. I'll try to figure out.
(In reply to comment #16) > (In reply to comment #13) > > (In reply to comment #12) > > How does it break? Could you write how to reproduce the break? > > I'm running Qt build and don't see any problem with it. > > And about "not the right place". > > We have do swapbuffers on webprocess and have to do matching operation on the UI process to re-bind texture to the new front buffer. We could do it during painting, but this will delay re-bind and will have greater chance of executing re-bind in parallel with swapbuffers on webprocess side. > > GLX pixmap references the contents of the original pixmap. The texture may or may not use the same storage (entirely implementation dependent). If the texture uses a different storage, it needs to be updated with a call to glxBindTexImageExt. > > One way to ensure synchronisation is to have a glxwaitgl after swapbuffers call in webprocess side. This would ensure that all gl calls are handled before X rendering calls. Have a GLXWaitX before bind call which ensures that all X rendering calls are done prior to GL calls. That's all good, but calls to GLXWaitX or XSync or XFlash do not guarantee correct synchronization with something on different thread or process. Even if you've executed GLXWaitX/XSync just before texture re-bind you can get thread switch after XSync and deallocation of the window on another thread/process. This just hides the problem and makes it very difficult to reproduce, but doesn't solve it correctly.
(In reply to comment #19) > (In reply to comment #16) > > (In reply to comment #13) > > > (In reply to comment #12) > > > How does it break? Could you write how to reproduce the break? > > > I'm running Qt build and don't see any problem with it. > > > And about "not the right place". > > > We have do swapbuffers on webprocess and have to do matching operation > That's all good, but calls to GLXWaitX or XSync or XFlash do not guarantee correct synchronization with something on different thread or process. Even if you've executed GLXWaitX/XSync just before texture re-bind you can get thread switch after XSync and deallocation of the window on another thread/process. This just hides the problem and makes it very difficult to reproduce, but doesn't solve it correctly. That was for updating texture content in swapbuffers vs doing it before rendering. As mentioned above, it is not part of the proposed change and could be discussed later. For Window deletion part: Any easy way to reproduce the issue?? Would keeping the display open in WebProcess side(like in UI side) solve the issue??
> For Window deletion part: > Any easy way to reproduce the issue?? No. I can only reproduce it running all webgl tests in parallel with the build. It reproduced now once of 5-10 runs. I'd like to see, will it ever show up on build bots. I'm watching for flaky webgl tests. > Would keeping the display open in WebProcess side(like in UI side) solve the issue?? No. I tried.
Comment on attachment 183812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review A more general question/comment: could we abstract these details inside GLPlatformSurface (in this case implementation of GLXTransportSurface)? Then we just be careful with regards to the lifetime of GLPlatformSurface objects. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:390 > + // Add platform layer to the list of the delete pending layers to make sure that > + // they stay until using it object is deleted on UI side (till renderNextFrame message). > + m_sharedPlatformLayersToFlush.append(sharedSurface); I might be wrong, but seems to me that destroyCanvas() is called only during the execution of CoordinatedLayerTreeHost::flushPendingLayerChanges(), which will eventually call flushPendingImageBackingChanges() anyway. So we could simply store those surfaces in m_sharedPlatformLayersToDelete directly, and totally skipping this second list. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:402 > + scheduleLayerFlush(); Is this necessary. Can this function be called outside the scope of CoordinatedLayerTreeHost::flushPendingLayerChanges()?
Comment on attachment 183812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:390 >> + m_sharedPlatformLayersToFlush.append(sharedSurface); > > I might be wrong, but seems to me that destroyCanvas() is called only during the execution of CoordinatedLayerTreeHost::flushPendingLayerChanges(), which will eventually call flushPendingImageBackingChanges() anyway. So we could simply store those surfaces in m_sharedPlatformLayersToDelete directly, and totally skipping this second list. It is also called now from layer purgeBackingStores which is also called from layer deallocation. >> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:402 >> + scheduleLayerFlush(); > > Is this necessary. Can this function be called outside the scope of CoordinatedLayerTreeHost::flushPendingLayerChanges()? Yes, may be it is not needed now in current version of patch. Layer delete also forces layer flush.
(In reply to comment #23) > (From update of attachment 183812 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review > With latest baseline, we shouldn't be synchronizing(i.e which results in calls to swapbuffer or drawing the texture) any layer(having a canvas) marked for deletion. Are we still having the issue now??
(In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 183812 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review > > > > With latest baseline, we shouldn't be synchronizing(i.e which results in calls to swapbuffer or drawing the texture) any layer(having a canvas) marked for deletion. What do you mean? > Are we still having the issue now?? I'll try later today.
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > (From update of attachment 183812 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review > > > > > > > With latest baseline, we shouldn't be synchronizing(i.e which results in calls to swapbuffer or drawing the texture) any layer(having a canvas) marked for deletion. > > What do you mean? We don't try to update/paint the contents of a graphicssurface on UI side if it is already marked for deletion.
(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > (In reply to comment #23) > > > > (From update of attachment 183812 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review > > > > > > > > > > With latest baseline, we shouldn't be synchronizing(i.e which results in calls to swapbuffer or drawing the texture) any layer(having a canvas) marked for deletion. > > > > What do you mean? > > We don't try to update/paint the contents of a graphicssurface on UI side if it is already marked for deletion. Could you point me to the commit?
(In reply to comment #27) > (In reply to comment #26) > Could you point me to the commit? http://trac.webkit.org/changeset/141247
(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > Could you point me to the commit? > > http://trac.webkit.org/changeset/141247 It is unrelated. It solves duplicated delete, but doesn't do anything about race condition. Canvas X11 window is deleted when webgl context deleted before the layer delete.
Comment on attachment 183812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review I'm sure we can find a more explicit solution; Since a canvas can only be used in one layer, we should be able to free it after we get the next RenderNextFrame, like we do with atlases. In any case, this code is very hard to read and does things it's not supposed to (see comments). > Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:50 > + if (m_window) > + destroyWindow(m_window); > if (m_configVisualInfo) { > XFree(m_configVisualInfo); > m_configVisualInfo = 0; > } Can you use the new OwnPtrX11 for this? > Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:148 > + void reSizeWindow(const IntRect&); It should really be resize and not reSize > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:388 > + > + // If content layer was deleted it have to be reset immediately at the > + // texture mapper because webprocess shared window will be deleted. > + if (!m_contentsLayer) > + m_layer->flushCompositingStateForThisLayerOnly(this); No way. m_contentsLayer is used for other things, not only for WebGL on X11. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:821 > + m_coordinator->destroyCanvas(id(), m_canvasSharedPlatformLayer, false); boolean trap > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:825 > + m_coordinator->destroyCanvas(id(), m_canvasSharedPlatformLayerToDestroy, false); ditto
> we should be able to free it after we get the next RenderNextFrame, like we do with atlases. Yes, I was looking at the atlases when doing this. > Can you use the new OwnPtrX11 for this? Do you mean move surface ownership to the CoordinatedLayerTreeHost completely? I, actually, wanted to avoid this and just keep the shared window data referenced where it is required and not change behavior for win or mac implementations. At the same time it is easy to add the same referencing to other ports if necessary. We can't keep canvas ownership in the layer because layers are deleted immediately and only list of deleted layers is kept until next layer flash. That's why I had 2 lists of deleted canvases - 1st is to wait until next layer flush, 2nd is to keep shared window referenced until next renderNextFrame after flush. Also moving shared surface ownership affects webkit1. The question also, do we really need this? Commit r139725 have hidden problem well enough and I can only reproduce it by running webgl layout tests several times with pixel tests enabled and increased timeout in parallel with "make -j12" webkit build. It crashes mostly in webgl-composite-modes or webgl-composite-modes-repaint tests (because they have 8 canvases and have higher probability of the crash) and doesn't crash without pixel tests enabled, IMHO, because pixel tests require extra paint in snapshot. Also, it is likely that we have problem only on mesa (bug in mesa) because spec. says that glXBindTexImage/glXReleaseTexImage should be NoOp if window is invalid or deleted.
(In reply to comment #31) >It is unrelated. It solves duplicated delete, but doesn't do anything about >race condition. Canvas X11 window is deleted when webgl context deleted >before the layer delete. Apart from that the changeset also solved the following situation indirectly: Before this changeset we always tried to sync the canvas even if it was marked for deletion which doesn't happen anymore. But I understand that the case here is when in current frame, we are trying to access the shared buffers while deletion is ongoing in WebProcess Side. > Can you use the new OwnPtrX11 for this? I think noam meant about freeing the X11 resources. The code in question doesn’t exist anymore, so I guess we could ignore it for now. > Do you mean move surface ownership to the CoordinatedLayerTreeHost completely? > The question also, do we really need this? Commit r139725 have hidden problem well enough and I can only reproduce it by running webgl layout tests several times with pixel tests enabled and increased timeout in parallel with "make -j12" webkit build. I have mixed feelings on this one. On one side it seems we are trying to workaround some issues in extension implementation. On positive side it is nice to ensure that crashes don't happen. >It crashes mostly in webgl-composite-modes or webgl-composite-modes-repaint >tests (because they have 8 canvases and have higher probability of the crash) >and doesn't crash without pixel tests enabled, IMHO, because pixel tests >require extra paint in snapshot. Also, it is likely that we have problem only >on mesa (bug in mesa) because spec. says that >glXBindTexImage/glXReleaseTexImage should be NoOp if window is invalid or >deleted. k, With pixel tests we are trying to take a snapshot in the UI process side.Spec also says that storage for GLX pixmap will be freed when it is not current to any client and all color buffers that are bound to a texture object have been released. Is the situation now so that window and glxpixmap share the same storage and it gets deleted when Window is deleted. Now, with glxpixmap we still try to access the deleted storage? Should we try to make a small test case and file a bug to Mesa??
(In reply to comment #32) > (In reply to comment #31) > > The question also, do we really need this? Commit r139725 have hidden k, Ateast I have not been able to reproduce this nor do I see the issue on our bots. I would propose to close this issue for now and come back to this in future if we see the problem ??
(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #31) > > > The question also, do we really need this? Commit r139725 have hidden > > k, Ateast I have not been able to reproduce this nor do I see the issue on our bots. I would propose to close this issue for now and come back to this in future if we see the problem ?? Bots don't run pixel tests. Also, not crashing on the bot running set of simple tests. I don't think it is great if it might crash for user when he runs some video encoding in parallel with browser.
Closing this bug because the EFL port has been removed from trunk. If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.