Reviewed by Darin.
authorthatcher <thatcher@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Oct 2006 22:56:13 +0000 (22:56 +0000)
committerthatcher <thatcher@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 18 Oct 2006 22:56:13 +0000 (22:56 +0000)
        <rdar://problem/4752138> Manipulating popup in HTML page crashed Xcode

        - Store the Mac popup button cell as a RetainPtr to prevent GC collection.
        - Convert more ObjC local statics and member variables to use RetainPtr.
        - Convert more CFRetain/CFRelease to HardRetain/HardRelease.

        * bindings/objc/DOM.mm:
        (ObjCNodeFilterCondition::ObjCNodeFilterCondition): use HardRetain and not CFRetain
        (ObjCNodeFilterCondition::~ObjCNodeFilterCondition): use HardRelease and not CFRelease
        * bridge/mac/AXObjectCacheMac.mm:
        (WebCore::AXObjectCache::~AXObjectCache): use HardRelease and not CFRelease
        (WebCore::AXObjectCache::get): use HardRetain and not CFRetain
        (WebCore::AXObjectCache::remove): use HardRelease and not CFRelease
        * bridge/mac/WebCoreFrameBridge.mm:
        (+[WebCoreFrameBridge supportedImageResourceMIMETypes]): use RetainPtr
        (+[WebCoreFrameBridge supportedImageMIMETypes]): ditto
        * bridge/mac/WebCoreIconDatabaseBridge.mm:
        (+[WebCoreIconDatabaseBridge sharedInstance]): use RetainPtr
        * platform/PopupMenu.h: renamed popup to m_popup and switched to RetainPtr<NSPopUpButtonCell>
        * platform/cf/RetainPtr.h:
        (WTF::::operator): implement a missing operator= template function
        * platform/mac/PopupMenuMac.mm:
        (WebCore::PopupMenu::PopupMenu): remove nil initialization
        (WebCore::PopupMenu::~PopupMenu): use .get(), remove release message and rename to m_popup
        (WebCore::PopupMenu::clear): use .get() when sending a message and rename to m_popup
        (WebCore::PopupMenu::populate): ditto
        (WebCore::PopupMenu::show): ditto
        (WebCore::PopupMenu::hide): ditto
        (WebCore::PopupMenu::addSeparator): ditto
        (WebCore::PopupMenu::addGroupLabel): ditto
        (WebCore::PopupMenu::addOption): ditto

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@17121 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/bindings/objc/DOM.mm
WebCore/bridge/mac/AXObjectCacheMac.mm
WebCore/bridge/mac/WebCoreFrameBridge.mm
WebCore/bridge/mac/WebCoreIconDatabaseBridge.mm
WebCore/platform/PopupMenu.h
WebCore/platform/cf/RetainPtr.h
WebCore/platform/mac/PopupMenuMac.mm

index 85eddd5..25d52e6 100644 (file)
@@ -1,3 +1,39 @@
+2006-10-18  Timothy Hatcher  <timothy@apple.com>
+
+        Reviewed by Darin.
+
+        <rdar://problem/4752138> Manipulating popup in HTML page crashed Xcode
+
+        - Store the Mac popup button cell as a RetainPtr to prevent GC collection.
+        - Convert more ObjC local statics and member variables to use RetainPtr.
+        - Convert more CFRetain/CFRelease to HardRetain/HardRelease.
+
+        * bindings/objc/DOM.mm:
+        (ObjCNodeFilterCondition::ObjCNodeFilterCondition): use HardRetain and not CFRetain
+        (ObjCNodeFilterCondition::~ObjCNodeFilterCondition): use HardRelease and not CFRelease
+        * bridge/mac/AXObjectCacheMac.mm:
+        (WebCore::AXObjectCache::~AXObjectCache): use HardRelease and not CFRelease
+        (WebCore::AXObjectCache::get): use HardRetain and not CFRetain
+        (WebCore::AXObjectCache::remove): use HardRelease and not CFRelease
+        * bridge/mac/WebCoreFrameBridge.mm:
+        (+[WebCoreFrameBridge supportedImageResourceMIMETypes]): use RetainPtr
+        (+[WebCoreFrameBridge supportedImageMIMETypes]): ditto
+        * bridge/mac/WebCoreIconDatabaseBridge.mm:
+        (+[WebCoreIconDatabaseBridge sharedInstance]): use RetainPtr
+        * platform/PopupMenu.h: renamed popup to m_popup and switched to RetainPtr<NSPopUpButtonCell>
+        * platform/cf/RetainPtr.h:
+        (WTF::::operator): implement a missing operator= template function
+        * platform/mac/PopupMenuMac.mm:
+        (WebCore::PopupMenu::PopupMenu): remove nil initialization
+        (WebCore::PopupMenu::~PopupMenu): use .get(), remove release message and rename to m_popup
+        (WebCore::PopupMenu::clear): use .get() when sending a message and rename to m_popup
+        (WebCore::PopupMenu::populate): ditto 
+        (WebCore::PopupMenu::show): ditto
+        (WebCore::PopupMenu::hide): ditto
+        (WebCore::PopupMenu::addSeparator): ditto
+        (WebCore::PopupMenu::addGroupLabel): ditto
+        (WebCore::PopupMenu::addOption): ditto
+
 2006-10-18  Geoffrey Garen  <ggaren@apple.com>
 
         Reviewed by Adele.
