Bug 66981 – [Chromium] Possible leak of LayerRendererChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Satish Sampath]

Opening and closing multiple tabs in the same process with hardware acceleration enabled seems to be causing an unbounded increase in the process's memory usage. It looks like LayerRendererChromium objects are staying alive forever after the page is closed, possibly due to a circular reference.

Steps to reproduce:
1. Run chrome with --force-compositing-mode argument
2. open chrome:memory
3. open build.webkit.org in 2 tabs, check that they appear under the same process in chrome:memory
4. close one of them, then open the same site again
5. repeat (4) multiple times

Refresh chrome:memory and the GPU process should show a much higher memory usage than in (3)

Also tried by forcing a GC and saw no drop in memory used.

[James Robinson]

Yeah we definitely have developed a few refcount cycles :(

[James Robinson]

Created attachment 105254 [details]
Patch

[James Robinson]

With this patch the LayerRendererChromium gets freed, but I'm pretty sure I'm still leaking every non-root CCLayerImpl.  Will try to figure out how to confirm or deny that.

This impacts m14.

[James Robinson]

~LayerRendererChromium() has actually been crashy for a while, since it doesn't explicitly clear out all of its programs before dropping it reference to the GraphicsContext3D, and our ProgramBindings only have weak references to their GraphicsContext3D and not RefPtr<>s.  It's a good thing that destructor has been dead code.

[James Robinson]

Created attachment 105261 [details]
Patch

[Nat Duca]

Looks sane to me. We should definitely eradicate refptrs from the compositor, entirely.

[Adrienne Walker]

Comment on attachment 105261 [details]
Patch

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

Also, we should probably fix the destruction of ProgramBinding to not depend on a naked pointer, but that can happen in another patch.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:184
>  void CCLayerTreeHost::setRootLayer(GraphicsLayer* layer)
>      if (layer) {
>          m_nonCompositedContentHost->graphicsLayer()->addChild(layer);
>          layer->platformLayer()->setLayerRenderer(m_layerRenderer.get());
> -    }
> +    } else
> +        layerRenderer()->clearRootCCLayerImpl();

Are we guaranteed to get a setRootLayer(0) before ~CCLayerTreeHost? It seems like it would be safer to call clearCCRootLayer in the CCLayerTreeHost destructor, because the CCLayerTreeHost (for now) is the LayerRendererChromium owner anyway.  When that changes, we can move that call to the CCLayerTreeHostImpl.

[James Robinson]

