Bug 21810 – Remove use of static C++ objects that are destroyed at exit time (destructors)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Greg Bolsinga]

Remove use of static C++ objects in general, but for an example from Timer.cpp:

static Vector<TimerBase*>* timerHeap;
static HashSet<const TimerBase*>* timersReadyToFire;

In a multi-threaded WebCore environment, a thread may be using these while the main thread is deleting them causing crashes.

Objects like this are also held in memory until exit is called, when the C++ runtime cleans these up. So the destructors only run at exit and don't really provide any benefit. Without them, the program would quit somewhat faster. ;)

[Adam Roben (:aroben)]

(In reply to comment #0)
> Remove use of static C++ objects in general, but for an example from Timer.cpp:
> 
> static Vector<TimerBase*>* timerHeap;
> static HashSet<const TimerBase*>* timersReadyToFire;
> 
> In a multi-threaded WebCore environment, a thread may be using these while the
> main thread is deleting them causing crashes.

WebCore in general is not usable from multiple threads concurrently. I guess this would be a step towards making it safe to use in that fashion?

> Objects like this are also held in memory until exit is called, when the C++
> runtime cleans these up. So the destructors only run at exit and don't really
> provide any benefit. Without them, the program would quit somewhat faster. ;)

The two examples you gave are just pointers, and thus don't cause any destructors to run when the process exits. But what you said is true for static objects.

[Greg Bolsinga]

Arg. You are correct about those being pointers. Terrible example to quote in this bug report.

But yes in general this would be good for static objects.

[Greg Bolsinga]

I think this finds the statics on Mac OS X:

nm /Builds/Release/WebCore.framework/WebCore | grep " b " | sed -e"s|^........ b ||" | sort

[Greg Bolsinga]

WebKit is currently using this script:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/check-for-global-initializers

but that script is not finding some instances of this. For example it doesn't find one in the following. Search for gFunctionMap in:

http://trac.webkit.org/browser/trunk/WebCore/html/HTMLParser.cpp

Looking at nm output, I think these are in the bss section, but so are pointers to classes.

[Sam Weinig]

(In reply to comment #4)
> WebKit is currently using this script:
> 
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/check-for-global-initializers
> 
> but that script is not finding some instances of this. For example it doesn't
> find one in the following. Search for gFunctionMap in:

This is because gFunctionMap is not a global initializer, it is a lazily initialized the first time the getNode method is called.  

[Greg Bolsinga]

Sure. However, if you look at MediaQueryEvaluator, HTMLElementFactory, SVGElementFactory, etc. They have FunctionMaps too, but allocate them on the heap with new. I'd like to be able to find ones that are not new'd, and eliminate those.

[Darin Adler]

1) We're not going to remove all use of static C++ objects, so the title of this bug is bogus. I've retitled.

2) The check-for-global-initializers script checks for objects with global initializers. Those are objects that are initialized at library load time.

3) What Greg is after is eliminating global objects with exit-time destructor calls. I think it's reasonable to get rid of those. We already use a coding style that avoids them. It could speed up process termination for any program using WebKit and has the benefit Greg mentioned for the project he's working on.

The script to grep for all static objects is a mistake. We need something that finds only statics that have exit-time destructor code.

All the examples cited in this bug so far, such as timerHeap, timersReadyToFire, and gFunctionMap, are already fine. They don't have any exit-time destructors. But there are lots of objects that do have them. Such as staticWrapperSet in JSDOMBinding.cpp. The trick is to find some way to locate all of those.

However, in debug builds we use these exit-time destructors to log error messages after all other cleanup is complete. I'm not sure what the right thing to do for those is.

[Greg Bolsinga]

I think this has been solved by 22061.

[Greg Bolsinga]

*** This bug has been marked as a duplicate of 22061 ***

[Greg Bolsinga]

Ah well, too soon. HTMLParser.cpp's gFunctionMap wasn't caught by that script.

[Darin Adler]

The script has a list of files to skip. Until that list is empty we're not done.

[Darin Adler]