index d7a206b..2978999 100644 (file)
@@ -613,12 +613,12 @@ ObjCNodeFilterCondition::ObjCNodeFilterCondition(id <DOMNodeFilter> filter)
     : m_filter(filter)
 {
     ASSERT(m_filter);
-    CFRetain(m_filter);
+    HardRetain(m_filter);
 }
 
 ObjCNodeFilterCondition::~ObjCNodeFilterCondition()
 {
-    CFRelease(m_filter);
+    HardRelease(m_filter);
 }
 
 short ObjCNodeFilterCondition::acceptNode(WebCore::Node* node) const
index e291f41..982a977 100644 (file)
@@ -27,6 +27,7 @@
 #import "AXObjectCache.h"
 
 #import "Document.h"
+#import "FoundationExtras.h"
 #import "RenderObject.h"
 #import "WebCoreAXObject.h"
 #import "WebCoreViewFactory.h"
@@ -50,7 +51,7 @@ AXObjectCache::~AXObjectCache()
     for (HashMap<RenderObject*, WebCoreAXObject*>::iterator it = m_objects.begin(); it != end; ++it) {
         WebCoreAXObject* obj = (*it).second;
         [obj detach];
-        CFRelease(obj);
+        HardRelease(obj);
     }
 }
 
@@ -61,9 +62,8 @@ WebCoreAXObject* AXObjectCache::get(RenderObject* renderer)
         return obj;
 
     obj = [[WebCoreAXObject alloc] initWithRenderer:renderer];
-    CFRetain(obj);
+    HardRetainWithNSRelease(obj);
     m_objects.set(renderer, obj);
-    [obj release];
     return obj;
 }
 
@@ -74,7 +74,7 @@ void AXObjectCache::remove(RenderObject* renderer)
         return;
     WebCoreAXObject* obj = (*it).second;
     [obj detach];
-    CFRelease(obj);
+    HardRelease(obj);
     m_objects.remove(it);
 
     ASSERT(m_objects.size() >= m_idsInUse.size());
index 7a9bd55..46f66db 100644 (file)
@@ -59,6 +59,7 @@
 #import "RenderTreeAsText.h"
 #import "RenderView.h"
 #import "RenderWidget.h"
+#import "RetainPtr.h"
 #import "ReplaceSelectionCommand.h"
 #import "ResourceRequest.h"
 #import "Screen.h"
@@ -418,7 +419,7 @@ static inline WebCoreFrameBridge *bridge(Frame *frame)
 
 + (NSArray *)supportedImageResourceMIMETypes
 {
-    static NSArray* supportedTypes = nil;
+    static RetainPtr<NSArray> supportedTypes;
     if (!supportedTypes) {
         NSMutableSet* set = [[NSMutableSet alloc] init];
 
@@ -439,17 +440,16 @@ static inline WebCoreFrameBridge *bridge(Frame *frame)
         [set removeObject:@"application/octet-stream"];
 
         supportedTypes = [set allObjects];
-        CFRetain(supportedTypes);
 
         [set release];
     }
 
-    return supportedTypes;
+    return supportedTypes.get();
 }
 
 + (NSArray *)supportedImageMIMETypes
 {
-    static NSArray* supportedTypes = nil;
+    static RetainPtr<NSArray> supportedTypes;
     if (!supportedTypes) {
         NSMutableArray* types = [[self supportedImageResourceMIMETypes] mutableCopy];
         [types removeObject:@"application/pdf"];
@@ -458,11 +458,11 @@ static inline WebCoreFrameBridge *bridge(Frame *frame)
         [types release];
 
         supportedTypes = copy;
-        CFRetain(supportedTypes);
 
         [copy release];
     }
-    return supportedTypes;
+
+    return supportedTypes.get();
 }
 
 + (WebCoreFrameBridge *)bridgeForDOMDocument:(DOMDocument *)document
