Bug 107839 – Objective-C API: JSObjCClassInfo creates reference cycle with JSContext

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

[Mark Hahnenberg]

JSContext has a JSWrapperMap, which has an NSMutableDictionary m_classMap, which has values that are JSObjCClassInfo objects, which have strong references to two JSValue *'s, m_prototype and m_constructor, which in turn have strong references to the JSContext, creating a reference cycle. We should make m_prototype and m_constructor Weak<JSObject>. This gets rid of the strong reference to the JSContext and also prevents clients from accidentally creating reference cycles by assigning to the prototype of the constructor. If Weak<JSObject> fields are ever garbage collected, we will reallocate them.

[Radar WebKit Bug Importer]

<rdar://problem/13079710>

[Mark Hahnenberg]

Created attachment 185314 [details]
Patch

[WebKit Review Bot]

Attachment 185314 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSContext.mm', u'Source/JavaScriptCore/API/JSContextInternal.h', u'Source/JavaScriptCore/API/JSWrapperMap.mm', u'Source/JavaScriptCore/ChangeLog']" exit_code: 1
Source/JavaScriptCore/API/JSContextInternal.h:70:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 185314 [details]
Patch

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

> Source/JavaScriptCore/API/JSWrapperMap.mm:382
> +    ASSERT(!m_constructor.get());
> +    ASSERT(!m_prototype.get());

Since the prototype's .constructor property is user-modifiable, it's possible for m_prototype to be NULL when m_constructor is not.

[Mark Hahnenberg]

> Since the prototype's .constructor property is user-modifiable, it's possible for m_prototype to be NULL when m_constructor is not.

With the way it's written here, isn't that also true of the constructor's .prototype property?

I guess the fix is to do the check for each and only re-generate the one(s) that have been collected.

[WebKit Review Bot]

Comment on attachment 185314 [details]
Patch

Clearing flags on attachment: 185314

Committed r141176: <http://trac.webkit.org/changeset/141176>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

> With the way it's written here, isn't that also true of the constructor's .prototype property?

Yes, I think so.

> I guess the fix is to do the check for each and only re-generate the one(s) that have been collected.

Sounds right.

[Mark Hahnenberg]

I'm reopening since those ASSERTs are wrong, along with some of the reallocation logic they're based on.

[Mark Hahnenberg]

Created attachment 185338 [details]
Patch

[Geoffrey Garen]

Comment on attachment 185338 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 185338 [details]
Patch

Clearing flags on attachment: 185338

Committed r141199: <http://trac.webkit.org/changeset/141199>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.