(In reply to comment #10)
> Ah well, too soon. HTMLParser.cpp's gFunctionMap wasn't caught by that script.

Specifically http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/check-for-exit-time-destructors contains this line:

    next if $shortName eq "HTMLParser.o";

If you remove this line then you'll see the error in that file. There are 89 files skipped right now. So writing the script is done; next step is to start fixing the cases in each of those 89 files (and 9 more files in WebKit).

[Greg Bolsinga]

Yep. I totally missed that. I had seen the script and the fixes to some parts of the code, and figured I had missed the commits for the rest going by.

[Greg Bolsinga]

Created attachment 24896 [details]
Removes many exit-time destructors

The types of exit time destructors this does not cover are for code like:

static RefPtr<T> sVar;

This would be:

static RefPtr<T>& sVar = *new RefPtr<T>;

and that just seems weird. Should these be OwnPtr's? There is a similar issue with RetainPtr<T>

[Darin Adler]

(In reply to comment #14)
> The types of exit time destructors this does not cover are for code like:
> 
> static RefPtr<T> sVar;
> 
> This would be:
> 
> static RefPtr<T>& sVar = *new RefPtr<T>;

For these I think we want this:

    static T* sVar = <expression>.releaseRef();

Just use a raw pointer for the intentionally leaked pointer. You'll have to change the sites that use the pointer to do things like remove calls to get().

[Darin Adler]

Comment on attachment 24896 [details]
Removes many exit-time destructors

> -    static ApplicationCacheStorage storage;
> +    static ApplicationCacheStorage& storage = *new ApplicationCacheStorage();

No need for the parens here.

r=me

[Greg Bolsinga]

I get different results once I run a release build:

CSSStyleSelector.o has exit time destructors in it! (/Builds/WebCore.build/Release/WebCore.build/Objects-normal/i386/CSSStyleSelector.o)
    WebCore::CSSStyleSelector::applyProperty(int, WebCore::CSSValue*)
FontCache.o has exit time destructors in it! (/Builds/WebCore.build/Release/WebCore.build/Objects-normal/i386/FontCache.o)
    WebCore::FontCache::purgeInactiveFontData(int)

[Darin Adler]

(In reply to comment #17)
> I get different results once I run a release build:
> 
> CSSStyleSelector.o has exit time destructors in it!
> (/Builds/WebCore.build/Release/WebCore.build/Objects-normal/i386/CSSStyleSelector.o)
>     WebCore::CSSStyleSelector::applyProperty(int, WebCore::CSSValue*)
> FontCache.o has exit time destructors in it!
> (/Builds/WebCore.build/Release/WebCore.build/Objects-normal/i386/FontCache.o)
>     WebCore::FontCache::purgeInactiveFontData(int)

Yes, I think that's why I broke the build when I first checked in. We'll have to try both debug and release builds.

It's possible the tool has a flaw, but it seems to be mostly working correctly.

[Greg Bolsinga]

(In reply to comment #18)
> Yes, I think that's why I broke the build when I first checked in. We'll have
> to try both debug and release builds.
> 
> It's possible the tool has a flaw, but it seems to be mostly working correctly.

Agreed. I'll get as many as I can now. There is definitely a _cxa_atexit, but I'm not sure what is forcing it to be called yet.

[Greg Bolsinga]

Created attachment 24917 [details]
Updated patch to get more objects

I've updated the patch to do some, but not all the RefPtr<T> code.

What to do with static RefPtr<T> m = f(), when PassRefPtr<T> f();?

[Greg Bolsinga]

Created attachment 24920 [details]
Missed one in WebHTMLRepresentation.mm

[Darin Adler]

(In reply to comment #20)
> What to do with static RefPtr<T> m = f(), when PassRefPtr<T> f();?

static T* m = f().releaseRef();

[Darin Adler]

Comment on attachment 24920 [details]
Missed one in WebHTMLRepresentation.mm

Am I re-reviewing stuff I reviewed before? Can you land part of this so we can just review the new parts?

Please don't keep making larger and larger patches. It's hard to review them!

> +	https://bugs.webkit.org/show_bug.cgi?id=21810
> +	Remove use of static C++ objects that are destroyed at exit time (destructors)

Please don't use tabs.

> -            static RetainPtr<NSColor> clearColor = [NSColor clearColor];
> -            return clearColor.get();
> +            static NSColor *clearColor = [NSColor clearColor];
> +            return clearColor;

How did you determine it was OK to not call retain in this case? Is the result of [NSColor clearColor] guaranteed to not be autoreleased?

> -            static RetainPtr<NSColor> cachedColors[cacheSize];
> +            static NSColor* cachedColors[cacheSize];

This change is wrong. We need to call CFRetain on the colors when we put them in the cache and CFRelease on the old colors when we punt them from the cache. I think that instead you should do this:

               static RetainPtr<NSColor>* cachedColors = new [cacheSize] RetainPtr<NSColor>;

> -    static RetainPtr<NSColor> spellingPatternColor = nil;
> +    static NSColor *spellingPatternColor = nil;

We need to retain the spelling pattern color when we initialize it. We can't keep the code as-is.

> -    static RetainPtr<NSColor> grammarPatternColor = nil;
> +    static NSColor *grammarPatternColor = nil;

Same comment.

> -    static RetainPtr<NSString> webFallbackFontFamily = nil;
> +    static NSString* webFallbackFontFamily = nil;

This change is wrong. We need to CFRetain the NSString when putting it into this variable.

> -    static RetainPtr<NSArray> types = nil;
> +    static NSArray *types = nil;

This change is wrong. We need to CFRetain the NSArray when putting it into this variable in case we're running under GC. Although I'm not entirely sure -- this might work properly in cases where the global is inside an Objective-C method and not a C/C++ function.

> -    static RetainPtr<NSMutableArray> types = nil;
> +    static NSMutableArray *types = nil;

Ditto.

> -    static RetainPtr<NSString> attachmentCharacterString = [NSString stringWithCharacters:&attachmentCharacter length:1];
> +    static NSString *attachmentCharacterString = [NSString stringWithCharacters:&attachmentCharacter length:1];

This change is wrong. We need to CFRetain this string.

> -    static RetainPtr<NSArray> types;
> +    static NSArray *types;

This change is wrong. We need to CFRetain the array in the case where we're running under GC.

> -    static RetainPtr<NSMutableArray> types;
> +    static NSMutableArray *types;

Ditto.

> -    static RetainPtr<NSMutableArray> types;
> +    static NSMutableArray *types;

Ditto.

> -    static RetainPtr<NSArray> types = [[NSArray alloc] initWithObjects:WebArchivePboardType, NSHTMLPboardType,
> +    static NSArray *types = [[NSArray alloc] initWithObjects:WebArchivePboardType, NSHTMLPboardType,
>             NSFilenamesPboardType, NSTIFFPboardType, NSPICTPboardType, NSURLPboardType, 
>             NSRTFDPboardType, NSRTFPboardType, NSStringPboardType, NSColorPboardType, nil];

Ditto.

>      
> -    static RetainPtr<NSArray> staticSupportedMIMETypes =
> +    static NSArray *staticSupportedMIMETypes =
>          concatenateArrays([self supportedNonImageMIMETypes], [self supportedImageMIMETypes]);

Ditto.

> -    static RetainPtr<NSArray> staticSupportedNonImageMIMETypes =
> +    static NSArray *staticSupportedNonImageMIMETypes =
>          stringArray(MIMETypeRegistry::getSupportedNonImageMIMETypes());

Ditto.

> -    static RetainPtr<NSArray> staticSupportedImageMIMETypes =
> +    static NSArray *staticSupportedImageMIMETypes =
>          stringArray(MIMETypeRegistry::getSupportedImageMIMETypes());

Ditto.

> +        next if (!defined($currentSymbol) || !length($currentSymbol));

This should just be:

    next unless $currentSymbol;

There's no case where $currentSymbol is undefined, and we can allow the string "0", so I think that's reads nicer.

review- because this includes quite a few incorrect RetainPtr removals.

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> (From update of attachment 24896 [details] [edit])
> > -    static ApplicationCacheStorage storage;
> > +    static ApplicationCacheStorage& storage = *new ApplicationCacheStorage();
> 
> No need for the parens here.
> 
> r=me

Landed Attachment #24896 [details] as r38189:

http://trac.webkit.org/changeset/38189

[Greg Bolsinga]

Created attachment 24956 [details]
Incremental patch addressing Retain & GC and PassRefPtr<T>

This gets all above but:

static String v = f(); where String f();

I completely missed the boat on GC! Thanks.

[Greg Bolsinga]

In addition EventHandlerMac.mm has:

static RetainPtr<NSEvent>& currentEvent();

and it is used like:

currentEvent() = event;

This and return values of String (from Comment 25) seem to require SPI changes, unless there is some C++ foo I've not yet attained.

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> (In reply to comment #16)
> > (From update of attachment 24896 [details] [edit] [edit])
> > > -    static ApplicationCacheStorage storage;
> > > +    static ApplicationCacheStorage& storage = *new ApplicationCacheStorage();
> > 
> > No need for the parens here.
> > 
> > r=me
> 
> Landed Attachment #24896 [details] [edit] as r38189:
> 
> http://trac.webkit.org/changeset/38189

Backed out in r38202:  http://trac.webkit.org/changeset/38202

Backed in in r38203:  http://trac.webkit.org/changeset/38203

Backed out again in r38207:  http://trac.webkit.org/changeset/38207

There is a bug in gcc that ships with Xcode 3.0 that must be worked around.

[Darin Adler]

Comment on attachment 24956 [details]
Incremental patch addressing Retain & GC and PassRefPtr<T>

> -    static RefPtr<Image> brokenImage;
> +    static Image* brokenImage = 0;
>      if (!brokenImage)
> -        brokenImage = Image::loadPlatformResource("missingImage");
> -    return brokenImage.get();
> +        brokenImage = Image::loadPlatformResource("missingImage").releaseRef();

There's no need for the if statement here; you didn't add it, but it's not needed.

    static Image* brokenImage = Image::loadPlatformResource("missingImage").releaseRef();

The above will do the same thing a bit more efficiently.

> +            static NSColor *clearColor = (NSColor*)CFRetain([NSColor clearColor]);

For these it would be slightly better to use the HardRetain function from FoundationExtras.h. I think that using HardRetain would eliminate the need for the typecast.

If it doesn't remove the need for the typecast, I'll mention that we'd normally use static_cast or reinterpret_cast rather than a C-style cast for these. Also, we use "NSColor *" with a space for Objective-C types. It's a confusing rule, but we've been following it consistently elsewhere.

> -        spellingPatternColor = color;
> +        spellingPatternColor = (NSColor*)CFRetain(color);

In these cases where it's just assignment and not a definition, I think it's better for type safety to just do the HardRetain on a separate line.

> -    static RetainPtr<NSString> webFallbackFontFamily = nil;
> +    static NSString* webFallbackFontFamily = nil;
>      if (!webFallbackFontFamily)
> -        webFallbackFontFamily = [[NSFont systemFontOfSize:16.0f] familyName];
> -    return webFallbackFontFamily.get();
> +        webFallbackFontFamily = (NSString*)CFRetain([[NSFont systemFontOfSize:16.0f] familyName]);
> +    return webFallbackFontFamily;

We use spaces in Objective-C types like "NSString *". This can be done in a single line. Please try this:

    static NSString *webFallbackFontFamily = HardRetain([[NSFont systemFontOfSize:16.0f] familyName]);

By the way, this single-line approach can be used even for more complicated cases by putting the code to compute the initial value into a separate function, perhaps an inline function. But I'm not asking you to change that everywhere -- just where it's simple and easy.

> -    static RetainPtr<NSMutableArray> types = nil;
> +    static NSMutableArray *types = nil;
>      if (!types) {
>          types = [[NSMutableArray alloc] initWithObjects:NSTIFFPboardType, nil];
> -        [types.get() addObjectsFromArray:writableTypesForURL()];
> -        [types.get() addObject:NSRTFDPboardType];
> +        [types addObjectsFromArray:writableTypesForURL()];
> +        [types addObject:NSRTFDPboardType];
> +        CFRetain(types);

This one could use HardRetainWithNSRelease; there's no need for the double retain in a case like this.

> -    static RefPtr<Image> resizeCornerImage;
> +    static Image* resizeCornerImage = 0;
>      if (!resizeCornerImage)
> -        resizeCornerImage = Image::loadPlatformResource("textAreaResizeCorner");
> +        resizeCornerImage = Image::loadPlatformResource("textAreaResizeCorner").releaseRef();

Another single-line candidate.

>  + (NSArray *)_web_writableTypesForURL
>  {
> -    static RetainPtr<NSArray> types;
> +    static NSArray *types = nil;
>      if (!types) {
>          types = [[NSArray alloc] initWithObjects:
>              WebURLsWithTitlesPboardType,
> @@ -67,29 +67,32 @@ NSString *WebURLNamePboardType = @"publi
>              WebURLNamePboardType,
>              NSStringPboardType,
>              nil];
> +        CFRetain(types);
>      }
> -    return types.get();
> +    return types;
>  }

Another HardRetainWithNSRelease candidate.

>  static NSArray *_writableTypesForImageWithoutArchive (void)
>  {
> -    static RetainPtr<NSMutableArray> types;
> +    static NSMutableArray *types = nil;
>      if (types == nil) {
>          types = [[NSMutableArray alloc] initWithObjects:NSTIFFPboardType, nil];
> -        [types.get() addObjectsFromArray:[NSPasteboard _web_writableTypesForURL]];
> +        [types addObjectsFromArray:[NSPasteboard _web_writableTypesForURL]];
> +        CFRetain(types);
>      }
> -    return types.get();
> +    return types;
>  }

Ditto.

>  static NSArray *_writableTypesForImageWithArchive (void)
>  {
> -    static RetainPtr<NSMutableArray> types;
> +    static NSMutableArray *types = nil;
>      if (types == nil) {
>          types = [_writableTypesForImageWithoutArchive() mutableCopy];
> -        [types.get() addObject:NSRTFDPboardType];
> -        [types.get() addObject:WebArchivePboardType];
> +        [types addObject:NSRTFDPboardType];
> +        [types addObject:WebArchivePboardType];
> +        CFRetain(types);
>      }
> -    return types.get();
> +    return types;
>  }

Ditto.

> -    static RetainPtr<NSArray> types = [[NSArray alloc] initWithObjects:WebArchivePboardType, NSHTMLPboardType,
> +    static NSArray *types = (NSArray*)CFRetain([[NSArray alloc] initWithObjects:WebArchivePboardType, NSHTMLPboardType,
>             NSFilenamesPboardType, NSTIFFPboardType, NSPICTPboardType, NSURLPboardType, 
> -           NSRTFDPboardType, NSRTFPboardType, NSStringPboardType, NSColorPboardType, nil];
> +           NSRTFDPboardType, NSRTFPboardType, NSStringPboardType, NSColorPboardType, nil]);

HardRetainWithNSRelease is exactly what we want here.

>          if (!$result) {
> -            print "$shortName has exit time destructors in it! ($path)\n";
> +            print "$shortName has exit time destructors in it!\n  ($path)\n";
>              $result = 1;
>          }

I don't like this change. Usually you want to ignore the full path of the file, which is why it's off to the right.

And if you are convinced that the new format is better, then I'd like to change the other two check-for scripts that use this same format for the error message.

I'm going to say review+ because none of my comments above seem like showstoppers, but I think this can be improved.

[Darin Adler]

(In reply to comment #26)
> In addition EventHandlerMac.mm has:
> 
> static RetainPtr<NSEvent>& currentEvent();
> 
> and it is used like:
> 
> currentEvent() = event;

That's no problem. If you write the function like this it will work:

    RetainPtr<NSEvent>& currentEvent()
    {
        static RetainPtr<NSEvent>& currentEventStorage = *new RetainPtr<NSEvent>;
        return currentEventStorage;
    }

> This and return values of String (from Comment 25) seem to require SPI changes,
> unless there is some C++ foo I've not yet attained.

I'm almost certain they won't require those changes. Please give me a specific example and I'll help you out with it.

[Greg Bolsinga]

Created attachment 24975 [details]
This is the bt of the crash that happens with the first patch applied

[Greg Bolsinga]

I have filed <rdar://problem/6354696> bad compiler code on the compiler problems found with the first patch from 11/05/2008 that is holding up landing this fix.

[Greg Bolsinga]

Also if I don't have static HashMap<String, unsigned>& defaultPorts = *new HashMap<String, unsigned>; and switch it back to static HashMap<String, unsigned> defaultPorts; the contents of defaultPorts in gdb look the same, and it doesn't crash.

[Greg Bolsinga]

Created attachment 25008 [details]
Updates patch that doesn't crash running all LayoutTests

This updates the patch that David Kilzer added and removed in Comment 27.

It sets GCC_THREADSAFE_STATICS = YES to work around the compiler bug reported in <rdar://problem/6354696> Codegen issue with C++ static reference in gcc build 5465

[Greg Bolsinga]

After the most recent path is reviewed and committed, I'll address the review comments in Comment 28 and Comment 29.

[Darin Adler]

(In reply to comment #33)
> It sets GCC_THREADSAFE_STATICS = YES to work around the compiler bug reported
> in <rdar://problem/6354696> Codegen issue with C++ static reference in gcc
> build 5465

Please do *not* land this change. We turned this option off for a speedup, and we don't want to give the speed back.

[Darin Adler]

Comment on attachment 25008 [details]
Updates patch that doesn't crash running all LayoutTests

-GCC_THREADSAFE_STATICS = NO;
+GCC_THREADSAFE_STATICS = YES;

This will cause a performance regression. We need to solve the problem some other way.

[George Staikos]

Won't this cause problems in cases where webkit is a dll that we load and unload dynamically?  Seems to be ruling out this use case permanently...

[Darin Adler]

(In reply to comment #37)
> Won't this cause problems in cases where webkit is a dll that we load and
> unload dynamically?  Seems to be ruling out this use case permanently...

If we want to create a project so that DLLs can be unloaded, we could visit these sites again and come up with a way to destroy the objects.

WebKit today isn't even close to working that way, and I don't think these changes create any major additional obstacles to doing that work some day.

[Greg Bolsinga]

Created attachment 25029 [details]
Remove *new construct

This is an update to the 11/05/2008 patch.

For each of these: Changed to leak an object to avoid an exit-time destructor.
        
In general code of the form:
        
static T m;
        
became:
        
static T* mPtr = new T;
 T& m = *mPtr;
        
This is to work around <rdar://problem/6354696> Codegen issue with C++ static reference in gcc build 5465

[Greg Bolsinga]

In addition, this gets some other code points that have the *new construct that were not in the previous patch. I tested it on a Leopard Xcode 3.0 system (with the bad compiler), and ran all LayoutTests without crashing.

[Greg Bolsinga]

See Bug 22183. The last patch covers this case.

[Eric Seidel (no email)]

Comment on attachment 25029 [details]
Remove *new construct

This seems really bad to me!  I'd much rather see a temporary macro being used to avoid this ugliness.  This is all to work around a temporary bug in gcc??

[Eric Seidel (no email)]

Comment on attachment 25029 [details]
Remove *new construct

Another single-line way to hand this would be:

static Mutex& mutex = *(new Mutex);

Assuming that compiles.

[Eric Seidel (no email)]

Comment on attachment 25029 [details]
Remove *new construct

Someone else can review this. :)  It's ugly.  I wish that we had a nicer solution to avoid this temporary ugliness, but I'll let the more-informed Apple people make that call.

[Adam Roben (:aroben)]

(In reply to comment #43)
> (From update of attachment 25029 [details] [review])
> Another single-line way to hand this would be:
> 
> static Mutex& mutex = *(new Mutex);
> 
> Assuming that compiles.

That will compile, and was in fact the way this patch was first written (though without the parentheses you added). But this caused crashes with some versions of GCC. See comment 30, comment 31, and comment 32.

[Darin Adler]

Greg, I think you're already doing this, but I think a macro would be a good idea. Something with a name like DEFINE_STATIC_LOCAL or something along those lines to be used like this:

    DEFINE_STATIC_LOCAL(type, name, arguments)

So you'd say:

    DEFINE_STATIC_LOCAL(HashSet<int>, intSet, ())

Is this what you're currently working on?

[Darin Adler]

Comment on attachment 24956 [details]
Incremental patch addressing Retain & GC and PassRefPtr<T>

Clearing review flag because of the GCC issue that we discovered.

[Darin Adler]

Comment on attachment 24896 [details]
Removes many exit-time destructors

Clearing review flag because of the GCC issue that we discovered.

[Darin Adler]

(In reply to comment #42)
> This is all to work around a temporary bug in gcc??

No.

There are two issues here:

    1) We need to avoid having destructors that are run at process exit time.

    2) The simplest style we came up with to do that ran into a bug in older versions of gcc and we're now trying to sidestep that issue.

[Greg Bolsinga]

I'm working the macros style now. Do varargs macros work across compilers? I think I'll need them for multiple value initializers.

[Darin Adler]

(In reply to comment #50)
> Do varargs macros work across compilers? I
> think I'll need them for multiple value initializers.

I don't think you need them. The arguments to the initializer should be a single macro argument:

    #define DEFINE_STATIC_LOCAL(type, name, arguments) \
        static type* const name##Ptr = new type arguments; \
        static type& name = *name##Ptr

Something like that. Used as I mentioned above.

[Greg Bolsinga]

Awesome. I did not know the parenthesis trick!

[Darin Adler]

(In reply to comment #52)
> Awesome. I did not know the parenthesis trick!

I know you're trying to get this whole thing done as fast as you can, but may I suggest that the first patch simply add the macro and then adopt it in places where we're already doing the "bad thing" just to fix the crashes with the old compiler? I'd like to see that done soon because I think it's bad that we're crashing on Tiger! That's basically a P1 regression I caused while trying to help fix the P2 issue.

Then do the "use the macro in more places" patches in subsequent chunks.

[Greg Bolsinga]

I have a full patch (matching the first one committed, removed, committed, removed) coming in about 30 minutes.

[Greg Bolsinga]

Created attachment 25102 [details]
This patch uses a macro

This is a cleaner patch. It uses a macro for DEFINE_STATIC_LOCAL. This macro always uses the ugly code right now. It can be updated to check compiler versions and platform versions to default the the sane static T& = *new T variant.

I built optimized  on a Leopard system with the 'bad' compiler, and ran all the LayoutTests without crashing.

[Alexey Proskuryakov]

Does this compiler bug exist for pointers? I'm not sure why we can't just write "static Mutex* mutex = new Mutex;" and change places where it is used. Seems much cleaner than a macro to me.

[Darin Adler]

(In reply to comment #56)
> Does this compiler bug exist for pointers? I'm not sure why we can't just write
> "static Mutex* mutex = new Mutex;" and change places where it is used. Seems
> much cleaner than a macro to me.

It's a possibility, but the macro has at least one other benefit: We could use the macro as a hook for some kind of destruction scheme other than "never destroy any of these objects" if we want to pursue that later.

[Greg Bolsinga]

(In reply to comment #56)
> Does this compiler bug exist for pointers? I'm not sure why we can't just write
> "static Mutex* mutex = new Mutex;" and change places where it is used. Seems
> much cleaner than a macro to me.

This macro is the smallest change, as all of these are used as non-pointer variables. Changing them to pointers would be an even larger change.

[Darin Adler]

Comment on attachment 25102 [details]
This patch uses a macro

> +        * wtf/Platform.h: Add DEFINE_STATIC_LOCAL macro

This file is not the right place for the macro. I know it's tempting because it's a header that's "included everywhere", but as you discovered while working on the patch, it's not included everywhere. And the header is supposed to contain only platform-configuration macros, not general purpose ones like this.

Lets put this in its own separate header file. As we discussed on IRC, I think it could be <wtf/StdLibExtras.h> since this is closely related to exit() and atexit() behavior. I think it might be OK to stick this into "config.h" files so we don't have to include it everywhere, or add the includes everywhere we need it.

> +typedef HashMap<int, PropertyLonghand> ShortHandMap;

Since "shorthand" is one word, it should be ShorthandMap, not ShortHandMap. Also, to match the other names that use this.

> -            static const String rectParen("rect(");
> +            DEFINE_STATIC_LOCAL(String, rectParen, ("rect("));

You're removing the "const" here and in many other places too.

    DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));

The above would work and I think be slightly better because it preserves the const. These globals can be const or non-const. We could even make some more const that weren't previously const.

> -    static Cursor& c = leakNamedCursor("crossHairCursor", 11, 11);
> +    static Cursor* cPtr = leakNamedCursor("crossHairCursor", 11, 11);
> +    Cursor& c = *cPtr;

These aren't using the macro. They should.

> -    static HashMap<String, int>& stringIdentifierToGUIDMap = *new HashMap<String, int>;
> +    typedef HashMap<String, int> IDGuidMap;
> +    DEFINE_STATIC_LOCAL(IDGuidMap, stringIdentifierToGUIDMap, ());

Are you adding these typedefs for clarity, or does the macro malfunction if you don't use them? I would have tried something like this:

    DEFINE_STATIC_LOCAL(HashMap<String, int>, stringIdentifierToGUIDMap, ());

I think when you land this it should be in small pieces. The first patch could just add the macro and include the JavaScriptCore changes. Then you could do the WebCore change and the WebKit change each as a separate patch or a number of them.

Then you could finish up with the tools list change as a separate patch.

This would be especially good if some of these cause problems. Someone could figure out which patch broke things if the patches are smaller pieces.

review- because of the Platform.h issue

[Greg Bolsinga]

(In reply to comment #59)

> atexit() behavior. I think it might be OK to stick this into "config.h" files
> so we don't have to include it everywhere, or add the includes everywhere we
> need it.

I'll include everywhere required

> > -            static const String rectParen("rect(");
> > +            DEFINE_STATIC_LOCAL(String, rectParen, ("rect("));
> 
> You're removing the "const" here and in many other places too.
> 
>     DEFINE_STATIC_LOCAL(const String, rectParen, ("rect("));
> 
> The above would work and I think be slightly better because it preserves the
> const. These globals can be const or non-const. We could even make some more
> const that weren't previously const.

I'll see if I can safely have all of them be const inside of the macro; if that doesn't work I'll const them as needed.

> > -    static Cursor& c = leakNamedCursor("crossHairCursor", 11, 11);
> > +    static Cursor* cPtr = leakNamedCursor("crossHairCursor", 11, 11);
> > +    Cursor& c = *cPtr;
> 
> These aren't using the macro. They should.

I'll have to add a new macro for these, as there is no 'new' involved.

> Are you adding these typedefs for clarity, or does the macro malfunction if you
> don't use them? I would have tried something like this:

The macro fails without them

[Greg Bolsinga]

Comment on attachment 25102 [details]
This patch uses a macro

> +#define DEFINE_STATIC_LOCAL(type, name, arguments) \
> +    static type* name##Ptr = new type arguments; \
> +    type& name = *name##Ptr

To see how fun this compiler bug is, I had tried what Darin originally suggested:

 +    static type* const name##Ptr = new type arguments; \

That crashes like crazy!

[Greg Bolsinga]

Created attachment 25143 [details]
JavaScriptCore only patch

It has a new header file <wtf/StdLibExtras.h> that is included only by the files that need this functionality.

[Greg Bolsinga]

Created attachment 25144 [details]
WebCore only patch

This uses wtf/StdLibExtras.h where applicable. It uses const in the same places as before. It also uses an ASSIGN macro for the CursorMac.mm case.

[Greg Bolsinga]

Created attachment 25145 [details]
WebKit only patch

This uses wtf/StdLibExtras.h where applicable.

[Greg Bolsinga]

Created attachment 25146 [details]
WebKitTools only patch

This updates the script to remove classes that no longer have static C++ global destructors.

[Darin Adler]

Comment on attachment 25143 [details]
JavaScriptCore only patch

> +#define DEFINE_STATIC_LOCAL_ASSIGN(type, name, data) \
> +    static type* name##Ptr = data; \
> +    type& name = *name##Ptr

I think we should come up with a better name for this later. I'm not sure this is really a static local at all and if it is, I think the word "pointer" needs to be in the name somewhere, since data is a type*, not an initialization parameter for a type. Maybe DEFINE_STATIC_LOCAL_WITH_POINTER. Now I almost wish I hadn't asked for the Cursor class changes!

>  #include "config.h"
> +#include "StdLibExtras.h"
>  #include "Threading.h"

Threading.h here is the file's "own" header, so "StdLibExtras.h" should go in a separate include paragraph below.

r=me

[Darin Adler]

Comment on attachment 25144 [details]
WebCore only patch

> -static Cursor& leakNamedCursor(const char* name, int x, int y)
> +static Cursor* leakNamedCursor(const char* name, int x, int y)

I think that if you made this return an NSCursor * instead of a Cursor you could just change the call sites to use the plain old DEFINE_STATIC_LOCAL and you wouldn't need DEFINE_STATIC_LOCAL_ASSIGN at all.

> -    static Cursor& c = leakNamedCursor("crossHairCursor", 11, 11);
> +    DEFINE_STATIC_LOCAL_ASSIGN(Cursor, c, (leakNamedCursor("crossHairCursor", 11, 11)));

You don't need the parentheses around the leakNamedCursor call. But really, we should eliminate the name "leak" from this entirely and use DEFINE_STATIC_LOCAL if we can.

r=me

[Darin Adler]

Comment on attachment 25145 [details]
WebKit only patch

r=me

[Darin Adler]

Comment on attachment 25146 [details]
WebKitTools only patch

r=me

[Greg Bolsinga]

(In reply to comment #67)
> I think that if you made this return an NSCursor * instead of a Cursor you
> could just change the call sites to use the plain old DEFINE_STATIC_LOCAL and
> you wouldn't need DEFINE_STATIC_LOCAL_ASSIGN at all.

I've done exactly that, and it looks much better.

[Greg Bolsinga]

(In reply to comment #66)
> r=me

http://trac.webkit.org/changeset/38411

[Greg Bolsinga]

(In reply to comment #67)
> r=me

http://trac.webkit.org/changeset/38418

[Greg Bolsinga]

(In reply to comment #68)
> (From update of attachment 25145 [details] [review])
> r=me

http://trac.webkit.org/changeset/38421

[Greg Bolsinga]

(In reply to comment #69)
> (From update of attachment 25146 [details] [review])
> r=me
> 

http://trac.webkit.org/changeset/38422

[Darin Adler]

Comment on attachment 25143 [details]
JavaScriptCore only patch

Clearing review flag since this patch has been landed, but the bug is still open, so this won't show up in the commit queue

[Darin Adler]

Comment on attachment 25144 [details]
WebCore only patch

Clearing review flag since this patch has been landed, but the bug is still open, so this won't show up in the commit queue

[Darin Adler]

Comment on attachment 25145 [details]
WebKit only patch

Clearing review flag since this patch has been landed, but the bug is still open, so this won't show up in the commit queue

[Darin Adler]

Comment on attachment 25146 [details]
WebKitTools only patch

Clearing review flag since this patch has been landed, but the bug is still open, so this won't show up in the commit queue

[Greg Bolsinga]

Created attachment 25189 [details]
WebCore only patch addressing RetainPtr<>, RefPtr<>

This is basically an update of Attachment 24956 [details] covering Darin's review.

[Greg Bolsinga]

Created attachment 25190 [details]
WebKit only patch addressing RetainPtr<>, RefPtr<>

This is basically an update of Attachment 24956 [details] covering Darin's review.

[Greg Bolsinga]

Created attachment 25191 [details]
WebKitTools only patch addressing RetainPtr<>, RefPtr<>

This is basically an update of Attachment 24956 [details] covering Darin's review.

[Greg Bolsinga]

Created attachment 25194 [details]
JavaScriptCore patch to workaround buggy compilers only when required

Conditionally have the DEFINE_STATIC_LOCAL workaround <rdar://problem/6354696> (Codegen issue with C++ static reference in gcc build 5465) based upon the compiler build versions. It will use the:
        static T& = *new T;
style for all other compilers.

[Darin Adler]

Comment on attachment 25189 [details]
WebCore only patch addressing RetainPtr<>, RefPtr<>

As we discussed, lets use DEFINE_STATIC_LOCAL in more places instead of more efficient hand coded ones.

Also, a couple places where you removed const.

[Darin Adler]

Comment on attachment 25190 [details]
WebKit only patch addressing RetainPtr<>, RefPtr<>

As we discussed, lets use DEFINE_STATIC_LOCAL in more places instead of more efficient hand coded ones. But we still might want to use those helper functions to make the initialization cleaner.

[Darin Adler]

Comment on attachment 25194 [details]
JavaScriptCore patch to workaround buggy compilers only when required

> +#if COMPILER(GCC) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 0) && (__GNUC_PATCHLEVEL__ == 1) && (__APPLE_CC__ == 5465)

This vesrion check seems a little too specific to me. Are you sure that no other version needs it?

r=me

[Greg Bolsinga]

(In reply to comment #85)
> This vesrion check seems a little too specific to me. Are you sure that no
> other version needs it?

This is the only one we saw it on, so I wanted to be very specific.

[Greg Bolsinga]

Created attachment 25200 [details]
Address Comment 83 for WebCore

Please look at FTPDirectoryDocument.cpp closely.

[Greg Bolsinga]

Created attachment 25201 [details]
Address Comment 84 for WebKit

[Greg Bolsinga]

(In reply to comment #85)
> r=me

http://trac.webkit.org/changeset/38457

[Greg Bolsinga]

Comment on attachment 25194 [details]
JavaScriptCore patch to workaround buggy compilers only when required

Clearing flag since it is committed.

[Greg Bolsinga]

Comment on attachment 24956 [details]
Incremental patch addressing Retain & GC and PassRefPtr<T>

This has been obsoleted.

[Darin Adler]

Comment on attachment 25200 [details]
Address Comment 83 for WebCore

> -    static RefPtr<BitmapImage> nullImage = BitmapImage::create();
> +    DEFINE_STATIC_LOCAL(RefPtr<BitmapImage>, nullImage, (BitmapImage::create()));;

Extra semicolon here.

> +static inline SharedBuffer* _createTemplateDocumentData(Settings *settings)

The * should be next to Settings, not "settings".

There's no reason to have a leading underscore in this function name.

The argument here could be the Document rather than the Settings. But it's fine this way.

> +static inline NSColor* _createPatternColor(NSString* name, NSColor* defaultColor, bool& usingDot)

There's no reason to have a leading underscore in this function name.

I'm don't think this function should be marked inline.

r=me

[Darin Adler]

Comment on attachment 25201 [details]
Address Comment 84 for WebKit

> +    DEFINE_STATIC_LOCAL(RetainPtr<NSArray>, types, ([[NSArray alloc] initWithObjects:
> +                                WebURLsWithTitlesPboardType,
> +                                NSURLPboardType,
> +                                WebURLPboardType,
> +                                WebURLNamePboardType,
> +                                NSStringPboardType,
> +                                nil]));

I would just indent these type names in four spaces, not so far in, if it was me

r=me

[Greg Bolsinga]

(In reply to comment #92)
> r=me
> 

http://trac.webkit.org/changeset/38458

[Greg Bolsinga]

Comment on attachment 25200 [details]
Address Comment 83 for WebCore

clearing flag since it is committed.

[Greg Bolsinga]

(In reply to comment #93)
> r=me
> 

http://trac.webkit.org/changeset/38469

[Greg Bolsinga]

Comment on attachment 25201 [details]
Address Comment 84 for WebKit

This has been committed.

[Greg Bolsinga]

WebKitTools committed:

http://trac.webkit.org/changeset/38476

[Greg Bolsinga]

Comment on attachment 25191 [details]
WebKitTools only patch addressing RetainPtr<>, RefPtr<>

This has been committed.

[Greg Bolsinga]

So these are the remaining files (only in WebCore):

"AccessibilityRenderObject.o";
"CSSStyleSelector.o";
"FontCache.o";
"JSHTMLElementWrapperFactory.o";
"JSSVGAnimatedLength.o";
"JSSVGAnimatedRect.o";
"JSSVGElementWrapperFactory.o";
"JSSVGLength.o";
"JSSVGRect.o";
"StyleTransformData.o";

Nothing obvious jumps out at me in the source code. I'm not sure if it is a tool bug, or something subtler.

[Greg Bolsinga]

Created attachment 25207 [details]
This gets the last of them in WebCore

It also has to generate code properly.

[Greg Bolsinga]

Created attachment 25208 [details]
Tool now has no exceptions

This also prints out what the developer ought to do if they create a static C++ object with an atexit dtor by pointing them to DEFINE_STATIC_LOCAL and StdLibExtras.h

[Darin Adler]

Comment on attachment 25207 [details]
This gets the last of them in WebCore

r=me

[Greg Bolsinga]

(In reply to comment #103)
> (From update of attachment 25207 [details] [review])
> r=me
> 

http://trac.webkit.org/changeset/38502

[Greg Bolsinga]

Comment on attachment 25207 [details]
This gets the last of them in WebCore

This has been committed.

[Darin Adler]

Comment on attachment 25208 [details]
Tool now has no exceptions

r=me

[Greg Bolsinga]

(In reply to comment #106)
> (From update of attachment 25208 [details] [review])
> r=me
> 

http://trac.webkit.org/changeset/38598

[Greg Bolsinga]

Comment on attachment 25208 [details]
Tool now has no exceptions

committed.