In Chromium WebGL contexts are not being garbage collected eagerly enough. This is causing unexpected behavior in cases where many WebGL contexts are created in succession, such as refreshing the page multiple times during development. WebGLRenderingContexts should follow the pattern set by AudioContext and inherit from ActiveDOMObject to quickly detect when the page containing the context is closed/refreshed/navigated and actively release GPU resources. An attempt to hint the garbage collector to collect WebGLRenderingContexts more eagerly was made in Bug 76225, but some problems still persist. The use of ActiveDOMObject to detect WebGLRenderingContext end of life was suggested in the comments for that bug.
To be clear: there were significant and valid objections to using a GC hint in the fix for Bug 76225. Using ActiveDOMObject to decide when to tear down the WebGLRenderingContext will work better for all ports.
Created attachment 179321 [details] Patch
Comment on attachment 179321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179321&action=review > Source/WebCore/ChangeLog:18 > + (WebCore): Remove this useless line. > Source/WebCore/ChangeLog:22 > + (WebCore::WebGLRenderingContext::WebGLRenderingContext): > + (WebCore::WebGLRenderingContext::~WebGLRenderingContext): > + (WebCore::WebGLRenderingContext::destroyGraphicsContext3D): > + (WebCore::WebGLRenderingContext::stop): DarinA always encourages me to write per-function comments, especially when they are easy like this, and I appreciate it. Let me pay it forward! > Source/WebCore/ChangeLog:24 > + (WebGLRenderingContext): And this useless line. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4597 > + // Will this cause anything else to panic and die? You should probably preface this with FIXME. It's a fairly scary statement. > Source/WebCore/html/canvas/WebGLRenderingContext.h:321 > + // Document notification Maybe be clear that this block of functions comes from ActiveDOMObject? I did a quick search through WebCore and sometimes people do this, other times not.
Comment on attachment 179321 [details] Patch Attachment 179321 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15318541 New failing tests: platform/chromium/virtual/threaded/compositing/webgl/webgl-reflection.html platform/chromium/virtual/threaded/compositing/webgl/webgl-no-alpha.html
Created attachment 179556 [details] Patch
Comment on attachment 179556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179556&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:617 > + // The drawing buffer holds a context reference. It must also be destroyed > + // in order for the context to be released Nit: Comment needs "."
Comment on attachment 179556 [details] Patch Attachment 179556 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15362075 New failing tests: platform/chromium/virtual/threaded/compositing/webgl/webgl-reflection.html
Created attachment 179777 [details] Patch
Comment on attachment 179777 [details] Patch Looks good. From an offline discussion it sounds like this patch has undergone good testing. Let's let the final patch pass the EWS and then set cq+.
Comment on attachment 179777 [details] Patch Attachment 179777 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15362861 New failing tests: platform/chromium/virtual/threaded/compositing/webgl/webgl-no-alpha.html platform/chromium/virtual/threaded/compositing/webgl/webgl-repaint.html
(In reply to comment #10) > (From update of attachment 179777 [details]) > Attachment 179777 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/15362861 > > New failing tests: > platform/chromium/virtual/threaded/compositing/webgl/webgl-no-alpha.html > platform/chromium/virtual/threaded/compositing/webgl/webgl-repaint.html It's suspicious that some WebGL tests have started to fail with this patch. Are these crashes reproducible locally? What happens if a WebGL context is used after it's stopped? Are there sufficient guards in the WebGLRenderingContext implementation (in particular, all of the OpenGL-related entry points) to have m_context deleted out from under it?
Created attachment 180229 [details] Patch
NOTE: The patch uploaded on 2012-12-19 has known issues with the threaded compositor and should not be used in production code. It is uploaded here for archival purposes only.
Created attachment 181424 [details] Patch
There was a missing call to suspendIfNeeded in the earlier patch which was causing spurious assertion failures in debug builds. The reason this is failing with Chromium's threaded compositor is that the impl tree is referencing a stale texture ID after the DrawingBuffer has been cleared and it's deleted its resources. I've made a couple of attempts to tell the cc tree that it shouldn't draw that layer any more but haven't succeeded yet. I don't know how to run DumpRenderTree manually with the threaded compositor so can't catch it in the debugger yet. Would certainly appreciate any advice.
Comment on attachment 181424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181424&action=review Looks good overall, but obviously you need approval from a someone besides me. > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:470 > + fprintf(stderr, "WebGLRenderingContext::WebGLRenderingContext %p\n", this); Remove debugging statements > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:603 > + fprintf(stderr, "WebGLRenderingContext::~WebGLRenderingContext %p\n", this); Same here > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4609 > + fprintf(stderr, "WebGLRenderingContext::stop %p\n", this); And here
Comment on attachment 181424 [details] Patch There's not ChangeLog, so I'm not sure what you're trying to accomplish by using ActiveDOMObject. Are you just looking for the stop() callback? Most ActiveDOMObjects also use the [ActiveDOMObject] IDL attribute, which tells the garbage collector to keep the object's wrapper alive why hasPendingActivity returns true.
> In Chromium WebGL contexts are not being garbage collected eagerly enough. This is causing unexpected behavior in cases where many WebGL contexts are created in succession, such as refreshing the page multiple times during development. WebGLRenderingContexts should follow the pattern set by AudioContext and inherit from ActiveDOMObject to quickly detect when the page containing the context is closed/refreshed/navigated and actively release GPU resources. Hum... It's not clear to me what using ActiveDOMObject is the right way to solve this problem. Do we understand why the WebGL context isn't being garbage collected when you expect? As an example, an app-like web page that runs for a long time will probably want its WebGL context garbage collected before ActiveDOMObject::stop gets called given that stop won't be called for the lifetime of the app. AudioContext is different because it's "active" in the sense that it's actively producing sound that we want to stop when we navigate away from the page. Does WebGL actively produce anything?
Created attachment 181626 [details] Patch for testing Properly clear out texture ID from compositor based on feedback from jamesr
Comment on attachment 181626 [details] Patch for testing Attachment 181626 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15738976
Comment on attachment 181626 [details] Patch for testing Attachment 181626 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15744893 New failing tests: fast/forms/range/slider-delete-while-dragging-thumb.html http/tests/security/webgl-remote-read-remote-image-allowed.html fast/canvas/webgl/error-reporting.html fast/loader/text-document-wrapping.html fast/canvas/webgl/context-lost-restored.html fast/canvas/webgl/compressed-tex-image.html fast/canvas/webgl/webgl-specific.html fast/canvas/webgl/buffer-bind-test.html fast/canvas/webgl/WebGLContextEvent.html fast/forms/range/slider-mouse-events.html fast/canvas/webgl/tex-sub-image-2d-bad-args.html fast/canvas/webgl/index-validation-copies-indices.html fast/canvas/webgl/index-validation-verifies-too-many-indices.html fast/canvas/webgl/instanceof-test.html fast/canvas/webgl/uniform-array-length-overflow.html fast/canvas/webgl/css-webkit-canvas.html fast/canvas/webgl/gl-bind-attrib-location-test.html fast/canvas/webgl/tex-image-with-format-and-type.html fast/canvas/webgl/premultiplyalpha-test.html fast/canvas/webgl/draw-elements-out-of-bounds.html fast/canvas/webgl/texture-active-bind.html fast/loader/javascript-url-in-object.html fast/canvas/webgl/null-uniform-location.html fast/canvas/webgl/shader-deleted-by-accessor.html fast/canvas/webgl/canvas-2d-webgl-texture.html fast/canvas/webgl/invalid-passed-params.html http/tests/security/window-events-clear-domain.html fast/canvas/webgl/context-destroyed-crash.html
(In reply to comment #18) > > In Chromium WebGL contexts are not being garbage collected eagerly enough. This is causing unexpected behavior in cases where many WebGL contexts are created in succession, such as refreshing the page multiple times during development. WebGLRenderingContexts should follow the pattern set by AudioContext and inherit from ActiveDOMObject to quickly detect when the page containing the context is closed/refreshed/navigated and actively release GPU resources. > > Hum... It's not clear to me what using ActiveDOMObject is the right way to solve this problem. Do we understand why the WebGL context isn't being garbage collected when you expect? As an example, an app-like web page that runs for a long time will probably want its WebGL context garbage collected before ActiveDOMObject::stop gets called given that stop won't be called for the lifetime of the app. The WebGL context wasn't being GC'd eagerly enough in Chrome because of V8's generational garbage collector. It used to be the case that DOM nodes could only be collected in full GCs, but many tests only provoke young generation GCs, so the WebGL contexts were staying alive much longer than expected or desired. haraken's recent work to support collection of DOM nodes in young gen collections improved the situation, but even so, if the HTMLCanvasElement is promoted to the old gen, then it won't be collected as eagerly as in non-generational GCs like JSC's. I added a hack in Bug 76255 to hint the GC to run when navigating away from a page containing a WebGL canvas. This was hotly debated and was not an ideal solution. The desired behavior is to discard the heavyweight GPU resources as quickly as possible when the page is shut down. This is what this patch implements, and it removes the previous hack. > AudioContext is different because it's "active" in the sense that it's actively producing sound that we want to stop when we navigate away from the page. Does WebGL actively produce anything? WebGL isn't exactly like AudioContext in this regard, but yes, it produces output into a canvas element, and doing so requires many GPU resources. Releasing those resources eagerly upon page navigation or reload is necessary to improve robustness of real-world applications like MapsGL. (More work is underway in this area, but this patch is desired permanently -- it is not a stopgap solution.)
Comment on attachment 181626 [details] Patch for testing View in context: https://bugs.webkit.org/attachment.cgi?id=181626&action=review > Source/WebCore/html/canvas/WebGLRenderingContext.h:322 > + // ActiveDOMObject notifications > + virtual void stop(); Makes sense. In that case, you probably want to override hasPendingActivity here and always return false. Also, you're correct to skip the [ActiveDOMObject] IDL attribute.
Comment on attachment 181626 [details] Patch for testing View in context: https://bugs.webkit.org/attachment.cgi?id=181626&action=review >> Source/WebCore/html/canvas/WebGLRenderingContext.h:322 >> + virtual void stop(); > > Makes sense. In that case, you probably want to override hasPendingActivity here and always return false. Also, you're correct to skip the [ActiveDOMObject] IDL attribute. Thanks, new patch coming which does this. Thanks also for the pointer to the [ActiveDOMObject] IDL attribute; I didn't know about that.
Created attachment 181783 [details] Patch
Comment on attachment 181783 [details] Patch Thanks for explaining the patch!
Comment on attachment 181783 [details] Patch Clearing flags on attachment: 181783 Committed r139142: <http://trac.webkit.org/changeset/139142>
All reviewed patches have been landed. Closing bug.