Bug 156689 – Creating a large number of WebGL contexts should recycle older contexts

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

[Antoine Quint]

We currently stop creating WebGL contexts once a maximum of 64 WebGL contexts have been created on a page. Other browsers have a limit of 16 concurrent active WebGL contexts and they lose older contexts when the developer creates a new context, logging a warning to the console. We should follow a similar approach.

[Antoine Quint]

<rdar://problem/19535330>

[Radar WebKit Bug Importer]

<rdar://problem/25774766>

[Antoine Quint]

<rdar://problem/19535330>

[Antoine Quint]

Created attachment 276625 [details]
Patch

[Dean Jackson]

Bit of a worry that mac-debug EWS bombed out.
38815 tests ran as expected, 2671 didn't

You should probably upload a patch before landing to see if that happens again, and is happening all the time on other patches.

[Dean Jackson]

Comment on attachment 276625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276625&action=review

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:3780
> +    // Using SyntheticLostContext means the developer won't be able to force the restoration
> +    // of the context by calling preventDefault() in a "webglcontextlost" event handler.

I wasn't aware we did that on preventDefault for real lost contexts. That sounds like terrible behaviour.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1269
> +    static Vector<GraphicsContext3D*>* s_activeContexts;

Is this exposed to other platforms? I guess they never used it since otherwise you'd have to change their .cpp files too.

I wonder if we should do this for all ports.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:1860
> +void GraphicsContext3D::recycleContext()
> +{
> +#if ENABLE(WEBGL)
> +    if (m_webglContext)
> +        m_webglContext->recycleContext();
> +#endif
> +}

I'm not sure we ever have a GC3D in a situation where ENABLE(WEBGL) is false.

[Said Abou-Hallawa]

Comment on attachment 276625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276625&action=review

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:67
> +const int maxActiveContexts = 16;

This constant can be a member the class and can be moved to the header file also.

private:
    static const int MaxActiveContexts = 16;

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68
> +Vector<GraphicsContext3D*>* GraphicsContext3D::s_activeContexts;

Why do not we use NeverDestroyed template for s_activeContexts? activeContexts does not even need to be a member of GraphicsContext3D class.

static Vector<GraphicsContext3D*>& activeContexts()
{
    static NeverDestroyed<Vector<GraphicsContext3D*>> s_activeContexts;
    return s_activeContexts;
}

You can also get rid of the allocation in the create() and the assertion in the destructor.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:137
>      return context.release();

I think we can use return WTFMove(context) instead. Also the type of the return value can be change to RefPtr.

[Antoine Quint]

Created attachment 276665 [details]
Patch

[Antoine Quint]

Created attachment 276668 [details]
Patch

[Antoine Quint]

Created attachment 276670 [details]
Patch

[Said Abou-Hallawa]

Looks good to me. Unofficial r=me.

[Alexey Proskuryakov]

Debug EWS failures look strange, but possibly related to this patch. I just rebooted the bot that was seeing them to be more sure about what's going on.

[Antoine Quint]

