Bug 57717

Summary: Convert use of raw pointers to RefPtr in using Cairo
Product: WebKit Reporter: Joone Hur <joone>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, dglazkov, joone.hur, morrita, mrobinson, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Proposed Patch
none
Proposed Patch2
none
Proposed Patch3
eric: review-
Proposed Patch4
mrobinson: review-
Proposed Patch5
none
Proposed Patch6
none
Proposed Patch7
mrobinson: review-
Proposed Patch8 none

Description Joone Hur 2011-04-02 21:51:36 PDT
We need to use RefPtr instead of raw pointers when using cairo_t, cairo_surface_t in WebCore/platform/graphics/cairo.
Comment 1 Joone Hur 2011-04-03 07:03:21 PDT
Created attachment 88009 [details]
Proposed Patch
Comment 2 WebKit Review Bot 2011-04-03 07:08:25 PDT
Attachment 88009 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8247736
Comment 3 Early Warning System Bot 2011-04-03 07:12:48 PDT
Attachment 88009 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8328020
Comment 4 WebKit Review Bot 2011-04-03 07:59:03 PDT
Attachment 88009 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8318751
Comment 5 Joone Hur 2011-04-03 09:20:41 PDT
Created attachment 88011 [details]
Proposed Patch2
Comment 6 WebKit Review Bot 2011-04-03 09:23:42 PDT
Attachment 88011 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8319696
Comment 7 WebKit Review Bot 2011-04-03 10:16:11 PDT
Attachment 88011 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8307749
Comment 8 Joone Hur 2011-04-03 11:00:27 PDT
Created attachment 88012 [details]
Proposed Patch3
Comment 9 Eric Seidel (no email) 2011-04-03 22:59:10 PDT
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.
Comment 10 Martin Robinson 2011-04-04 07:04:59 PDT
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.
Comment 11 Joone Hur 2011-04-04 08:55:03 PDT
(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
Comment 12 Joone Hur 2011-04-05 06:00:30 PDT
Created attachment 88212 [details]
Proposed Patch4

I updated the patch with the change suggested by Martin.
Thanks, Eric and Martin for the review.
Comment 13 Martin Robinson 2011-04-05 08:34:23 PDT
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.
Comment 14 Joone Hur 2011-04-06 20:16:04 PDT
Created attachment 88561 [details]
Proposed Patch5

I applied Martin's suggestions to the patch.
Thanks!
Comment 15 Martin Robinson 2011-04-07 07:56:49 PDT
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.
Comment 16 Joone Hur 2011-04-08 10:25:59 PDT
(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.
Comment 17 Martin Robinson 2011-04-08 10:30:17 PDT
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.
Comment 18 Joone Hur 2011-04-08 10:55:37 PDT
(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.
Comment 19 Brent Fulgham 2011-04-11 13:07:01 PDT
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?
Comment 20 Joone Hur 2011-04-12 03:34:02 PDT
Created attachment 89184 [details]
Proposed Patch6

Creating the pathSurface could be threadsafe.
Comment 21 Hajime Morrita 2011-04-12 13:35:17 PDT
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 ;-)
Comment 22 Joone Hur 2011-04-13 11:12:25 PDT
Created attachment 89407 [details]
Proposed Patch7

Thanks for your suggestions.
Comment 23 Martin Robinson 2011-04-13 11:58:03 PDT
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.
Comment 24 Joone Hur 2011-04-13 23:42:14 PDT
(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.
Comment 25 Joone Hur 2011-04-13 23:43:04 PDT
(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
Comment 26 Joone Hur 2011-04-14 00:38:57 PDT
Created attachment 89545 [details]
Proposed Patch8

This patch is updated to only deal with replacing raw pointers with RefPtr.
Comment 27 WebKit Commit Bot 2011-04-14 19:12:43 PDT
Comment on attachment 89545 [details]
Proposed Patch8

Clearing flags on attachment: 89545

Committed r83932: <http://trac.webkit.org/changeset/83932>
Comment 28 WebKit Commit Bot 2011-04-14 19:12:50 PDT
All reviewed patches have been landed.  Closing bug.