index e13817a..5f6c260 100644 (file)
 #import "config.h"
 #import "WebCoreIconDatabaseBridge.h"
 
-#import "Logging.h"
 #import "IconDatabase.h"
 #import "Image.h"
+#import "Logging.h"
 #import "PlatformString.h"
+#import "RetainPtr.h"
 
 using namespace WebCore;
 
@@ -190,16 +191,10 @@ using namespace WebCore;
 
 + (WebCoreIconDatabaseBridge *)sharedInstance
 {
-    static WebCoreIconDatabaseBridge* bridge = nil;
-
-    if (bridge == nil) {
+    static RetainPtr<WebCoreIconDatabaseBridge> bridge;
+    if (!bridge)
         bridge = [self createInstance];
-        // Need to CFRetain something that's in a global variable, since we want it to
-        // hang around forever, even when running under GC.
-        CFRetain(bridge);
-        [bridge release];
-    }
-    return bridge;
+    return bridge.get();
 }
 
 @end
index 57ca123..fe3ec59 100644 (file)
@@ -25,6 +25,7 @@
 #include <wtf/PassRefPtr.h>
 
 #if PLATFORM(MAC)
+#include "RetainPtr.h"
 #ifdef __OBJC__
 @class NSPopUpButtonCell;
 #else
@@ -85,7 +86,7 @@ protected:
     bool m_wasClicked;
     
 #if PLATFORM(MAC)
-    NSPopUpButtonCell* popup;
+    RetainPtr<NSPopUpButtonCell> m_popup;
 #elif PLATFORM(WIN)
     HWND m_popup;
     HWND m_container;
index ccbc722..ef6613d 100644 (file)
@@ -111,6 +111,17 @@ namespace WTF {
         return *this;
     }
 
