Bug 57812 – Re-entrancy issue with setIsAcceleratedCompositingActive

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Antoine Labour]

Re-entrancy issue with setIsAcceleratedCompositingActive

[Antoine Labour]

Created attachment 88181 [details]
Patch

[Antoine Labour]

http://code.google.com/p/chromium-os/issues/detail?id=13821 shows this issue. In chrome, LayerRendererChromium::create might re-enter WebViewImpl::graphicsContext3D, but at that point m_temporaryOnscreenGraphicsContext3D is already reset, whereas m_layerRenderer is not set yet. So we end up creating a new context that isn't used by the compositor, and we then give the compositor a texture ID that doesn't belong to the right context.
This patch makes sure we only reset m_temporaryOnscreenGraphicsContext3D after m_layerRenderer has been set.

[Vangelis Kokkevis]

(In reply to comment #2)
> http://code.google.com/p/chromium-os/issues/detail?id=13821 shows this issue. In chrome, LayerRendererChromium::create might re-enter WebViewImpl::graphicsContext3D, but at that point m_temporaryOnscreenGraphicsContext3D is already reset, whereas m_layerRenderer is not set yet. So we end up creating a new context that isn't used by the compositor, and we then give the compositor a texture ID that doesn't belong to the right context.
> This patch makes sure we only reset m_temporaryOnscreenGraphicsContext3D after m_layerRenderer has been set.

I'm not sure I see how LayerRendererChromium::create() can re-enter WebViewImpl::graphicsContext3D but I do agree that if that's possible your fix is necessary.  Have you been able to trace the calling sequence that results in the re-entrancy?

[Antoine Labour]

LayerRendererChromium::create does some GL calls, some of which may block (e.g. compile a shader and check for result). While it's blocking on the IPC to the gpu process, it may process messages from other channels, e.g. in my case from the PPAPI process, which may end up trying to access the compositor's context.

That being said,, I fear that this behavior may be causing a lot of different problems, and it's possible that it's better to make sure that it doesn't happen.

[Eric Seidel (no email)]

How do we test this?

[Kenneth Russell]

Comment on attachment 88181 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:2425
> +            m_temporaryOnscreenGraphicsContext3D = 0;

This needs to occur in both code paths, whether or not the LayerRendererChromium succeeded in being created, to avoid a resource leak. I suggest using m_temporaryOnscreenGraphicsContext3D.clear() to do so.

As long as you're convinced that there are no other problems in allowing the reentrancy, this otherwise looks fine. Please upload a revised patch (and set cq?) and I'll r+ it. Note that the 'webkit-patch upload' script assists with this.

[Antoine Labour]

I'm looking at the more general re-entrancy problem. If I still need this patch, I'll update. Otherwise, I'll close.

[Antoine Labour]

I solved the problem in Chrome.