Bug 66981 - [Chromium] Possible leak of LayerRendererChromium
Summary: [Chromium] Possible leak of LayerRendererChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-25 14:29 PDT by Satish Sampath
Modified: 2011-08-26 17:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.18 KB, patch)
2011-08-25 15:00 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2011-08-25 15:28 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Satish Sampath 2011-08-25 14:29:12 PDT
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.
Comment 1 James Robinson 2011-08-25 14:29:49 PDT
Yeah we definitely have developed a few refcount cycles :(
Comment 2 James Robinson 2011-08-25 15:00:01 PDT
Created attachment 105254 [details]
Patch
Comment 3 James Robinson 2011-08-25 15:00:33 PDT
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.
Comment 4 James Robinson 2011-08-25 15:21:39 PDT
~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.
Comment 5 James Robinson 2011-08-25 15:28:07 PDT
Created attachment 105261 [details]
Patch
Comment 6 Nat Duca 2011-08-25 16:03:22 PDT
Looks sane to me. We should definitely eradicate refptrs from the compositor, entirely.
Comment 7 Adrienne Walker 2011-08-25 16:10:57 PDT
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.
Comment 8 James Robinson 2011-08-25 16:15:14 PDT
(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).
Comment 9 James Robinson 2011-08-25 16:20:12 PDT
(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.
Comment 10 Nat Duca 2011-08-25 16:23:19 PDT
Shoudl we add a LayerTreeHost::Destroy? And call it in the right places on WebView?
Comment 11 Adrienne Walker 2011-08-25 16:24:25 PDT
(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.
Comment 12 James Robinson 2011-08-25 18:06:02 PDT
(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 :)
Comment 13 James Robinson 2011-08-25 18:15:59 PDT
(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).
Comment 14 Adrienne Walker 2011-08-26 11:05:47 PDT
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?
Comment 15 Kenneth Russell 2011-08-26 16:20:54 PDT
Comment on attachment 105261 [details]
Patch

Looks good to me. r=me
Comment 16 WebKit Review Bot 2011-08-26 17:39:03 PDT
Comment on attachment 105261 [details]
Patch

Clearing flags on attachment: 105261

Committed r93927: <http://trac.webkit.org/changeset/93927>
Comment 17 WebKit Review Bot 2011-08-26 17:39:08 PDT
All reviewed patches have been landed.  Closing bug.