Bug 108803 – Web Inspector: Native Memory Instrumentation: fix the instrumentation of GraphicsLayers

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

GraphicsLayer class was instrumented earlier but it doesn't appear in the snapshot because there were no links to it.
Also it could report the expected size of the image buffer, so we can report it.

[Ilya Tikhonovsky]

Created attachment 186319 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 186319 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2902
> -    info.addWeakPointer(m_renderView);
> +    info.addMember(m_renderView, "renderView");

It's not clear to me that this is correct. RenderView is a "back pointer"; it's not owned by RLC.

[Ilya Tikhonovsky]

(In reply to comment #2)
> (From update of attachment 186319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186319&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2902
> > -    info.addWeakPointer(m_renderView);
> > +    info.addMember(m_renderView, "renderView");
> 
> It's not clear to me that this is correct. RenderView is a "back pointer"; it's not owned by RLC.

The instrumentation code detects the ownership attribute automatically by addObjectImpl method. The method has explicit overloads for RefPtr and OwnPtr.
https://code.google.com/p/chromium/source/search?q=addObjectImpl&origq=addObjectImpl&btnG=Search+Trunk

So the edges for the raw pointers will get 'weak' attribute. The old idea with addWeakPointer doesn't work properly with our template based containers. I'm going to remove this method from the instrumentation code.

It works fine in almost all the cases except those where objects are owned via raw pointers.

I've found and fixed two such cases:
http://trac.webkit.org/changeset/136288
http://trac.webkit.org/changeset/135605

Unfortunately we have the same problem with the pointers to the render objects which couldn't be solved that way. But I think we can support a 'delete strategy' template parameter with a default implementation in RefPtr and OwnPtr templates and create the custom 'delete strategy' for the pointers to the render objects. But I'd like to discuss this idea in webkit-dev at the beginning.

[Ilya Tikhonovsky]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 186319 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186319&action=review
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:2902
> > > -    info.addWeakPointer(m_renderView);
> > > +    info.addMember(m_renderView, "renderView");
> > 
> > It's not clear to me that this is correct. RenderView is a "back pointer"; it's not owned by RLC.
> 
> The instrumentation code detects the ownership attribute automatically by addObjectImpl method. The method has explicit overloads for RefPtr and OwnPtr.
> https://code.google.com/p/chromium/source/search?q=addObjectImpl&origq=addObjectImpl&btnG=Search+Trunk
> 
> So the edges for the raw pointers will get 'weak' attribute. The old idea with addWeakPointer doesn't work properly with our template based containers. I'm going to remove this method from the instrumentation code.
> 
> It works fine in almost all the cases except those where objects are owned via raw pointers.

I mean the current schema works fine .......

> 
> I've found and fixed two such cases:
> http://trac.webkit.org/changeset/136288
> http://trac.webkit.org/changeset/135605
> 
> Unfortunately we have the same problem with the pointers to the render objects which couldn't be solved that way. But I think we can support a 'delete strategy' template parameter with a default implementation in RefPtr and OwnPtr templates and create the custom 'delete strategy' for the pointers to the render objects. But I'd like to discuss this idea in webkit-dev at the beginning.

[Pavel Feldman]

Comment on attachment 186319 [details]
Patch

Clearing r? - it has been there for ages.