Bug 21810 - Remove use of static C++ objects that are destroyed at exit time (destructors)
Summary: Remove use of static C++ objects that are destroyed at exit time (destructors)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Greg Bolsinga
URL:
Keywords:
Depends on: 22061
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-22 14:31 PDT by Greg Bolsinga
Modified: 2023-03-02 11:02 PST (History)
8 users (show)

See Also:


Attachments
Removes many exit-time destructors (89.74 KB, patch)
2008-11-04 15:10 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
Updated patch to get more objects (102.04 KB, patch)
2008-11-05 14:08 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
Missed one in WebHTMLRepresentation.mm (102.35 KB, patch)
2008-11-05 14:49 PST, Greg Bolsinga
darin: review-
Details | Formatted Diff | Diff
Incremental patch addressing Retain & GC and PassRefPtr<T> (24.69 KB, patch)
2008-11-06 14:23 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
This is the bt of the crash that happens with the first patch applied (5.52 KB, text/plain)
2008-11-07 16:31 PST, Greg Bolsinga
no flags Details
Updates patch that doesn't crash running all LayoutTests (83.51 KB, patch)
2008-11-09 14:18 PST, Greg Bolsinga
darin: review-
Details | Formatted Diff | Diff
Remove *new construct (122.83 KB, patch)
2008-11-10 14:57 PST, Greg Bolsinga
eric: review-
Details | Formatted Diff | Diff
This patch uses a macro (116.92 KB, patch)
2008-11-12 13:14 PST, Greg Bolsinga
darin: review-
Details | Formatted Diff | Diff
JavaScriptCore only patch (8.47 KB, patch)
2008-11-13 16:49 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
WebCore only patch (113.92 KB, patch)
2008-11-13 16:51 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
WebKit only patch (6.03 KB, patch)
2008-11-13 16:52 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
WebKitTools only patch (7.08 KB, patch)
2008-11-13 16:53 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
WebCore only patch addressing RetainPtr<>, RefPtr<> (21.00 KB, patch)
2008-11-15 12:51 PST, Greg Bolsinga
darin: review-
Details | Formatted Diff | Diff
WebKit only patch addressing RetainPtr<>, RefPtr<> (7.30 KB, patch)
2008-11-15 12:52 PST, Greg Bolsinga
darin: review-
Details | Formatted Diff | Diff
WebKitTools only patch addressing RetainPtr<>, RefPtr<> (2.91 KB, patch)
2008-11-15 12:53 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
JavaScriptCore patch to workaround buggy compilers only when required (1.75 KB, patch)
2008-11-15 13:31 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
Address Comment 83 for WebCore (22.89 KB, patch)
2008-11-16 15:05 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
Address Comment 84 for WebKit (6.91 KB, patch)
2008-11-16 15:06 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
This gets the last of them in WebCore (5.42 KB, patch)
2008-11-16 22:02 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff
Tool now has no exceptions (1.87 KB, patch)
2008-11-16 22:03 PST, Greg Bolsinga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Bolsinga 2008-10-22 14:31:32 PDT
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. ;)
Comment 1 Adam Roben (:aroben) 2008-10-22 14:39:40 PDT
(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.
Comment 2 Greg Bolsinga 2008-10-22 14:49:09 PDT
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.
Comment 3 Greg Bolsinga 2008-10-22 18:44:41 PDT
I think this finds the statics on Mac OS X:

nm /Builds/Release/WebCore.framework/WebCore | grep " b " | sed -e"s|^........ b ||" | sort
Comment 4 Greg Bolsinga 2008-10-27 11:50:44 PDT
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.

Comment 5 Sam Weinig 2008-10-27 12:07:26 PDT
(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.  
Comment 6 Greg Bolsinga 2008-10-27 12:51:29 PDT
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.
Comment 7 Darin Adler 2008-10-27 13:20:04 PDT
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.
Comment 8 Greg Bolsinga 2008-11-04 11:09:28 PST
I think this has been solved by 22061.
Comment 9 Greg Bolsinga 2008-11-04 11:11:44 PST

*** This bug has been marked as a duplicate of 22061 ***
Comment 10 Greg Bolsinga 2008-11-04 11:18:19 PST
Ah well, too soon. HTMLParser.cpp's gFunctionMap wasn't caught by that script.
Comment 11 Darin Adler 2008-11-04 11:20:40 PST
The script has a list of files to skip. Until that list is empty we're not done.
Comment 12 Darin Adler 2008-11-04 11:32:04 PST
(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).
Comment 13 Greg Bolsinga 2008-11-04 11:34:45 PST
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.
Comment 14 Greg Bolsinga 2008-11-04 15:10:11 PST
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>
Comment 15 Darin Adler 2008-11-04 15:30:10 PST
(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().
Comment 16 Darin Adler 2008-11-04 15:33:49 PST
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
Comment 17 Greg Bolsinga 2008-11-04 15:57:08 PST
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)
Comment 18 Darin Adler 2008-11-04 15:59:25 PST
(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.
Comment 19 Greg Bolsinga 2008-11-04 16:04:21 PST
(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.
Comment 20 Greg Bolsinga 2008-11-05 14:08:17 PST
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();?
Comment 21 Greg Bolsinga 2008-11-05 14:49:11 PST
Created attachment 24920 [details]
Missed one in WebHTMLRepresentation.mm
Comment 22 Darin Adler 2008-11-05 15:38:40 PST
(In reply to comment #20)
> What to do with static RefPtr<T> m = f(), when PassRefPtr<T> f();?

static T* m = f().releaseRef();
Comment 23 Darin Adler 2008-11-05 15:54:29 PST
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.
Comment 24 David Kilzer (:ddkilzer) 2008-11-06 11:29:29 PST
(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

Comment 25 Greg Bolsinga 2008-11-06 14:23:00 PST
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.
Comment 26 Greg Bolsinga 2008-11-06 14:29:15 PST
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.
Comment 27 David Kilzer (:ddkilzer) 2008-11-06 16:57:13 PST
(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.

Comment 28 Darin Adler 2008-11-07 09:15:09 PST
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.
Comment 29 Darin Adler 2008-11-07 09:30:00 PST
(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.
Comment 30 Greg Bolsinga 2008-11-07 16:31:26 PST
Created attachment 24975 [details]
This is the bt of the crash that happens with the first patch applied
Comment 31 Greg Bolsinga 2008-11-07 16:56:32 PST
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.
Comment 32 Greg Bolsinga 2008-11-07 16:59:02 PST
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.
Comment 33 Greg Bolsinga 2008-11-09 14:18:38 PST
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
Comment 34 Greg Bolsinga 2008-11-09 14:20:03 PST
After the most recent path is reviewed and committed, I'll address the review comments in Comment 28 and Comment 29.
Comment 35 Darin Adler 2008-11-09 16:18:42 PST
(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.
Comment 36 Darin Adler 2008-11-09 16:58:49 PST
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.
Comment 37 George Staikos 2008-11-10 13:40:09 PST
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...
Comment 38 Darin Adler 2008-11-10 13:58:26 PST
(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.
Comment 39 Greg Bolsinga 2008-11-10 14:57:43 PST
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
Comment 40 Greg Bolsinga 2008-11-10 14:59:10 PST
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.
Comment 41 Greg Bolsinga 2008-11-11 16:51:54 PST
See Bug 22183. The last patch covers this case.
Comment 42 Eric Seidel (no email) 2008-11-11 17:15:34 PST
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??
Comment 43 Eric Seidel (no email) 2008-11-11 17:18:09 PST
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.
Comment 44 Eric Seidel (no email) 2008-11-11 17:30:03 PST
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.
Comment 45 Adam Roben (:aroben) 2008-11-12 08:29:47 PST
(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.
Comment 46 Darin Adler 2008-11-12 09:57:06 PST
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?
Comment 47 Darin Adler 2008-11-12 09:57:41 PST
Comment on attachment 24956 [details]
Incremental patch addressing Retain & GC and PassRefPtr<T>

Clearing review flag because of the GCC issue that we discovered.
Comment 48 Darin Adler 2008-11-12 09:57:45 PST
Comment on attachment 24896 [details]
Removes many exit-time destructors

Clearing review flag because of the GCC issue that we discovered.
Comment 49 Darin Adler 2008-11-12 09:59:15 PST
(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.
Comment 50 Greg Bolsinga 2008-11-12 10:17:32 PST
I'm working the macros style now. Do varargs macros work across compilers? I think I'll need them for multiple value initializers.
Comment 51 Darin Adler 2008-11-12 10:21:52 PST
(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.
Comment 52 Greg Bolsinga 2008-11-12 11:22:33 PST
Awesome. I did not know the parenthesis trick!
Comment 53 Darin Adler 2008-11-12 12:49:05 PST
(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.
Comment 54 Greg Bolsinga 2008-11-12 12:58:47 PST
I have a full patch (matching the first one committed, removed, committed, removed) coming in about 30 minutes.
Comment 55 Greg Bolsinga 2008-11-12 13:14:02 PST
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.
Comment 56 Alexey Proskuryakov 2008-11-12 14:36:59 PST
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.
Comment 57 Darin Adler 2008-11-12 14:43:05 PST
(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.
Comment 58 Greg Bolsinga 2008-11-13 12:55:10 PST
(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.
Comment 59 Darin Adler 2008-11-13 13:30:27 PST
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
Comment 60 Greg Bolsinga 2008-11-13 13:40:17 PST
(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
Comment 61 Greg Bolsinga 2008-11-13 16:31:33 PST
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!
Comment 62 Greg Bolsinga 2008-11-13 16:49:03 PST
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.
Comment 63 Greg Bolsinga 2008-11-13 16:51:36 PST
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.
Comment 64 Greg Bolsinga 2008-11-13 16:52:32 PST
Created attachment 25145 [details]
WebKit only patch

This uses wtf/StdLibExtras.h where applicable.
Comment 65 Greg Bolsinga 2008-11-13 16:53:21 PST
Created attachment 25146 [details]
WebKitTools only patch

This updates the script to remove classes that no longer have static C++ global destructors.
Comment 66 Darin Adler 2008-11-13 16:59:08 PST
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
Comment 67 Darin Adler 2008-11-13 17:03:12 PST
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
Comment 68 Darin Adler 2008-11-13 17:03:35 PST
Comment on attachment 25145 [details]
WebKit only patch

r=me
Comment 69 Darin Adler 2008-11-13 17:04:13 PST
Comment on attachment 25146 [details]
WebKitTools only patch

r=me
Comment 70 Greg Bolsinga 2008-11-14 09:27:06 PST
(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.
Comment 71 Greg Bolsinga 2008-11-14 17:30:40 PST
(In reply to comment #66)
> r=me

http://trac.webkit.org/changeset/38411
Comment 72 Greg Bolsinga 2008-11-14 20:48:01 PST
(In reply to comment #67)
> r=me

http://trac.webkit.org/changeset/38418
Comment 73 Greg Bolsinga 2008-11-15 08:56:13 PST
(In reply to comment #68)
> (From update of attachment 25145 [details] [review])
> r=me

http://trac.webkit.org/changeset/38421
Comment 74 Greg Bolsinga 2008-11-15 10:21:01 PST
(In reply to comment #69)
> (From update of attachment 25146 [details] [review])
> r=me
> 

http://trac.webkit.org/changeset/38422
Comment 75 Darin Adler 2008-11-15 12:10:09 PST
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
Comment 76 Darin Adler 2008-11-15 12:10:12 PST
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
Comment 77 Darin Adler 2008-11-15 12:10:17 PST
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
Comment 78 Darin Adler 2008-11-15 12:10:20 PST
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
Comment 79 Greg Bolsinga 2008-11-15 12:51:45 PST
Created attachment 25189 [details]
WebCore only patch addressing RetainPtr<>, RefPtr<>

This is basically an update of Attachment 24956 [details] covering Darin's review.
Comment 80 Greg Bolsinga 2008-11-15 12:52:44 PST
Created attachment 25190 [details]
WebKit only patch addressing RetainPtr<>, RefPtr<>

This is basically an update of Attachment 24956 [details] covering Darin's review.
Comment 81 Greg Bolsinga 2008-11-15 12:53:31 PST
Created attachment 25191 [details]
WebKitTools only patch addressing RetainPtr<>, RefPtr<>

This is basically an update of Attachment 24956 [details] covering Darin's review.
Comment 82 Greg Bolsinga 2008-11-15 13:31:05 PST
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.
Comment 83 Darin Adler 2008-11-16 12:45:18 PST
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.
Comment 84 Darin Adler 2008-11-16 12:46:06 PST
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.
Comment 85 Darin Adler 2008-11-16 12:47:30 PST
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
Comment 86 Greg Bolsinga 2008-11-16 13:29:27 PST
(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.
Comment 87 Greg Bolsinga 2008-11-16 15:05:19 PST
Created attachment 25200 [details]
Address Comment 83 for WebCore

Please look at FTPDirectoryDocument.cpp closely.
Comment 88 Greg Bolsinga 2008-11-16 15:06:24 PST
Created attachment 25201 [details]
Address Comment 84 for WebKit
Comment 89 Greg Bolsinga 2008-11-16 15:08:31 PST
(In reply to comment #85)
> r=me

http://trac.webkit.org/changeset/38457
Comment 90 Greg Bolsinga 2008-11-16 15:08:55 PST
Comment on attachment 25194 [details]
JavaScriptCore patch to workaround buggy compilers only when required

Clearing flag since it is committed.
Comment 91 Greg Bolsinga 2008-11-16 15:09:39 PST
Comment on attachment 24956 [details]
Incremental patch addressing Retain & GC and PassRefPtr<T>

This has been obsoleted.
Comment 92 Darin Adler 2008-11-16 15:13:50 PST
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
Comment 93 Darin Adler 2008-11-16 15:15:08 PST
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
Comment 94 Greg Bolsinga 2008-11-16 17:05:59 PST
(In reply to comment #92)
> r=me
> 

http://trac.webkit.org/changeset/38458
Comment 95 Greg Bolsinga 2008-11-16 17:06:34 PST
Comment on attachment 25200 [details]
Address Comment 83 for WebCore

clearing flag since it is committed.
Comment 96 Greg Bolsinga 2008-11-16 20:19:28 PST
(In reply to comment #93)
> r=me
> 

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

Comment 97 Greg Bolsinga 2008-11-16 20:19:50 PST
Comment on attachment 25201 [details]
Address Comment 84 for WebKit

This has been committed.
Comment 98 Greg Bolsinga 2008-11-16 20:21:11 PST
WebKitTools committed:

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

Comment 99 Greg Bolsinga 2008-11-16 20:21:32 PST
Comment on attachment 25191 [details]
WebKitTools only patch addressing RetainPtr<>, RefPtr<>

This has been committed.
Comment 100 Greg Bolsinga 2008-11-16 20:24:59 PST
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.
Comment 101 Greg Bolsinga 2008-11-16 22:02:08 PST
Created attachment 25207 [details]
This gets the last of them in WebCore

It also has to generate code properly.
Comment 102 Greg Bolsinga 2008-11-16 22:03:24 PST
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
Comment 103 Darin Adler 2008-11-17 09:04:50 PST
Comment on attachment 25207 [details]
This gets the last of them in WebCore

r=me
Comment 104 Greg Bolsinga 2008-11-17 09:11:49 PST
(In reply to comment #103)
> (From update of attachment 25207 [details] [review])
> r=me
> 

http://trac.webkit.org/changeset/38502
Comment 105 Greg Bolsinga 2008-11-17 09:12:19 PST
Comment on attachment 25207 [details]
This gets the last of them in WebCore

This has been committed.
Comment 106 Darin Adler 2008-11-19 09:59:32 PST
Comment on attachment 25208 [details]
Tool now has no exceptions

r=me
Comment 107 Greg Bolsinga 2008-11-19 10:10:19 PST
(In reply to comment #106)
> (From update of attachment 25208 [details] [review])
> r=me
> 

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

Comment 108 Greg Bolsinga 2008-11-19 10:10:34 PST
Comment on attachment 25208 [details]
Tool now has no exceptions

committed.