Bug 26997 – [Chromium] Remove buggy usage of DEFINE_STATIC_LOCAL in V8Binding.cpp

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

....this usage of DEFINE_STATIC_LOCAL is causing a bunch of layout test failures for Chromium.

[Nate Chapin]

Created attachment 32313 [details]
patch

[Dimitri Glazkov (Google)]

Comment on attachment 32313 [details]
patch

ok.

[Eric Seidel (no email)]

Why is this bad usage?

[Eric Seidel (no email)]

CCing Darin who knows more about this macro than I do.

[Nate Chapin]

To be honest, I'm not 100% sure what I did wrong here.  All I know is that I clearly didn't initialize it correctly, because the test shell was consistently crashing when lowNumbers[foo] was used a couple lines below the DEFINE_STATIC_LOCAL().

I probably should have included a FIXME to use DEFINE_STATIC_LOCAL properly instead of the traditional static variable declaration.  I just wanted to get it working so that we could finish upstreaming V8Binding and get the chromium canary builder green again.  

However, it's looking like I'm going to be rolling this change and the one that introduced the bug back due to the chromium build mess and the fact that I won't be able to clean up after myself tonight.  If someone can help me figure out how to correctly use DEFINE_STATIC_LOCAL in this situation, I'll happily do it right when I re-upstream V8Binding.cpp tomorrow.

[Eric Seidel (no email)]

From StdLibExtras.h:

// Use these to declare and define a static local variable (static T;) so that
//  it is leaked so that its destructors are not called at exit. Using this
//  macro also allows workarounds a compiler bug present in Apple's version of GCC 4.0.1.
#if COMPILER(GCC) && defined(__APPLE_CC__) && __GNUC__ == 4 && __GNUC_MINOR__ == 0 && __GNUC_PATCHLEVEL__ == 1
#define DEFINE_STATIC_LOCAL(type, name, arguments) \
    static type* name##Ptr = new type arguments; \
    type& name = *name##Ptr
#else
#define DEFINE_STATIC_LOCAL(type, name, arguments) \
    static type& name = *new type arguments
#endif

These exist only to work around bugs in GCC 4.0.1 on Macs.  I don't know enough about the bug to comment further however.

[Dimitri Glazkov (Google)]

Has this landed?

[Nate Chapin]

This has landed, but I left it open since I wasn't sure whether the conversation was going to continue regarding my original usage of DEFINE_STATIC_LOCAL.  The current version of the file in the repository does not use DEFINE_STATIC_LOCAL.  If it should, I'm happy to take care of it, but I guess I'll close this issue out in the meantime.