[webkit-reviews] review granted: [Bug 95379] [chromium] Register/unregister contents layers with GraphicsLayerChromium : [Attachment 161345] with test

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has granted James Robinson
<jamesr at chromium.org>'s request for review:
Bug 95379: [chromium] Register/unregister contents layers with
GraphicsLayerChromium
https://bugs.webkit.org/show_bug.cgi?id=95379

Attachment 161345: with test
https://bugs.webkit.org/attachment.cgi?id=161345&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
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?