Bug 104733 – Make WebGLRenderingContext inherit from ActiveDOMObject

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Brandon Jones]

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.

[Kenneth Russell]

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.

[Brandon Jones]

Created attachment 179321 [details]
Patch

[Dean Jackson]

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.

[WebKit Review Bot]

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

[Brandon Jones]

Created attachment 179556 [details]
Patch

[Dean Jackson]

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

[WebKit Review Bot]

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

[Brandon Jones]

Created attachment 179777 [details]
Patch

[Kenneth Russell]

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

[WebKit Review Bot]

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

[Kenneth Russell]

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

[Brandon Jones]

Created attachment 180229 [details]
Patch

[Brandon Jones]

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.

[Kenneth Russell]

Created attachment 181424 [details]
Patch

[Kenneth Russell]

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.

[Brandon Jones]

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

[Adam Barth]

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.

[Adam Barth]

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

[Kenneth Russell]

Created attachment 181626 [details]
Patch for testing

Properly clear out texture ID from compositor based on feedback from jamesr

[EFL EWS Bot]

Comment on attachment 181626 [details]
Patch for testing

Attachment 181626 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15738976

[WebKit Review Bot]

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

[Kenneth Russell]

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

[Adam Barth]

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.

[Kenneth Russell]

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.

[Kenneth Russell]

Created attachment 181783 [details]
Patch

[Adam Barth]

Comment on attachment 181783 [details]
Patch

Thanks for explaining the patch!

[WebKit Review Bot]

Comment on attachment 181783 [details]
Patch

Clearing flags on attachment: 181783

Committed r139142: <http://trac.webkit.org/changeset/139142>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.