Crash in FrameLoader::stopAllLoaders via [WebView dealloc] inside ~ObjCEventListener

• Component: WebKit API
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Some WebKit clients crash in FrameLoader::stopAllLoaders via [WebView dealloc] inside ~ObjCEventListener while deleting event targets.

<rdar://problem/40888424>

[Ryosuke Niwa]

Created attachment 367769 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 367770 [details]
Fixes the bug

[Darin Adler]

Comment on attachment 367770 [details]
Fixes the bug

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

> Source/WebKitLegacy/mac/DOM/ObjCEventListener.mm:75
> +    // Avoid executing arbitrary code during GC; e.g. inside Node::~Node.
> +    [m_listener.get() retain];
> +    [m_listener.get() autorelease];

Some day we will move to ARC. To be prepared for when we do, it will be much more future-proof to use CFRetain and CFAutorelease, which can be used for this purpose even under ARC. I suggest doing that.

    CFRetain((__bridge CFTypeRef)m_listener.get());
    CFAutorelease((__bridge CFTypeRef)m_listener.get());

To make it cleaner, if this idiom comes up more often, we could add a RetainPtr member function designed for this purpose. Unfortunately, the current RetainPtr::autorelease is not what we want because it compiles to a bridging release under ARC, not an autorelease.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #3)
> Comment on attachment 367770 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367770&action=review
> 
> > Source/WebKitLegacy/mac/DOM/ObjCEventListener.mm:75
> > +    // Avoid executing arbitrary code during GC; e.g. inside Node::~Node.
> > +    [m_listener.get() retain];
> > +    [m_listener.get() autorelease];
> 
> Some day we will move to ARC. To be prepared for when we do, it will be much
> more future-proof to use CFRetain and CFAutorelease, which can be used for
> this purpose even under ARC. I suggest doing that.
> 
>     CFRetain((__bridge CFTypeRef)m_listener.get());
>     CFAutorelease((__bridge CFTypeRef)m_listener.get());
> 
> To make it cleaner, if this idiom comes up more often, we could add a
> RetainPtr member function designed for this purpose. Unfortunately, the
> current RetainPtr::autorelease is not what we want because it compiles to a
> bridging release under ARC, not an autorelease.

Okay, sure, I'll use CF*.

[Ryosuke Niwa]

Committed r244459: <https://trac.webkit.org/changeset/244459>