+    template <typename T> template <typename U> inline RetainPtr<T>& RetainPtr<T>::operator=(U* optr)
+    {
+        if (optr)
+            CFRetain(optr);
+        PtrType ptr = m_ptr;
+        m_ptr = optr;
+        if (ptr)
+            CFRelease(ptr);
+        return *this;
+    }
+
     template <class T> inline void RetainPtr<T>::swap(RetainPtr<T>& o)
     {
         std::swap(m_ptr, o.m_ptr);
index 3ca7bb1..7c243a2 100644 (file)
@@ -35,54 +35,52 @@ using namespace HTMLNames;
 
 PopupMenu::PopupMenu(RenderMenuList* menuList)
     : m_menuList(menuList)
-    , popup(nil)
 {
 }
 
 PopupMenu::~PopupMenu()
 {
-    if (popup) {
-        [popup setControlView:nil];
-        [popup release];
-    }
+    if (m_popup)
+        [m_popup.get() setControlView:nil];
 }
 
 void PopupMenu::clear()
 {
-    if (popup)
-        [popup removeAllItems];
+    if (m_popup)
+        [m_popup.get() removeAllItems];
 }
 
 void PopupMenu::populate()
 {
-    if (popup)
-        [popup removeAllItems];
+    if (m_popup)
+        [m_popup.get() removeAllItems];
     else {
-        popup = [[NSPopUpButtonCell alloc] initTextCell:@"" pullsDown:NO];
-        [popup setUsesItemFromMenu:NO];
-        [popup setAutoenablesItems:NO];
+        m_popup = [[NSPopUpButtonCell alloc] initTextCell:@"" pullsDown:NO];
+        [m_popup.get() release]; // release here since the RetainPtr has retained the object already
+        [m_popup.get() setUsesItemFromMenu:NO];
+        [m_popup.get() setAutoenablesItems:NO];
     }
-    BOOL messagesEnabled = [[popup menu] menuChangedMessagesEnabled];
-    [[popup menu] setMenuChangedMessagesEnabled:NO];
+    BOOL messagesEnabled = [[m_popup.get() menu] menuChangedMessagesEnabled];
+    [[m_popup.get() menu] setMenuChangedMessagesEnabled:NO];
     PopupMenu::addItems();
-    [[popup menu] setMenuChangedMessagesEnabled:messagesEnabled];
+    [[m_popup.get() menu] setMenuChangedMessagesEnabled:messagesEnabled];
 }
 
 void PopupMenu::show(const IntRect& r, FrameView* v, int index)
 {
     populate();
-    if ([popup numberOfItems] <= 0)
+    if ([m_popup.get() numberOfItems] <= 0)
         return;
-    ASSERT([popup numberOfItems] > index);
+    ASSERT([m_popup.get() numberOfItems] > index);
 
     NSView* view = v->getDocumentView();
 
-    [popup attachPopUpWithFrame:r inView:view];
-    [popup selectItemAtIndex:index];
+    [m_popup.get() attachPopUpWithFrame:r inView:view];
+    [m_popup.get() selectItemAtIndex:index];
     
     NSFont* font = menuList()->style()->font().primaryFont()->getNSFont();
 
-    NSRect titleFrame = [popup titleRectForBounds:r];
+    NSRect titleFrame = [m_popup.get() titleRectForBounds:r];
     if (titleFrame.size.width <= 0 || titleFrame.size.height <= 0)
         titleFrame = r;
     float vertOffset = roundf((NSMaxY(r) - NSMaxY(titleFrame)) + NSHeight(titleFrame));
@@ -91,7 +89,7 @@ void PopupMenu::show(const IntRect& r, FrameView* v, int index)
     vertOffset += [font descender] - [defaultFont descender];
     vertOffset = fmin(NSHeight(r), vertOffset);
 
-    NSMenu* menu = [popup menu];
+    NSMenu* menu = [m_popup.get() menu];
     // FIXME: Need to document where this magic number 10 comes from.
     NSPoint location = NSMakePoint(NSMinX(r) - 10, NSMaxY(r) - vertOffset);
 
@@ -106,7 +104,7 @@ void PopupMenu::show(const IntRect& r, FrameView* v, int index)
     wkPopupMenu(menu, location, roundf(NSWidth(r)), view, index, font);
 
     if (menuList()) {
-        int newIndex = [popup indexOfSelectedItem];
+        int newIndex = [m_popup.get() indexOfSelectedItem];
         menuList()->hidePopup();
 
         if (index != newIndex && newIndex >= 0)
@@ -122,12 +120,12 @@ void PopupMenu::show(const IntRect& r, FrameView* v, int index)
 
 void PopupMenu::hide()
 {
-    [popup dismissPopUp];
+    [m_popup.get() dismissPopUp];
 }
 
 void PopupMenu::addSeparator()
 {
-    [[popup menu] addItem:[NSMenuItem separatorItem]];
+    [[m_popup.get() menu] addItem:[NSMenuItem separatorItem]];
 }
 
 void PopupMenu::addGroupLabel(HTMLOptGroupElement* element)
@@ -145,8 +143,8 @@ void PopupMenu::addGroupLabel(HTMLOptGroupElement* element)
     NSAttributedString* string = [[NSAttributedString alloc] initWithString:text attributes:attributes];
     [attributes release];
 
-    [popup addItemWithTitle:@""];
-    NSMenuItem* menuItem = [popup lastItem];
+    [m_popup.get() addItemWithTitle:@""];
+    NSMenuItem* menuItem = [m_popup.get() lastItem];
     [menuItem setAttributedTitle:string];
     [menuItem setEnabled:NO];
 
@@ -173,8 +171,8 @@ void PopupMenu::addOption(HTMLOptionElement* element)
     NSAttributedString* string = [[NSAttributedString alloc] initWithString:text attributes:attributes];
     [attributes release];
 
-    [popup addItemWithTitle:@""];
-    NSMenuItem* menuItem = [popup lastItem];
+    [m_popup.get() addItemWithTitle:@""];
+    NSMenuItem* menuItem = [m_popup.get() lastItem];
     [menuItem setAttributedTitle:string];
     [menuItem setEnabled:groupEnabled && element->isEnabled()];