Bug 95379 – [chromium] Register/unregister contents layers with GraphicsLayerChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Register/unregister contents layers with GraphicsLayerChromium

[James Robinson]

Created attachment 161319 [details]
Patch

[James Robinson]

Enne's the most likely reviewer for this change, but they're out on vacation for a few days and this is a fairly frequent crasher on dev so could one of you fine folks take a look?  See the ChangeLog and http://code.google.com/p/chromium/issues/detail?id=144951 for context.  I'm still working on producing a good, reliable layout test to hit this case.  This is (sadly) yet another WeakPtr implementation, but this is WebKit where we spend all our time reimplementing WeakPtr.  I'm using the fact that WebLayers have unique, non-repeating IDs to do object identity checks to avoid doing a fully general WeakPtr<T> implementation.

[Adam Barth]

There's no way that I can check whether this patch is correct.  If no one feels like they know enough to review it, maybe we should land it TBR=enne?

[Dirk Pranke]

I'm way out of my depth on this patch, so I'm punting as well ... sorry!

[James Robinson]

Other approaches:

1.) reorder the RenderLayerBacking calls such that we always get a setContentsTo..() call before any calls that might want to mutate our contentsLayer.

2.) buffer state on GraphicsLayerChromium instead of pushing straight down to m_contentsLayer and only set state when we know we're in a "steady state"

(1) Seems good generally but tricky to do and tricky to maintain since nobody else would be depending on this particular behavior

(2) Requires inventing a new buffering+sync concept to GraphicsLayerChromium and defining a proper steady state timing. This is plausible but also a bit tricky since it means you have to be really careful about when you can touch certain bits of state and when things are out-of-sync.  We'd also have to keep redundant copies around of various bits of data.

[James Robinson]

Other approaches:

1.) reorder the RenderLayerBacking calls such that we always get a setContentsTo..() call before any calls that might want to mutate our contentsLayer.

2.) buffer state on GraphicsLayerChromium instead of pushing straight down to m_contentsLayer and only set state when we know we're in a "steady state"

(1) Seems good generally but tricky to do and tricky to maintain since nobody else would be depending on this particular behavior

(2) Requires inventing a new buffering+sync concept to GraphicsLayerChromium and defining a proper steady state timing. This is plausible but also a bit tricky since it means you have to be really careful about when you can touch certain bits of state and when things are out-of-sync.  We'd also have to keep redundant copies around of various bits of data.

[Nat Duca]

Would a "wilLDie" on the layer delegate be helpful here? Iirw, glc sets a delegate on the content ...

[James Robinson]

Created attachment 161345 [details]
with test

[Nat Duca]

Talked offline with James. I'm happy with this approach, all things considered. The key thing is exploding if a person-who-creates-a-new layer forgets to register, which is there.

[James Robinson]

For the record, comment #7's suggestion is valid.  The question basically boils down to whether it's better to implement weakptr-type semantics as part of the WebLayer interface - either through a destruction listener or some other mechanism - or keep it all contained in WebCore.  This patch does the latter, and hopefully the ASSERT()s are enough to keep folks in line.

[Adrienne Walker]

Comment on attachment 161345 [details]
with test

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

This is causing crashes, so I'm going to R+ it since the solution looks good to me in general.  I have a few comments, that you're free to address or not.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:295
> +    clearContentsLayerIfUnregistered();
> +    if (m_contentsLayer)

Can you wrap this common access pattern in some sort of contentsLayerSafe() function or somesuch?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:513
> +    ASSERT(s_registeredLayerSet);
> +    ASSERT(s_registeredLayerSet->contains(layer->id()));

I think the asserts in register and unregister should be CRASHes instead.  If any of these happen, you'll potentially have bogus content layer pointers lying around that point to deleted memory and that feels like a security issue.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:118
> -    virtual void setContentsToMedia(PlatformLayer*);
> -    virtual void setContentsToCanvas(PlatformLayer*);
> +    virtual void setContentsToMedia(WebKit::WebLayer*);
> +    virtual void setContentsToCanvas(WebKit::WebLayer*);

Why?

[James Robinson]

(In reply to comment #11)
> (From update of attachment 161345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161345&action=review
> 
> This is causing crashes, so I'm going to R+ it since the solution looks good to me in general.  I have a few comments, that you're free to address or not.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:295
> > +    clearContentsLayerIfUnregistered();
> > +    if (m_contentsLayer)
> 
> Can you wrap this common access pattern in some sort of contentsLayerSafe() function or somesuch?

Yup, I think so. Good idea.

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:513
> > +    ASSERT(s_registeredLayerSet);
> > +    ASSERT(s_registeredLayerSet->contains(layer->id()));
> 
> I think the asserts in register and unregister should be CRASHes instead.  If any of these happen, you'll potentially have bogus content layer pointers lying around that point to deleted memory and that feels like a security issue.

Sounds good.

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:118
> > -    virtual void setContentsToMedia(PlatformLayer*);
> > -    virtual void setContentsToCanvas(PlatformLayer*);
> > +    virtual void setContentsToMedia(WebKit::WebLayer*);
> > +    virtual void setContentsToCanvas(WebKit::WebLayer*);
> 
> Why?

No reason - I'll change them back.

[James Robinson]

Committed r127077: <http://trac.webkit.org/changeset/127077>