Bug 46220 - Fix some Objective-C GC problems and use RetainPtr instead of HardRetain/Release
: Fix some Objective-C GC problems and use RetainPtr instead of HardRetain/Release
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Misc.
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Darin Adler
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-21 14:56 PDT by Darin Adler
Modified: 2010-09-22 10:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch (39.22 KB, patch)
2010-09-21 16:00 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (39.21 KB, patch)
2010-09-21 17:04 PDT, Darin Adler
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2010-09-21 14:56:45 PDT
Fix some Objective-C GC problems and use RetainPtr instead of HardRetain/Release
Comment 1 Darin Adler 2010-09-21 16:00:05 PDT
Created attachment 68299 [details]
Patch
Comment 2 Early Warning System Bot 2010-09-21 16:10:49 PDT
Attachment 68299 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3996095
Comment 3 WebKit Review Bot 2010-09-21 16:27:09 PDT
Attachment 68299 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3996096
Comment 4 WebKit Review Bot 2010-09-21 17:02:10 PDT
Attachment 68299 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4081046
Comment 5 Darin Adler 2010-09-21 17:04:58 PDT
Created attachment 68312 [details]
Patch
Comment 6 Anders Carlsson 2010-09-22 09:43:39 PDT
Comment on attachment 68312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68312&action=review

> WebCore/bindings/objc/ObjCEventListener.h:59
> +        typedef id <DOMEventListener> ObjCListener;

There shouldn't be a space after id here.

> WebCore/bridge/objc/objc_instance.mm:88
> +    s_exception = nil;

How will not retaining here work in a non-GC environment?
Comment 7 Darin Adler 2010-09-22 09:48:44 PDT
(In reply to comment #6)
> > WebCore/bridge/objc/objc_instance.mm:88
> > +    s_exception = nil;
> 
> How will not retaining here work in a non-GC environment?

This is a global in an Objective-C++ source file; the compiler and garbage collection runtime already handles this correctly without explicit retain/release. We have many other globals like this. An earlier version of my patch attempted to “fix” all those, but then I talked to Tim Hatcher, read some documentation, and did a little testing, and saw this worked fine.

In fact, the problem in ThemeMac.mm quite possibly is Leopard-only because it seems that the SnowLeopard version of GC can handle modifying an Objective-C global through a pointer even though the Leopard version cannot. But the fix is OK for all platforms.
Comment 8 Darin Adler 2010-09-22 09:50:52 PDT
(In reply to comment #6)
> > WebCore/bindings/objc/ObjCEventListener.h:59
> > +        typedef id <DOMEventListener> ObjCListener;
> 
> There shouldn't be a space after id here.

I did a quick grep and I see that we have id without a space in 6 places in WebCore and id with a space in about 50 places, including public headers.

I think we should standardize on one or the other, but for this patch I will leave this site alone -- it matches what used to be there.
Comment 9 Darin Adler 2010-09-22 09:55:51 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > > WebCore/bridge/objc/objc_instance.mm:88
> > > +    s_exception = nil;
> > 
> > How will not retaining here work in a non-GC environment?
> 
> This is a global in an Objective-C++ source file; the compiler and garbage collection runtime already handles this correctly without explicit retain/release. We have many other globals like this. An earlier version of my patch attempted to “fix” all those, but then I talked to Tim Hatcher, read some documentation, and did a little testing, and saw this worked fine.
> 
> In fact, the problem in ThemeMac.mm quite possibly is Leopard-only because it seems that the SnowLeopard version of GC can handle modifying an Objective-C global through a pointer even though the Leopard version cannot. But the fix is OK for all platforms.

Oops! You said “non-GC”!

That change was bad. I have to retain/release. Fixed now.
Comment 10 Darin Adler 2010-09-22 10:26:33 PDT
Committed r68054: <http://trac.webkit.org/changeset/68054>