Bug 68121 – [chromium] LayerRendererChromium shouldn't be RefCounted

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] LayerRendererChromium shouldn't be RefCounted

[James Robinson]

Created attachment 107397 [details]
Patch

[Adrienne Walker]

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?

[Vangelis Kokkevis]

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?

[James Robinson]

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.

[James Robinson]

(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)

[James Robinson]

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.

[James Robinson]

Created attachment 107402 [details]
remove LayerRendererChromium weak ptrs in favor of draw time parameter

[James Robinson]

Updated, PTAL.

[Adrienne Walker]

(In reply to comment #8)
> Updated, PTAL.

This looks great.  :)

[Kenneth Russell]

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.

[James Robinson]

Filed https://bugs.webkit.org/show_bug.cgi?id=68130 to track the poor name for LRC.

[James Robinson]

Committed r95135: <http://trac.webkit.org/changeset/95135>