(In reply to comment #7)
> (From update of attachment 105261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105261&action=review
> 
> Also, we should probably fix the destruction of ProgramBinding to not depend on a naked pointer, but that can happen in another patch.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:184
> >  void CCLayerTreeHost::setRootLayer(GraphicsLayer* layer)
> >      if (layer) {
> >          m_nonCompositedContentHost->graphicsLayer()->addChild(layer);
> >          layer->platformLayer()->setLayerRenderer(m_layerRenderer.get());
> > -    }
> > +    } else
> > +        layerRenderer()->clearRootCCLayerImpl();
> 
> Are we guaranteed to get a setRootLayer(0) before ~CCLayerTreeHost?

We are, by way of http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp&q=detachRootLAyer&exact_package=chromium&l=1794

> It seems like it would be safer to call clearCCRootLayer in the CCLayerTreeHost destructor, because the CCLayerTreeHost (for now) is the LayerRendererChromium owner anyway.  When that changes, we can move that call to the CCLayerTreeHostImpl.

CCLayerTreeHost is RefPtr<> by WebViewImpl, which Nat points out is also wrong (it should be OwnPtr<>).  I'm a little resistent to put it in ~CCLayerTreeHost because I think we'll still leak when switching from compositing to non-composited mode but not killing the WebViewImpl (I'll check that now).

[James Robinson]

(In reply to comment #8)
> > It seems like it would be safer to call clearCCRootLayer in the CCLayerTreeHost destructor, because the CCLayerTreeHost (for now) is the LayerRendererChromium owner anyway.  When that changes, we can move that call to the CCLayerTreeHostImpl.
> 
> CCLayerTreeHost is RefPtr<> by WebViewImpl, which Nat points out is also wrong (it should be OwnPtr<>).  I'm a little resistent to put it in ~CCLayerTreeHost because I think we'll still leak when switching from compositing to non-composited mode but not killing the WebViewImpl (I'll check that now).

Yeah, if I wait for ~CCLayerTreeHost() then the layers and all their textures and everything stick around until the WebViewImpl is destroyed.  Not so hot.

[Nat Duca]

Shoudl we add a LayerTreeHost::Destroy? And call it in the right places on WebView?

[Adrienne Walker]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 105261 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=105261&action=review
> > 
> > Also, we should probably fix the destruction of ProgramBinding to not depend on a naked pointer, but that can happen in another patch.
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:184
> > >  void CCLayerTreeHost::setRootLayer(GraphicsLayer* layer)
> > >      if (layer) {
> > >          m_nonCompositedContentHost->graphicsLayer()->addChild(layer);
> > >          layer->platformLayer()->setLayerRenderer(m_layerRenderer.get());
> > > -    }
> > > +    } else
> > > +        layerRenderer()->clearRootCCLayerImpl();
> > 
> > Are we guaranteed to get a setRootLayer(0) before ~CCLayerTreeHost?
> 
> We are, by way of http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp&q=detachRootLAyer&exact_package=chromium&l=1794

Ok, so we're not intrinsically safe, just safe by the conventions of the caller.

> > It seems like it would be safer to call clearCCRootLayer in the CCLayerTreeHost destructor, because the CCLayerTreeHost (for now) is the LayerRendererChromium owner anyway.  When that changes, we can move that call to the CCLayerTreeHostImpl.
> 
> CCLayerTreeHost is RefPtr<> by WebViewImpl, which Nat points out is also wrong (it should be OwnPtr<>).  I'm a little resistent to put it in ~CCLayerTreeHost because I think we'll still leak when switching from compositing to non-composited mode but not killing the WebViewImpl (I'll check that now).

That's totally valid.  We'd keep around the LRC once switching back to non-composited mode but before the WebViewImpl was destroyed.

This call will move to the CCLayerTreeHostImpl destructor soon enough when the ownership of LayerRendererChromium changes, so this is fine as-is.

[James Robinson]

(In reply to comment #11)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 105261 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=105261&action=review
> > > 
> > > Also, we should probably fix the destruction of ProgramBinding to not depend on a naked pointer, but that can happen in another patch.
> > > 
> > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:184
> > > >  void CCLayerTreeHost::setRootLayer(GraphicsLayer* layer)
> > > >      if (layer) {
> > > >          m_nonCompositedContentHost->graphicsLayer()->addChild(layer);
> > > >          layer->platformLayer()->setLayerRenderer(m_layerRenderer.get());
> > > > -    }
> > > > +    } else
> > > > +        layerRenderer()->clearRootCCLayerImpl();
> > > 
> > > Are we guaranteed to get a setRootLayer(0) before ~CCLayerTreeHost?
> > 
> > We are, by way of http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayerCompositor.cpp&q=detachRootLAyer&exact_package=chromium&l=1794
> 
> Ok, so we're not intrinsically safe, just safe by the conventions of the caller.

Yes.  The detach behavior is also required by the CoreAnimation backend so I don't feel too bad depending on it.

> 
> > > It seems like it would be safer to call clearCCRootLayer in the CCLayerTreeHost destructor, because the CCLayerTreeHost (for now) is the LayerRendererChromium owner anyway.  When that changes, we can move that call to the CCLayerTreeHostImpl.
> > 
> > CCLayerTreeHost is RefPtr<> by WebViewImpl, which Nat points out is also wrong (it should be OwnPtr<>).  I'm a little resistent to put it in ~CCLayerTreeHost because I think we'll still leak when switching from compositing to non-composited mode but not killing the WebViewImpl (I'll check that now).
> 
> That's totally valid.  We'd keep around the LRC once switching back to non-composited mode but before the WebViewImpl was destroyed.
> 
> This call will move to the CCLayerTreeHostImpl destructor soon enough when the ownership of LayerRendererChromium changes, so this is fine as-is.

Right - I hope :)

[James Robinson]

(In reply to comment #10)
> Shoudl we add a LayerTreeHost::Destroy? And call it in the right places on WebView?

I don't want to destroy the LTH or the LayerRendererChromium any sooner than they are now, I think it's perfectly fine to hold the programs, etc, alive when compositing is disabled until the WebViewImpl goes away since it's pretty common for a page to flip in and out of compositing mode.  What I want is to make sure that all of the layers (and their associated textures, buffers, etc) go away as soon as compositing is disabled, because even if compositing is later re-enabled we have to reupload everything anyway because we have no way of knowing what is still valid.  So I think the destruction path for LayerTreeHost is fine right now as far as WebViewImpl goes, it's just the teardown for the layers inside of it (which happens to hold LayerRendererChromium alive as well in the current design, but leaking just the layers is still really bad).

[Adrienne Walker]

This patch unofficially looks good to me.  I've verified that if I open poster circle, duplicate tabs, then close those tabs, the GPU process memory usage shrinks back close to where it started.  Without this patch, the memory doesn't shrink at all.

(It looks like the GPU process memory still grows by about ~1MB per poster circle tab duplicate/close, which probably deserves more investigation.  However, this patch is still a huge win.)

Can I get an official review?

[Kenneth Russell]

Comment on attachment 105261 [details]
Patch

Looks good to me. r=me

[WebKit Review Bot]

Comment on attachment 105261 [details]
Patch

Clearing flags on attachment: 105261

Committed r93927: <http://trac.webkit.org/changeset/93927>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.