(In reply to comment #12)
> Debug EWS failures look strange, but possibly related to this patch. I just
> rebooted the bot that was seeing them to be more sure about what's going on.

It seems that since rebooting the bots are unable to build at all. FWIW, I can't reproduce the WebGL test timeouts when running the tests locally in debug.

[Antoine Quint]

Created attachment 276711 [details]
Patch

[Antoine Quint]

Actually, I did see some crashes due to the ASSERT(activeContexts().contains(this)) I had added in the deconstructor which was a mistake since contexts can be removed from the list of active contexts prior to being GC'd.

[Alexey Proskuryakov]

I filed bug 156750 to track debug EWS problems.

Maybe there was a change recently that makes WindowServer freeze when running WebKit tests. Or maybe it's this patch that kills them one by one.

[Alexey Proskuryakov]

Comment on attachment 276711 [details]
Patch

Marking r- to get this patch out of EWS queue.

Antoine, could you please work with Ryan Haddad to have this patch tested on Mac Pros manually?

[Antoine Quint]

Created attachment 276821 [details]
Patch

[Antoine Quint]

I think I finally found the cause of the debug crasher. In recycleContext(), we needed to call destroyGraphicsContext3D() after calling forceLostContext(SyntheticLostContext) so as to disassociate the WebGL context and the GraphicsContext3D objects and clear the Open GL context. All WebGL tests now pass for me in debug builds without a crash or timeout. I hope this will keep the mac-debug bot happy this time.

[Antoine Quint]

The ios-sim test failures are not related to this patch.

[Said Abou-Hallawa]

Comment on attachment 276821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276821&action=review

Looks good to me. Unofficial r=me.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:136
> +        contexts.removeFirst(contextToLose);

GraphicsContext3D::recycleContext() calls WebGLRenderingContextBase::recycleContext()
    WebGLRenderingContextBase::recycleContext() calls WebGLRenderingContextBase::destroyGraphicsContext3D()
        WebGLRenderingContextBase::destroyGraphicsContext3D() sets m_context = nullptr
                Setting m_context = nullptr may call GraphicsContext3D()::~GraphicsContext3D() if it is the last reference.
You added activeContexts().removeFirst(this) to GraphicsContext3D()::~GraphicsContext3D(). So I think there is no need to call contexts.removeFirst(contextToLose) here.

Actually I think it is safer and cleaner to call activeContexts().removeFirst() only from the GraphicsContext3D()::~GraphicsContext3D() because of the following reasons:

1. In the destructor of GraphicsContext3D(), we are sure that "this" is still a valid pointer but after that it will no longer be valid. So this is exactly the right time to remove the raw pointer from the activeContexts().
2. This function returns a RefPtr<GraphicsContext3D> which may be ref-counted multiple times.  When calling recycleContext(), for a previously allocated PtrRef<GraphicsContext3D> by this function, might not actually dispose its raw pointer. I am not sure if this can happen or not. But if it happens, that means there will be extra contexts hanging around but not recorded by activeContexts() because we removed their references from the activeContexts() even though their raw pointer are still alive.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:139
> +    contexts.append(context.get());

If you decide to remove the  contexts.removeFirst(contextToLose), then you will need to check:
if (contexts.size() >= MaxActiveContexts)
    return nullptr;

before this append(). You may also want to move the code which tries to find space in activeContexts() before creating the the GraphicsContext3D.

if (contexts.size() >= MaxActiveContexts) {
    ...
}

if (contexts.size() >= MaxActiveContexts)
   return nullptr;

RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs, hostWindow, renderStyle));

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:349
> +    activeContexts().removeFirst(this);

I think if you have removing the raw pointer from the activeContexts() only in the destructor, then you can safely asserts that activeContexts().contains(this).

> Source/WebCore/platform/graphics/win/GraphicsContext3DWin.cpp:63
>          return 0;

0 ==> nullptr.

[Alexey Proskuryakov]

Comment on attachment 276821 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276821&action=review

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:77
> +    return WTFMove(context);

cq-, as this should be "return context".

[Antoine Quint]

