Bug 107778 – Objective-C API: JSContext exception property causes reference cycle

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

JSContext has a (retain) JSValue * exception property which, when non-null, creates a reference cycle (since the JSValue * holds a strong reference back to the JSContext *).

[Radar WebKit Bug Importer]

<rdar://problem/13079694>

[Mark Hahnenberg]

Created attachment 185587 [details]
Patch

[Geoffrey Garen]

Comment on attachment 185587 [details]
Patch

Do we need to to something to keep the exception value from being garbage collected?

[Gavin Barraclough]

(In reply to comment #3)
> (From update of attachment 185587 [details])
> Do we need to to something to keep the exception value from being garbage collected?

Oh, I think you're right.

[Gavin Barraclough]

Comment on attachment 185587 [details]
Patch

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

r- for protect issue Geoff identified.

> Source/JavaScriptCore/API/JSContext.mm:93
> +        m_exception = nil;

I think we only use nil for Objective-C pointer types; this should probably by 0.

[Mark Hahnenberg]

Created attachment 185638 [details]
Patch

[Oliver Hunt]

Comment on attachment 185638 [details]
Patch

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

> Source/JavaScriptCore/API/JSContext.mm:45
> +    JSValueRef m_exception;

Make this Strong<> or ProtectedPtr<>

> Source/JavaScriptCore/API/JSContext.mm:100
> +    if (m_exception)
> +        JSValueUnprotect(m_context, m_exception);
> +
> +    if (value) {
> +        m_exception = valueInternalValue(value);
> +        JSValueProtect(m_context, m_exception);
> +    } else
> +        m_exception = 0;

This becomes much simpler if m_exception is simply a Strong or ProtectedPtr -- I assume we'd prefer Strong<> these days but maybe i'm wrong?

> Source/JavaScriptCore/API/JSContext.mm:107
> +    if (m_exception)
> +        return [JSValue valueWithValue:m_exception inContext:self];
> +     return nil;

Formatting horror!

[Geoffrey Garen]

Strong is a bit more efficient than ProtectedPtr.

[Mark Hahnenberg]

> > Source/JavaScriptCore/API/JSContext.mm:107
> > +    if (m_exception)
> > +        return [JSValue valueWithValue:m_exception inContext:self];
> > +     return nil;
> 
> Formatting horror!

Oh my.

[Mark Hahnenberg]

Created attachment 185827 [details]
Patch

[Mark Hahnenberg]

In JSContext.h, the exception property is marked with the "retain" attribute. I'm not sure whether this is correct or not, since we do "retain" the internal JavaScript value, but we don't retain that particular JSValue that is passed into setException.

[Darin Adler]

Comment on attachment 185827 [details]
Patch

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

> Source/JavaScriptCore/API/JSContext.mm:99
> +    if (m_exception)
> +        return [JSValue valueWithValue:toRef(m_exception.get()) inContext:self];
> +    return nil;

Normally WebKit style is to use early return for the exceptional cases, so the return nil would come first.

Is the special case for nil needed? Does valueWithValue:inContext: handle null properly? Do we need to optimize the null case?

[Mark Hahnenberg]

(In reply to comment #12)
> (From update of attachment 185827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185827&action=review
> 
> > Source/JavaScriptCore/API/JSContext.mm:99
> > +    if (m_exception)
> > +        return [JSValue valueWithValue:toRef(m_exception.get()) inContext:self];
> > +    return nil;
> 
> Normally WebKit style is to use early return for the exceptional cases, so the return nil would come first.
> 
> Is the special case for nil needed? Does valueWithValue:inContext: handle null properly? Do we need to optimize the null case?

valueWithValue:inContext: calls initWithValue:inContext: which has an ASSERT(value), so it doesn't do the right thing here.

[Mark Hahnenberg]

Landed in http://trac.webkit.org/changeset/141490.