Bug 68121 - [chromium] LayerRendererChromium shouldn't be RefCounted
Summary: [chromium] LayerRendererChromium shouldn't be RefCounted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks: 66995
  Show dependency treegraph
 
Reported: 2011-09-14 14:47 PDT by James Robinson
Modified: 2011-09-14 16:00 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.75 KB, patch)
2011-09-14 14:50 PDT, James Robinson
no flags Details | Formatted Diff | Diff
remove LayerRendererChromium weak ptrs in favor of draw time parameter (40.54 KB, patch)
2011-09-14 15:20 PDT, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-09-14 14:47:43 PDT
[chromium] LayerRendererChromium shouldn't be RefCounted
Comment 1 James Robinson 2011-09-14 14:50:18 PDT
Created attachment 107397 [details]
Patch
Comment 2 Adrienne Walker 2011-09-14 14:58:30 PDT
Comment on attachment 107397 [details]
Patch

I think this is a net positive change.  I totally agree that OwnPtr<> reduces the chance of reference cycles, but I worry that making them raw pointers instead of RefPtr<>s increases the chance that a layer is holding onto a stale layer renderer pointer.

Maybe this is a naive suggestion, but do you think it would be an improvement to pass a LayerRendererChromium* into the CCLayerImpl::draw() function to avoid having to store it on the layers themselves and thereby avoiding any chance that it's stale?
Comment 3 Vangelis Kokkevis 2011-09-14 14:58:51 PDT
Comment on attachment 107397 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:253
> +    LayerRendererChromium* m_layerRenderer;

Does the CCLayerImpl still need this pointer?
Comment 4 James Robinson 2011-09-14 15:00:45 PDT
The most interesting lifecycle bit here is on lost context, which triggers a LayerRendererChromium recreation.  Here's what happens then (in the CCSingleThreadProxy case which we use today, will be CCThreadProxy in the future):

Initial state: LayerRendererChromium A, tree of CCLayerImpls that have (weak) pointers to A

CCSingleThreadProxy detects a lost context and calls CCSingleThreadProxy::recreateContextIfNeeded()
CCSingleThreadProxy::recreateContextIfNeeded() calls CCLayerTreeHostImpl::initializeLayerRenderer()
  CCLayerTreeHostImpl::initializeLayerRenderer() constructs a new LayerRendererChromium (B) with an initially empty CCLayerImpl tree
  CCLayerTreeHostImpl::initializeLayerRenderer() calls LayerRendererChromium::close() on A, which drops all rendersurface refs to CCLayerImpls
  CCLayerTreeHostImpl::initializeLayerRenderer() deletes the old LayerRendererChromium, which drops the last reference to its CCLayerImpl tree and destroys all CCLayerImpls
(CCSingleThreadProxy::recreateContextIfNeeded() returns)
CCSingleThreadProxy::compositeImmediately() calls commitIfTo, which calls the TreeSynchronizer, which constructs a new CCLayerImpl tree under LayerRendererChromium B
LayerRendererChromium::drawLayers on B calls setLayerRendererRecursive() on all CCLayerImpls in the new tree and sets the LayerRendererChromium weak pointers


Technically speaking we don't need LayerRendererChromium weak pointers on CCLayerImpls at all.
Comment 5 James Robinson 2011-09-14 15:01:02 PDT
(In reply to comment #3)
> (From update of attachment 107397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107397&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:253
> > +    LayerRendererChromium* m_layerRenderer;
> 
> Does the CCLayerImpl still need this pointer?

It doesn't.  I can try to remove this as part of this patch if you'd like (it seems useful)
Comment 6 James Robinson 2011-09-14 15:01:42 PDT
Looks like all 3 of us posted at the same time that CCLayerImpl weak pointer should go away :P.  I'll kill it too and update this patch - hopefully that doesn't grow the thing too much.
Comment 7 James Robinson 2011-09-14 15:20:46 PDT
Created attachment 107402 [details]
remove LayerRendererChromium weak ptrs in favor of draw time parameter
Comment 8 James Robinson 2011-09-14 15:29:58 PDT
Updated, PTAL.
Comment 9 Adrienne Walker 2011-09-14 15:40:49 PDT
(In reply to comment #8)
> Updated, PTAL.

This looks great.  :)
Comment 10 Kenneth Russell 2011-09-14 15:51:02 PDT
Comment on attachment 107402 [details]
remove LayerRendererChromium weak ptrs in favor of draw time parameter

I was concerned that the dependency from the cc/ classes to LayerRendererChromium was a conceptual layering violation, but jamesr assures me that it's just that the LRC needs to be renamed to something like CCGLRendererImpl. This looks fine to me in that case.
Comment 11 James Robinson 2011-09-14 15:55:08 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=68130 to track the poor name for LRC.
Comment 12 James Robinson 2011-09-14 16:00:41 PDT
Committed r95135: <http://trac.webkit.org/changeset/95135>