(In reply to comment #21)
> Comment on attachment 276821 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276821&action=review
> 
> Looks good to me. Unofficial r=me.
> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:136
> > +        contexts.removeFirst(contextToLose);
> 
> GraphicsContext3D::recycleContext() calls
> WebGLRenderingContextBase::recycleContext()
>     WebGLRenderingContextBase::recycleContext() calls
> WebGLRenderingContextBase::destroyGraphicsContext3D()
>         WebGLRenderingContextBase::destroyGraphicsContext3D() sets m_context
> = nullptr
>                 Setting m_context = nullptr may call
> GraphicsContext3D()::~GraphicsContext3D() if it is the last reference.
> You added activeContexts().removeFirst(this) to
> GraphicsContext3D()::~GraphicsContext3D(). So I think there is no need to
> call contexts.removeFirst(contextToLose) here.
> 
> Actually I think it is safer and cleaner to call
> activeContexts().removeFirst() only from the
> GraphicsContext3D()::~GraphicsContext3D() because of the following reasons:
> 
> 1. In the destructor of GraphicsContext3D(), we are sure that "this" is
> still a valid pointer but after that it will no longer be valid. So this is
> exactly the right time to remove the raw pointer from the activeContexts().
> 2. This function returns a RefPtr<GraphicsContext3D> which may be
> ref-counted multiple times.  When calling recycleContext(), for a previously
> allocated PtrRef<GraphicsContext3D> by this function, might not actually
> dispose its raw pointer. I am not sure if this can happen or not. But if it
> happens, that means there will be extra contexts hanging around but not
> recorded by activeContexts() because we removed their references from the
> activeContexts() even though their raw pointer are still alive.

The thing is that if the context is lost for other reasons than making room for a new context, then the call to destroyGraphicsContext3D() will not result in any action and thus the destructor will not be called right away. This code doesn't make assumptions about the state of the context.

> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:139
> > +    contexts.append(context.get());
> 
> If you decide to remove the  contexts.removeFirst(contextToLose), then you
> will need to check:
> if (contexts.size() >= MaxActiveContexts)
>     return nullptr;
> 
> before this append(). You may also want to move the code which tries to find
> space in activeContexts() before creating the the GraphicsContext3D.
> 
> if (contexts.size() >= MaxActiveContexts) {
>     ...
> }
> 
> if (contexts.size() >= MaxActiveContexts)
>    return nullptr;

I'm not sure I understand this comment. The goal of this patch is to always return a context and recycle older context if need be.

> RefPtr<GraphicsContext3D> context = adoptRef(new GraphicsContext3D(attrs,
> hostWindow, renderStyle));
> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:349
> > +    activeContexts().removeFirst(this);
> 
> I think if you have removing the raw pointer from the activeContexts() only
> in the destructor, then you can safely asserts that
> activeContexts().contains(this).

I don't think we can have such an assertion, per my point above about contexts being lost for other reasons than there being too many active contexts, which means calling recycleContext() will not result in the destructor being called. In that case the context may have been removed from the active context list and will only be destructed when GC kicks in at a later time.

> 
> > Source/WebCore/platform/graphics/win/GraphicsContext3DWin.cpp:63
> >          return 0;
> 
> 0 ==> nullptr.

OK. Will change that upon landing if the rest of the patch is fine as-is.

[Antoine Quint]

Created attachment 276836 [details]
Patch

[Antoine Quint]

Actually, Said, I think your comments were correct. I was confused because I used WebGLRenderingContextBase::stop() instead of WebGLRenderingContextBase::destroyGraphicsContext3D() for a while, and that had checks for lost contexts, destroyGraphicsContext3D() does not.

[Antoine Quint]

Created attachment 276843 [details]
Patch

[Dean Jackson]

Comment on attachment 276843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276843&action=review

Nearly there. Just some small fixes.

> Source/WebCore/platform/graphics/GraphicsContext3D.h:1269
> +    static const int MaxActiveContexts = 16;

Since this is only used in the GraphicsContext3DMac.cpp file, it doesn't need to be a class attribute. Just use a static variable in the .cpp (like it was before this patch).

Alternatively, we could put it here and encourage other ports to do the same. This would also allow us to be more descriptive in the error message (in Base.cpp). We could say "We only allow %d active contexts at a time.", MaxActiveContexts)

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:77
> +    return WTFMove(context);

This should be return context;

Or, you could do it all in one line. return adoptRef(*new Graphics....)

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:36
> +    return context->m_private ? WTFMove(context) : 0;

Same here, just return context. And 0 should be nullptr.

> Source/WebCore/platform/graphics/win/GraphicsContext3DWin.cpp:66
> +    return WTFMove(context);

return context here too.

[Dean Jackson]

r=me if you make those changes.

[Antoine Quint]

Created attachment 276925 [details]
Patch

[Antoine Quint]

(In reply to comment #28)
> r=me if you make those changes.

Made the suggested changes.

[Dean Jackson]

Comment on attachment 276925 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=276925&action=review

> Source/WebCore/ChangeLog:28
> +        * html/canvas/WebGLRenderingContextBase.h:

You probably should note that you changed PassRefPtr into RefPtr.

[Antoine Quint]

(In reply to comment #31)
> Comment on attachment 276925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276925&action=review
> 
> > Source/WebCore/ChangeLog:28
> > +        * html/canvas/WebGLRenderingContextBase.h:
> 
> You probably should note that you changed PassRefPtr into RefPtr.

Will add this when landing.

[Antoine Quint]

Created attachment 276928 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 276928 [details]
Patch for landing

Clearing flags on attachment: 276928

Committed r199819: <http://trac.webkit.org/changeset/199819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.