Bug 87873 – [chromium] Single thread proxy should not tick animations unless the layer renderer has been initialized

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Currently, in CCSingleThreadProxy::initializeLayerRenderer(), the reference to m_contextBeforeInitialization is released and passed to m_layerTreeHostImpl->initializeLayerRenderer(...), but if that call fails, then m_contextBeforeInitialization is null, so subsequent calls to CCSingleThreadProxy::initializeLayerRenderer() will cause a crash. It is reasonable to retry initialization, because it can fail due to memory issues that may eventually be resolved.

Created attachment 144862 [details]
Patch

[James Robinson]

Comment on attachment 144862 [details]
Patch

If initializeLayerRenderer() fails then we should stop using the compositor completely and never hit the function again - initialization errors are fatal and permanent until the embedder attempts to instantiate compositing again.

[James Robinson]

(In reply to comment #0)
> Currently, in CCSingleThreadProxy::initializeLayerRenderer(), the reference to m_contextBeforeInitialization is released and passed to m_layerTreeHostImpl->initializeLayerRenderer(...), but if that call fails, then m_contextBeforeInitialization is null, so subsequent calls to CCSingleThreadProxy::initializeLayerRenderer() will cause a crash. It is reasonable to retry initialization, because it can fail due to memory issues that may eventually be resolved.

The embedder should decide to retry by passing in a new context, the compositor shouldn't attempt to retry with the existing context.

Created attachment 144875 [details]
Patch

(In reply to comment #3)
> (In reply to comment #0)
> > Currently, in CCSingleThreadProxy::initializeLayerRenderer(), the reference to m_contextBeforeInitialization is released and passed to m_layerTreeHostImpl->initializeLayerRenderer(...), but if that call fails, then m_contextBeforeInitialization is null, so subsequent calls to CCSingleThreadProxy::initializeLayerRenderer() will cause a crash. It is reasonable to retry initialization, because it can fail due to memory issues that may eventually be resolved.
>
> The embedder should decide to retry by passing in a new context, the compositor shouldn't attempt to retry with the existing context.
Sg. If we fail with the first context. We won't try with it again.

[James Robinson]

Comment on attachment 144875 [details]
Patch

This still feels fishy - nobody should be calling initializeLayerRenderer() if we returned false.  Where's this call coming from?

[James Robinson]

Comment on attachment 144875 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:164
>      ASSERT(m_contextBeforeInitialization);
>      {
>          DebugScopedSetImplThread impl;
> -        bool ok = m_layerTreeHostImpl->initializeLayerRenderer(m_contextBeforeInitialization.release(), UnthrottledUploader);
> +        bool ok = m_contextBeforeInitialization && m_layerTreeHostImpl->initializeLayerRenderer(m_contextBeforeInitialization.release(), UnthrottledUploader);

Clearly somebody is confused about the expectations here - either the ASSERT() is wrong or the code is wrong

Created attachment 144886 [details]
Patch

(In reply to comment #5)
> (From update of attachment 144875 [details])
> This still feels fishy - nobody should be calling initializeLayerRenderer() if we returned false.  Where's this call coming from?
Someone started the animation timer before the layer renderer was initialized, and when it fires we land in this code.
If the CCSingleThreadProxy is destroyed, then this timer is stopped, but it doesn't seems like that's happening, so I've
put guard code in the timer callback to ensure that we only tick if we've got an initialized layer renderer.

[Dana Jansens]

To bikeshed, this does feel like the wrong solution to me because ticking calls updateLayers which does in fact do the initialization. We could ensure that initialization can't be attempted twice, if that's the desired behaviour, in CCLTH::updateLayers, by having it early-out instead.

[James Robinson]

(In reply to comment #7)
> Created an attachment (id=144886) [details]
> Patch
> 
> (In reply to comment #5)
> > (From update of attachment 144875 [details] [details])
> > This still feels fishy - nobody should be calling initializeLayerRenderer() if we returned false.  Where's this call coming from?
> Someone started the animation timer before the layer renderer was initialized, and when it fires we land in this code.
> If the CCSingleThreadProxy is destroyed, then this timer is stopped, but it doesn't seems like that's happening

I'd like to understand why this isn't happening.  Can we repro?

[James Robinson]

Comment on attachment 144886 [details]
Patch

R- until we have a better understanding of why the timer is still alive in this case

Here's the chain of events:

1. An animation is added. This can happen before we attempt to initialize the layer renderer, and causes a timer to be set to schedule a composite.

2. We get around to initializing the layer renderer. More specifically:

CCLayerTreeHost::initializeLayerRenderer()
  - calls CCSingleThreadProxy::initializeLayerRenderer() 
  - when it fails, calls 
    -> WebLayerTreeViewClientAdapter::didRecreateContext(false)
    -> WebViewImpl::didRebindGraphicsContext(false)
    -> WebViewImpl::setIsAcceleratedCompositingActive(false)

      *** NOTE: this does not cause WebViewImpl::m_layerTreeView to be deleted, nor, I think, should it.
          This means that the proxy is not destroyed nor is its timer stopped.

    -> RenderWidget::didDeactivateCompositor()
    -> (via IPC)RenderWidgetHostImpl::OnMsgDidActivateAcceleratedCompositing(false)
    -> RenderWidgetHostViewAura::OnAcceleratedCompositingStateChange()
    -> ui::Layer::set_scale_content(false)

3. The timer fires and indirectly tries to reinitialize the renderer (by trying to composite). It should not do this, a composite should only be attempted if we have a layer renderer.

Created attachment 145096 [details]
Patch

CCLayerTreeHost::finishAllRendering is called iff we are shutting down
accelerated compositing. If we are shutting down accelerated compositing, it
makes sense to stop CCSingleThreadProxy's timer here so it never fires after
we've switched to software mode. This required a minor change to
finishAllRendering. Previously, CCLTH::finishAllRendering bailed early if we
hadn't initialized the layer renderer (which foils the plan to stop the timer).
Now, the proxy's know that this function may be called without an initialized
layer renderer and have the appropriate checks.

I've added a unit test to ensure that we do indeed stop the timer in this case.

Created attachment 145103 [details]
Patch

Sorry for the spam. Removed some code that would have surely been contentious.

[James Robinson]

Comment on attachment 145103 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:97
> +    virtual void didStopAnimationTimer() { }

This is an odd part of the API, especially since the notion of animation timer is CCSingleThreadProxy specific.  Is this just a test hook?  Why not simply have a test that calls finishAllRendering and then ASSERT()s that the timer doesn't fire?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:126
> +    virtual bool initializeLayerRenderer(PassRefPtr<GraphicsContext3D>, TextureUploaderOption);

Also a weird test hook - we can make initialization fail by passing a GraphicsContext3D in that's not useful (say, make the first makeContextCurrent() call return false) - that's a much more realistic test and we do it in other places

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:133
> +        // We're done rendering and should not process any future animation events.

I'd much prefer that we cancel the timer when initialization fails.  The fact that WebViewImpl calls finishAllRendering() is a strange historical artifact of how shutdown used to work and isn't really a concern of the compositor - in fact I've been planning to delete that call.

Created attachment 145124 [details]
Patch

 (In reply to comment #14)
> (From update of attachment 145103 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145103&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:97
> > +    virtual void didStopAnimationTimer() { }
>
> This is an odd part of the API, especially since the notion of animation timer is CCSingleThreadProxy specific.  Is this just a test hook?  Why not simply have a test that calls finishAllRendering and then ASSERT()s that the timer doesn't fire?
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:126
> > +    virtual bool initializeLayerRenderer(PassRefPtr<GraphicsContext3D>, TextureUploaderOption);
>
> Also a weird test hook - we can make initialization fail by passing a GraphicsContext3D in that's not useful (say, make the first makeContextCurrent() call return false) - that's a much more realistic test and we do it in other places
Done.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:133
> > +        // We're done rendering and should not process any future animation events.
>
> I'd much prefer that we cancel the timer when initialization fails.  The fact that WebViewImpl calls finishAllRendering() is a strange historical artifact of how shutdown used to work and isn't really a concern of the compositor - in fact I've been planning to delete that call.
Done.

[James Robinson]

Comment on attachment 145124 [details]
Patch

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

Looks good

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1474
> +        m_layerTreeHost->finishAllRendering();

We don't need this call any more, do we?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1475
> +        endTestAfterDelay(CCSingleThreadProxy::animationTimerDelay() * 1000);

I think it's worth having a comment here explaining why this value is chosen (since we're setting the timer against the animation timer).  This will round down since it's truncating from a double to millis - might be safer to explicitly ceil() here so if the timer implementation does something else we can be sure we don't run before it.

Created attachment 145143 [details]
Patch

(In reply to comment #16)
> (From update of attachment 145124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145124&action=review
>
> Looks good
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1474
> > +        m_layerTreeHost->finishAllRendering();
>
> We don't need this call any more, do we?
Good point. Removed.
>
> > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1475
> > +        endTestAfterDelay(CCSingleThreadProxy::animationTimerDelay() * 1000);
>
> I think it's worth having a comment here explaining why this value is chosen (since we're setting the timer against the animation timer).  This will round down since it's truncating from a double to millis - might be safer to explicitly ceil() here so if the timer implementation does something else we can be sure we don't run before it.
Done.

[WebKit Review Bot]

Comment on attachment 145143 [details]
Patch

Clearing flags on attachment: 145143

Committed r119173: <http://trac.webkit.org/changeset/119173>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.