We need to use RefPtr instead of raw pointers when using cairo_t, cairo_surface_t in WebCore/platform/graphics/cairo.
Created attachment 88009 [details] Proposed Patch
Attachment 88009 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8247736
Attachment 88009 [details] did not build on qt: Build output: http://queues.webkit.org/results/8328020
Attachment 88009 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8318751
Created attachment 88011 [details] Proposed Patch2
Attachment 88011 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8319696
Attachment 88011 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8307749
Created attachment 88012 [details] Proposed Patch3
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 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.
(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
Created attachment 88212 [details] Proposed Patch4 I updated the patch with the change suggested by Martin. Thanks, Eric and Martin for the review.
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.
Created attachment 88561 [details] Proposed Patch5 I applied Martin's suggestions to the patch. Thanks!
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.
(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 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.
(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 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?
Created attachment 89184 [details] Proposed Patch6 Creating the pathSurface could be threadsafe.
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 ;-)
Created attachment 89407 [details] Proposed Patch7 Thanks for your suggestions.
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.
(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.
(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
Created attachment 89545 [details] Proposed Patch8 This patch is updated to only deal with replacing raw pointers with RefPtr.
Comment on attachment 89545 [details] Proposed Patch8 Clearing flags on attachment: 89545 Committed r83932: <http://trac.webkit.org/changeset/83932>
All reviewed patches have been landed. Closing bug.