Bug 130320

Created attachment 226890 [details] Patch idea

Comment on attachment 226890 [details] Patch idea View in context: https://bugs.webkit.org/attachment.cgi?id=226890&action=review Looks good. > Source/WTF/wtf/WTFThreadData.cpp:77 > +#if USE(PTHREAD_GETSPECIFIC_DIRECT) > +static void destroyWTFThreadData(void* data) Should leave another blank line here to match the one we left below before #endif. Also could consider using a lambda instead of declaring a separate function. Not sure what is preferred style. > Source/WTF/wtf/WTFThreadData.h:47 > +#ifdef __has_include > +#if __has_include(<System/pthread_machdep.h>) > + > +#include <System/pthread_machdep.h> > + > +#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0) > +#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1 > +#endif > + > +#endif > +#endif I don’t like nested #if, so I’d write it like this: #if defined(__has_include) && __has_include(<System/pthread_machdep.h>) #include <System/pthread_machdep.h> #endif #if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1) #define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1 #endif I think we should make the same improvement in FastMalloc.cpp; just easier to read without the nesting. But also, I am not sure we should use the same USE(PTHREAD_GETSPECIFIC_DIRECT) both here and there; is there a risk of conflict if we compile all-in-one or something? The reason I suggest checking for __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1 instead of __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0 is that is the key we are actually using here. Which I suppose doesn’t make sense if this is a single USE for both places. > Source/WTF/wtf/WTFThreadData.h:159 > +#if USE(PTHREAD_GETSPECIFIC_DIRECT) Might consider putting the portable side of the #if first rather than the platform-specific one. > Source/WTF/wtf/WTFThreadData.h:177 > // WRT WebCore: I think this comment applies to both sides of the #if, so it should be outside the #if.

Created attachment 226893 [details] Patch for landing Tweaked per Darin's recipe.

Created attachment 226894 [details] Patch for landing

Comment on attachment 226894 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=226894&action=review > Source/WTF/wtf/WTFThreadData.h:160 > +#if USE(PTHREAD_GETSPECIFIC_DIRECT) > + static const pthread_key_t directKey = __PTK_FRAMEWORK_JAVASCRIPTCORE_KEY1; > + WTF_EXPORT_PRIVATE static WTFThreadData& createAndRegisterForGetspecificDirect(); > +#else > static WTF_EXPORTDATA ThreadSpecific<WTFThreadData>* staticData; > +#endif Portable first could apply here too.

Comment on attachment 226894 [details] Patch for landing Clearing flags on attachment: 226894 Committed r165725: <http://trac.webkit.org/changeset/165725>

All reviewed patches have been landed. Closing bug.

(In reply to comment #6) > (From update of attachment 226894 [details]) > Clearing flags on attachment: 226894 > > Committed r165725: <http://trac.webkit.org/changeset/165725> And buildfixes landed in - http://trac.webkit.org/changeset/165726 - http://trac.webkit.org/changeset/165729 It would have been better to let the EWS run before landing the patch.

(In reply to comment #2) > > Source/WTF/wtf/WTFThreadData.h:47 > > +#ifdef __has_include > > +#if __has_include(<System/pthread_machdep.h>) > > + > > +#include <System/pthread_machdep.h> > > + > > +#if defined(__PTK_FRAMEWORK_JAVASCRIPTCORE_KEY0) > > +#define WTF_USE_PTHREAD_GETSPECIFIC_DIRECT 1 > > +#endif > > + > > +#endif > > +#endif > > I don’t like nested #if, so I’d write it like this: > > #if defined(__has_include) && __has_include(<System/pthread_machdep.h>) > #include <System/pthread_machdep.h> > #endif The idea was reasonable not to have nested ifs. But unfortunately it made GCC unhappy: /mnt/buildbot/efl-linux-slave-3/efl-linux-32-release/build/Source/WTF/wtf/WTFThreadData.h:37:44: error: missing binary operator before token "(" It seems short circuit evaluation doesn't work in GCC preprocessor and its parser gets confused by the undefined __has_include() macro. In this case only the nested #if works: #ifdef __has_include #if __has_include(...) #include ... #endif #endif

Tests started to crash with assertion failures around the time this was landed: http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r165732%20(3394)/svg/dom/transform-parser-crash-log.txt http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r165733%20(3842)/svg/W3C-SVG-1.1/shapes-polyline-01-t-crash-log.txt Can this change be the culprit? Should we temporarily roll out to see if that fixes the crashes?

Andreas found an instance of this crash happening prior to this change, and suspects bug 130317.