Bug 57717 – Convert use of raw pointers to RefPtr in using Cairo

• Component: Platform
• Status: RESOLVED FIXED

[Joone Hur]

We need to use RefPtr instead of raw pointers when using cairo_t, cairo_surface_t in WebCore/platform/graphics/cairo.

[Joone Hur]

Created attachment 88009 [details]
Proposed Patch

[WebKit Review Bot]

Attachment 88009 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8247736

[Early Warning System Bot]

Attachment 88009 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8328020

[WebKit Review Bot]

Attachment 88009 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8318751

[Joone Hur]

Created attachment 88011 [details]
Proposed Patch2

[WebKit Review Bot]

Attachment 88011 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8319696

[WebKit Review Bot]

Attachment 88011 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8307749

[Joone Hur]

Created attachment 88012 [details]
Proposed Patch3

[Eric Seidel (no email)]

Comment on attachment 88012 [details]
Proposed Patch3

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

This patch makes my heart sing!  Unfortunately I think it also will make WebCore crash (in the one case I pointed out.

> Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:82
> +    scratchBuffer = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, width, height));
> +    return scratchBuffer.get();

Um.  This isn't going to work, or?  I think you want to return a PassRefPtr here.

[Martin Robinson]

Comment on attachment 88012 [details]
Proposed Patch3

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

>> Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:82
>> +    return scratchBuffer.get();
> 
> Um.  This isn't going to work, or?  I think you want to return a PassRefPtr here.

I think it should work, since scratchBuffer is a static global. This patch should probably rename "scratchBuffer" to something like "gScratchBuffer" to make it clear that it's static though.

[Joone Hur]

(In reply to comment #10)
> (From update of attachment 88012 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88012&action=review
> 
> >> Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:82
> >> +    return scratchBuffer.get();
> > 
> > Um.  This isn't going to work, or?  I think you want to return a PassRefPtr here.
> 
> I think it should work, since scratchBuffer is a static global. This patch should probably rename "scratchBuffer" to something like "gScratchBuffer" to make it clear that it's static though.

This code works well without any crash.
http://www.w3schools.com/css3/tryit.asp?filename=trycss3_box-shadow

[Joone Hur]

Created attachment 88212 [details]
Proposed Patch4

I updated the patch with the change suggested by Martin.
Thanks, Eric and Martin for the review.

[Martin Robinson]

Comment on attachment 88212 [details]
Proposed Patch4

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

r- because of some minor style issues and a possible leak you might be able to fix!

> Source/WebCore/platform/graphics/ContextShadow.h:38
> +#if PLATFORM(CAIRO)
> +#include "RefPtrCairo.h"
> +#endif
>  #include <wtf/RefCounted.h>

This should be in separate block.

> Source/WebCore/platform/graphics/ContextShadow.h:133
> +#if PLATFORM(CAIRO)
> +    RefPtr<cairo_surface_t> m_layerImage; // Buffer to where the temporary shadow will be drawn to.
> +    RefPtr<cairo_t> m_layerContext; // Context used to paint the shadow to the layer image.
> +#else

Why not use change the typedefs above instead of adding #ifdefs to the code?

> Source/WebCore/platform/graphics/cairo/CairoPath.h:35
>          static cairo_surface_t* pathSurface = cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);
> -        m_cr = cairo_create(pathSurface);
> +        m_cr = adoptRef(cairo_create(pathSurface));

Looking at this now, isn't the surface leaked? It scould be a RefPtr here too.

[Joone Hur]

Created attachment 88561 [details]
Proposed Patch5

I applied Martin's suggestions to the patch.
Thanks!

[Martin Robinson]

Comment on attachment 88561 [details]
Proposed Patch5

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

> Source/WebCore/platform/graphics/ContextShadow.h:38
> +#if PLATFORM(CAIRO)
> +#include "RefPtrCairo.h"
> +#endif
> +

Please move this to a separate block with a newline between the first block of includes and this one.

> Source/WebCore/platform/graphics/ContextShadow.h:57
> +typedef RefPtr<cairo_surface_t> PlatformImage;

We don't need to take a reference to the m_layerImage, so maybe it makes sense to just leave this as a cairo_surface_t. What do you think?

> Source/WebCore/platform/graphics/cairo/CairoPath.h:34
> +        static RefPtr<cairo_surface_t> pathSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1));

Ack! I didn't realize this was static. Please revert this line. Sorry!

> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:95
> +    RefPtr<cairo_t> shadowContext = shadow->beginShadowLayer(graphicsContext, fontExtentsRect);
> +    if (shadowContext.get()) {
> +        prepareContextForGlyphDrawing(shadowContext.get(), font, point);
> +        drawGlyphsToContext(shadowContext.get(), font, glyphs, numGlyphs);
>          shadow->endShadowLayer(graphicsContext);

I don't think you should use RefPtr here since you don't want to take a reference.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:171
> -    cairo_t* shadowContext = shadow->beginShadowLayer(context, solidFigureExtents);
> -    if (!shadowContext)
> +    RefPtr<cairo_t> shadowContext = shadow->beginShadowLayer(context, solidFigureExtents);
> +    if (!shadowContext.get())

Ditto. I don't think it adds anything here.

> Source/WebCore/platform/graphics/cairo/ImageCairo.cpp:143
>      if (shadow->m_type != ContextShadow::NoShadow) {
> -        cairo_t* shadowContext = shadow->beginShadowLayer(context, dstRect);
> +        RefPtr<cairo_t> shadowContext = shadow->beginShadowLayer(context, dstRect);
>          if (shadowContext) {

Ditto.

> Source/WebCore/platform/graphics/gtk/FontGtk.cpp:248
> -    cairo_t* shadowContext = shadow->beginShadowLayer(graphicsContext, extents);
> +    RefPtr<cairo_t> shadowContext = shadow->beginShadowLayer(graphicsContext, extents);

Ditto.

[Joone Hur]

(In reply to comment #15)
> (From update of attachment 88561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88561&action=review
> 
> > Source/WebCore/platform/graphics/ContextShadow.h:38
> > +#if PLATFORM(CAIRO)
> > +#include "RefPtrCairo.h"
> > +#endif
> > +
> 
> Please move this to a separate block with a newline between the first block of includes and this one.
> 

I will fix it.

> > Source/WebCore/platform/graphics/ContextShadow.h:57
> > +typedef RefPtr<cairo_surface_t> PlatformImage;
> 
> We don't need to take a reference to the m_layerImage, so maybe it makes sense to just leave this as a cairo_surface_t. What do you think?
> 

I agree. m_layerImage just takes a static pointer of cairo_surface_t, so we don't need to mange the reference count.

> > Source/WebCore/platform/graphics/cairo/CairoPath.h:34
> > +        static RefPtr<cairo_surface_t> pathSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1));
> 
> Ack! I didn't realize this was static. Please revert this line. Sorry!
> 

That's ok.

> > Source/WebCore/platform/graphics/cairo/FontCairo.cpp:95
> > +    RefPtr<cairo_t> shadowContext = shadow->beginShadowLayer(graphicsContext, fontExtentsRect);
> > +    if (shadowContext.get()) {
> > +        prepareContextForGlyphDrawing(shadowContext.get(), font, point);
> > +        drawGlyphsToContext(shadowContext.get(), font, glyphs, numGlyphs);
> >          shadow->endShadowLayer(graphicsContext);
> 
> I don't think you should use RefPtr here since you don't want to take a reference. 
> 

However, beginShadowLayer returns RefPtr<cairo_t>, so I need to use RefPtr here.

[Martin Robinson]

Comment on attachment 88561 [details]
Proposed Patch5

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

>>> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:95
>>>          shadow->endShadowLayer(graphicsContext);
>> 
>> I don't think you should use RefPtr here since you don't want to take a reference.
> 
> However, beginShadowLayer returns RefPtr<cairo_t>, so I need to use RefPtr here.

Typically things only return a PassRefPtr if they are handing off ownership. It seems that beginShadowLayer shouldn't make this suggestion.

[Joone Hur]

(In reply to comment #17)
> (From update of attachment 88561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88561&action=review
> 
> >>> Source/WebCore/platform/graphics/cairo/FontCairo.cpp:95
> >>>          shadow->endShadowLayer(graphicsContext);
> >> 
> >> I don't think you should use RefPtr here since you don't want to take a reference.
> > 
> > However, beginShadowLayer returns RefPtr<cairo_t>, so I need to use RefPtr here.
> 
> Typically things only return a PassRefPtr if they are handing off ownership. It seems that beginShadowLayer shouldn't make this suggestion.

Yes, we can't use a PassRefPtr as return value here, so it seems to make sense to just leave m_layerContext as a cairo_t to return a raw pointer.

[Brent Fulgham]

Comment on attachment 88561 [details]
Proposed Patch5

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

>>> Source/WebCore/platform/graphics/cairo/CairoPath.h:34
>>> +        static RefPtr<cairo_surface_t> pathSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1));
>> 
>> Ack! I didn't realize this was static. Please revert this line. Sorry!
> 
> That's ok.

Is this static really threadsafe? Under MSVCRT (which also uses this code) there is no guarantee that this will only execute once.  Could it be changed to a static initializer so that it is known to complete prior to multi-threaded execution?

[Joone Hur]

Created attachment 89184 [details]
Proposed Patch6

Creating the pathSurface could be threadsafe.

[Hajime Morrita]

Comment on attachment 89184 [details]
Proposed Patch6

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

> Source/WebCore/platform/graphics/cairo/CairoPath.h:31
> +{

This isn't thread-safe as before.
I guess what Brent said is that you can put the variable to the file static scope,
and to make linkers/loaders to call its constructor. 

In general it's good to avoid static initializers. 
But WebKit uses it many places so you don't worry about to use it ;-)

[Joone Hur]

Created attachment 89407 [details]
Proposed Patch7

Thanks for your suggestions.

[Martin Robinson]

Comment on attachment 89407 [details]
Proposed Patch7

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

I apologize that this change has balooned from its original intent. I understand if you wish to separate out the parts of the change into separate bugs.

> Source/WebCore/platform/graphics/cairo/CairoPath.cpp:18
> +/*
> +    Copyright (C) 2011 Collabora Ltd.
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public
> +    License as published by the Free Software Foundation; either
> +    version 2 of the License, or (at your option) any later version.
> +
> +    This library is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +    Library General Public License for more details.
> +
> +    You should have received a copy of the GNU Library General Public License
> +    aint with this library; see the file COPYING.LIB.  If not, write to
> +    the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +    Boston, MA 02110-1301, USA.
> +*/

I think it's good that you are moving this stuff into an actual implementation file since it's becoming heftier. Please, please take this opportunity to rename this to PlatformPathCairo though. Having CairoPath.{cpp,h} and PathCairo.cpp is incredibly confusing!

Please format this license blocks with asterisks down the left side to match the other files in this directory.

> Source/WebCore/platform/graphics/cairo/CairoPath.cpp:25
> +cairo_surface_t* CairoPath::m_pathSurface = cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);

Now that this is in a CPP file you can dispense with the static class member altogether and just do:

static cairo_surface_t* getPathSurface()
{
    static pathSurface =  cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);
    return pathSurface;
}

> Source/WebCore/platform/graphics/cairo/CairoPath.h:35
>      CairoPath()
>      {
> -        static cairo_surface_t* pathSurface = cairo_image_surface_create(CAIRO_FORMAT_A8, 1, 1);
> -        m_cr = cairo_create(pathSurface);
> +        m_cr = adoptRef(cairo_create(m_pathSurface));
>      }

Please move the implementation of the constructor into the CPP file so you can remove the cairo.h include above. The assignment of m_cr should be an initializer.

CairoPath()
    : m_cr(adoptRef(cairo_create(m_pathSurface))
{
}

> Source/WebCore/platform/graphics/cairo/CairoPath.h:40
>      ~CairoPath()
>      {
> -        cairo_destroy(m_cr);
>      }
>  

This can be one line now and perhaps even omitted.

> Source/WebCore/platform/graphics/cairo/CairoPath.h:45
> +    static cairo_surface_t* m_pathSurface;

You can remove this. See above.

[Joone Hur]

(In reply to comment #23)
> (From update of attachment 89407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89407&action=review
> 
> I apologize that this change has balooned from its original intent. I understand if you wish to separate out the parts of the change into separate bugs.

No problem, it would be good to file a new bug to deal with the thread-safety issue.

[Joone Hur]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 89407 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=89407&action=review
> > 
> > I apologize that this change has balooned from its original intent. I understand if you wish to separate out the parts of the change into separate bugs.
> 
> No problem, it would be good to file a new bug to deal with the thread-safety issue.

https://bugs.webkit.org/show_bug.cgi?id=58514

[Joone Hur]

Created attachment 89545 [details]
Proposed Patch8

This patch is updated to only deal with replacing raw pointers with RefPtr.

[WebKit Commit Bot]

Comment on attachment 89545 [details]
Proposed Patch8

Clearing flags on attachment: 89545

Committed r83932: <http://trac.webkit.org/changeset/83932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.