Bug 104733

Summary: Make WebGLRenderingContext inherit from ActiveDOMObject
Product: WebKit Reporter: Brandon Jones <bajones>
Component: WebGLAssignee: Brandon Jones <bajones>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, cmarrin, danno, dglazkov, dino, d-r, enne, gman, haraken, jamesr, japhet, kbr, mifenton, nduca, ojan.autocc, rwlbuis, tonikitoo, ulan, webkit.review.bot, yong.li.webkit, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 76225    
Bug Blocks: 126947    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for testing
none
Patch none

Description Brandon Jones 2012-12-11 15:53:06 PST
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.
Comment 1 Kenneth Russell 2012-12-11 15:55:30 PST
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.
Comment 2 Brandon Jones 2012-12-13 13:26:35 PST
Created attachment 179321 [details]
Patch
Comment 3 Dean Jackson 2012-12-13 19:43:14 PST
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 4 WebKit Review Bot 2012-12-13 22:59:14 PST
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
Comment 5 Brandon Jones 2012-12-14 16:46:34 PST
Created attachment 179556 [details]
Patch
Comment 6 Dean Jackson 2012-12-14 17:01:16 PST
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 7 WebKit Review Bot 2012-12-15 07:42:43 PST
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
Comment 8 Brandon Jones 2012-12-17 12:04:33 PST
Created attachment 179777 [details]
Patch
Comment 9 Kenneth Russell 2012-12-17 13:13:17 PST
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 10 WebKit Review Bot 2012-12-17 13:18:26 PST
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
Comment 11 Kenneth Russell 2012-12-17 13:52:06 PST
(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?
Comment 12 Brandon Jones 2012-12-19 14:50:27 PST
Created attachment 180229 [details]
Patch
Comment 13 Brandon Jones 2012-12-19 14:54:51 PST
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.
Comment 14 Kenneth Russell 2013-01-04 20:34:34 PST
Created attachment 181424 [details]
Patch
Comment 15 Kenneth Russell 2013-01-04 20:38:45 PST
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 16 Brandon Jones 2013-01-04 20:44:44 PST
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 17 Adam Barth 2013-01-05 12:52:53 PST
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.
Comment 18 Adam Barth 2013-01-05 12:55:52 PST
> 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?
Comment 19 Kenneth Russell 2013-01-07 19:48:21 PST
Created attachment 181626 [details]
Patch for testing

Properly clear out texture ID from compositor based on feedback from jamesr
Comment 20 EFL EWS Bot 2013-01-07 20:20:26 PST
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 21 WebKit Review Bot 2013-01-08 03:10:29 PST
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
Comment 22 Kenneth Russell 2013-01-08 10:24:06 PST
(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 23 Adam Barth 2013-01-08 11:26:49 PST
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 24 Kenneth Russell 2013-01-08 13:34:02 PST
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.
Comment 25 Kenneth Russell 2013-01-08 15:23:47 PST
Created attachment 181783 [details]
Patch
Comment 26 Adam Barth 2013-01-08 15:32:05 PST
Comment on attachment 181783 [details]
Patch

Thanks for explaining the patch!
Comment 27 WebKit Review Bot 2013-01-08 18:23:19 PST
Comment on attachment 181783 [details]
Patch

Clearing flags on attachment: 181783

Committed r139142: <http://trac.webkit.org/changeset/139142>
Comment 28 WebKit Review Bot 2013-01-08 18:23:26 PST
All reviewed patches have been landed.  Closing bug.