Objective-C API: Need a good way to reference event handlers without causing cycles

[Mark Hahnenberg]

Currently, we just have JSValue, which is a strong reference, which risks a cycle if the value directly or indirectly has any pointers back to its owner.

[Mark Hahnenberg]

<rdar://problem/12991581>

[Mark Hahnenberg]

Created attachment 190846 [details]
patch

[Gavin Barraclough]

Comment on attachment 190846 [details]
patch

Per our discussion, please use DEFINE_STATIC_LOCAL thingumy,
maybe add a JSWeakValue parent type
make marking an obj-c wrapper make the obj-c object an opaque root
and change the owner to be the actual obj-c object rather than it's wrapper

cheers, G.

[Mark Hahnenberg]

Created attachment 191334 [details]
patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSAPIWrapperObject.cpp', u'Source/JavaScriptCore/API/JSAPIWrapperObject.h', u'Source/JavaScriptCore/API/JSCallbackObject.cpp', u'Source/JavaScriptCore/API/JSCallbackObject.h', u'Source/JavaScriptCore/API/JSContext.mm', u'Source/JavaScriptCore/API/JSContextInternal.h', u'Source/JavaScriptCore/API/JSManagedValue.h', u'Source/JavaScriptCore/API/JSManagedValue.mm', u'Source/JavaScriptCore/API/JSObjectRef.cpp', u'Source/JavaScriptCore/API/JSValueRef.cpp', u'Source/JavaScriptCore/API/JSVirtualMachine.mm', u'Source/JavaScriptCore/API/JSWeakValue.h', u'Source/JavaScriptCore/API/JSWeakValue.mm', u'Source/JavaScriptCore/API/JSWeakValueInternal.h', u'Source/JavaScriptCore/API/JSWrapperMap.mm', u'Source/JavaScriptCore/API/JavaScriptCore.h', u'Source/JavaScriptCore/API/tests/testapi.mm', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h']" exit_code: 1
Source/JavaScriptCore/API/JSAPIWrapperObject.cpp:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/runtime/JSGlobalObject.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/JSGlobalObject.cpp:233:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/API/JSWeakValueInternal.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 20 files


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

[Mark Hahnenberg]

Created attachment 191337 [details]
style fixes

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSAPIWrapperObject.cpp', u'Source/JavaScriptCore/API/JSAPIWrapperObject.h', u'Source/JavaScriptCore/API/JSCallbackObject.cpp', u'Source/JavaScriptCore/API/JSCallbackObject.h', u'Source/JavaScriptCore/API/JSContext.mm', u'Source/JavaScriptCore/API/JSContextInternal.h', u'Source/JavaScriptCore/API/JSManagedValue.h', u'Source/JavaScriptCore/API/JSManagedValue.mm', u'Source/JavaScriptCore/API/JSObjectRef.cpp', u'Source/JavaScriptCore/API/JSValueRef.cpp', u'Source/JavaScriptCore/API/JSVirtualMachine.mm', u'Source/JavaScriptCore/API/JSWeakValue.h', u'Source/JavaScriptCore/API/JSWeakValue.mm', u'Source/JavaScriptCore/API/JSWeakValueInternal.h', u'Source/JavaScriptCore/API/JSWrapperMap.mm', u'Source/JavaScriptCore/API/JavaScriptCore.h', u'Source/JavaScriptCore/API/tests/testapi.mm', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h']" exit_code: 1
Source/JavaScriptCore/API/JSWeakValueInternal.h:36:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 20 files


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

[Early Warning System Bot]

Comment on attachment 191337 [details]
style fixes

Attachment 191337 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16906069

[Early Warning System Bot]

Comment on attachment 191337 [details]
style fixes

Attachment 191337 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16851547

[Geoffrey Garen]

Comment on attachment 191337 [details]
style fixes

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

Please address comments before landing.

> Source/JavaScriptCore/API/JSAPIWrapperObject.cpp:56
> +    void* objcObject = thisObject->getPrivate();

Let's use a direct data member for the objcObject, and then we don't need the cast to JSCallbackObject.

> Source/JavaScriptCore/API/JSContext.mm:78
>      return self;

Would be nice to call initWithGlobalContextRef instead of duplicating this code. That can be a follow-up patch.

> Source/JavaScriptCore/API/JSContext.mm:271
> +- (void)garbageCollect
> +{
> +    JSC::APIEntryShim entryShim(toJS(m_context));
> +    toJS(m_context)->globalData().heap.collectAllGarbage();
> +}

I'm worried, even though this is SPI with a comment about debugging, that folks will find this symbol and use it to bad effect. Let's link our test code directly against the C++ API, and call that instead.

> Source/JavaScriptCore/API/JSManagedValue.h:39
> +@interface JSManagedValue : JSWeakValue

I'm concerned about adding API surface area here, when we don't know of clients that want that raw weak behavior. Let's leave out JSWeakValue. We can always add it later, as a subclass of JSManagedValue or a sibling class.

> Source/JavaScriptCore/API/JSManagedValue.h:42
> +- (id)initWithValue:(JSValue *)value owner:(id)owner;
> ++ (JSManagedValue *)managedValueWithValue:(JSValue *)value owner:(id)owner;

Class methods should be sorted first.

> Source/JavaScriptCore/API/JSManagedValue.mm:43
> +    virtual ~JSManagedValueHandleOwner();

No need for a dtor here, since we don't do anything it.

> Source/JavaScriptCore/API/JSManagedValue.mm:61
> +- (id)init

Since we don't export API for this, I don't think we should have it. There's no good way for us to test if this works.

> Source/JavaScriptCore/API/JSManagedValue.mm:74
> +    JSC::JSObject* object = toJS(JSValueToObject([value.context globalContextRef], [value JSValueRef], 0));

We don't want to call JSValueToObject here because that will perform a conversion, possibly allocating a new object. Let's call JSValueIsObject instead, and be null if not object.

> Source/JavaScriptCore/API/JSObjectRef.cpp:350
> +    if (jsObject->inherits(&JSCallbackObject<JSAPIWrapperObject>::s_info))
> +        return jsCast<JSCallbackObject<JSAPIWrapperObject>*>(jsObject)->getPrivate();

Ultimately, I think it would be nice to remove the dependency that requires our automatic wrappers to be JSCallbackObjects. That can be a follow-up patch.

> Source/JavaScriptCore/API/JSWeakValue.h:41
> ++ (id)weakValueWithValue:(JSValue *)value;

Class methods should be sorted first.

> Source/JavaScriptCore/API/JSWrapperMap.mm:120
> +static JSObjectRef JSObjectMakeWrapper(JSContextRef ctx, JSClassRef jsClass, void* data)

Let's not use the "JSObject" prefix for this function, since that implies that it belongs to the C API. Let's just call this "makeWrapper".

[Gavin Barraclough]

Comment on attachment 191337 [details]
style fixes

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

r+, with a few fixes.  There is an additional issue to  the comments above, like JSValues we should never create a wrapper around a weak/managed value, we should check in objectToValueWithoutCopy for objects deriving from JSWeakValue & unwrap.

> Source/JavaScriptCore/API/JSManagedValue.mm:71
> +

I think we might also need a "- (id)initWithValue:(JSValue *)value" constructor here –
[[JSManagedValue alloc] initWithValue:value] will compile (since initWithValue: is provided by JSWeakValue), and will result in mismatched init/destroy of m_weakOwner.

> Source/JavaScriptCore/API/JSManagedValue.mm:94
> +    return [[[JSManagedValue alloc] initWithValue:value owner:owner] autorelease];

I think Objective-C style here is to make this [self alloc], to better support subclassing.

> Source/JavaScriptCore/API/JSWeakValue.mm:64
> +    return [[[JSWeakValue alloc] initWithValue:value] autorelease];

I think Objective-C style here is to make this [self alloc], to better support subclassing.

[Build Bot]

Comment on attachment 191337 [details]
style fixes

Attachment 191337 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16795679

[Gavin Barraclough]

(In reply to comment #10)

> > Source/JavaScriptCore/API/JSContext.mm:271
> > +- (void)garbageCollect
> > +{
> > +    JSC::APIEntryShim entryShim(toJS(m_context));
> > +    toJS(m_context)->globalData().heap.collectAllGarbage();
> > +}
> 
> I'm worried, even though this is SPI with a comment about debugging, that folks will find this symbol and use it to bad effect. Let's link our test code directly against the C++ API, and call that instead.

Agreed.

> > Source/JavaScriptCore/API/JSManagedValue.h:39
> > +@interface JSManagedValue : JSWeakValue
> 
> I'm concerned about adding API surface area here, when we don't know of clients that want that raw weak behavior. Let's leave out JSWeakValue. We can always add it later, as a subclass of JSManagedValue or a sibling class.

I'm okay with this, but I think it's really ugly if we leave it that JSManagedValues can be used to make weak values, but that the only API to do so is to init with owner:nil (e.g. "[[JSManagedValue alloc] initWithValue:value owner:nil").  I'd prefer it if we could at least keep the "initWithValue:" constructor.

> > Source/JavaScriptCore/API/JSManagedValue.mm:61
> > +- (id)init
> 
> Since we don't export API for this, I don't think we should have it. There's no good way for us to test if this works.

Yes, we implicitly do export API for this, by subclassing NSObject! :o)
I think we really ought to make this work.

> > Source/JavaScriptCore/API/JSManagedValue.mm:74
> > +    JSC::JSObject* object = toJS(JSValueToObject([value.context globalContextRef], [value JSValueRef], 0));
> 
> We don't want to call JSValueToObject here because that will perform a conversion, possibly allocating a new object. Let's call JSValueIsObject instead, and be null if not object.

Yep, this seems incorrect as is – assigning a value to an weak ref should not change the value.

As devils advocate, we could make it such that JSWeak/ManagedValue hold a regular retaining ref to non-object values.  I guess the underlying question is, do we consider primitives to be referenced by any other value.  I'm guessing the correct answer should be no, in which case Geoff is right insta-null.

[Build Bot]

Comment on attachment 191337 [details]
style fixes

Attachment 191337 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16897109

[Geoffrey Garen]

> > I'm concerned about adding API surface area here, when we don't know of clients that want that raw weak behavior. Let's leave out JSWeakValue. We can always add it later, as a subclass of JSManagedValue or a sibling class.
> 
> I'm okay with this, but I think it's really ugly if we leave it that JSManagedValues can be used to make weak values, but that the only API to do so is to init with owner:nil (e.g. "[[JSManagedValue alloc] initWithValue:value owner:nil").  I'd prefer it if we could at least keep the "initWithValue:" constructor.

So, keep -initWithValue: as a way to make a JSManagedValue that gets auto-nulled? I'm OK with that.

> > > Source/JavaScriptCore/API/JSManagedValue.mm:61
> > > +- (id)init
> > 
> > Since we don't export API for this, I don't think we should have it. There's no good way for us to test if this works.
> 
> Yes, we implicitly do export API for this, by subclassing NSObject! :o)
> I think we really ought to make this work.

Other Cocoa classes don't work this way. For example, there's no NSView -init. Declaring an -init in your class declaration implies that it's reasonable to call it. We don't want people to call -init because we don't have a meaningful way to interpret it.

> As devils advocate, we could make it such that JSWeak/ManagedValue hold a regular retaining ref to non-object values.  I guess the underlying question is, do we consider primitives to be referenced by any other value.  I'm guessing the correct answer should be no, in which case Geoff is right insta-null.

The practical reason to insta-null is that our implementation doesn't have a good way to represent non-object weak references. 

I think you can make a logic argument that non-objects have value semantics in JavaScript, so there's no such thing as an owner "referencing" a primitive value.

[Gavin Barraclough]

(In reply to comment #15)

> So, keep -initWithValue: as a way to make a JSManagedValue that gets auto-nulled? I'm OK with that.

Yes – specifically I'm suggesting "initWithValue:value" be equivalent to "initWithValue:value owner:nil" – given that JSManagedValue has ephemeron-esque semantics, a managed value whose owner has already been nilled out is effectively a weak reference.

> Other Cocoa classes don't work this way. For example, there's no NSView -init. Declaring an -init in your class declaration implies that it's reasonable to call it. We don't want people to call -init because we don't have a meaningful way to interpret it.

NSView is a good example.  It doesn't declare -init in its @interface (since it isn't particularly meaningful), but it *does* provide a definition in its @implementation, so that a call to [[NSView alloc] init] is safe, does work and produces an object in an internally consistent state.

[Geoffrey Garen]

> NSView is a good example.  It doesn't declare -init in its @interface (since it isn't particularly meaningful), but it *does* provide a definition in its @implementation, so that a call to [[NSView alloc] init] is safe, does work and produces an object in an internally consistent state.

OK, let's do that.

[EFL EWS Bot]

Comment on attachment 191337 [details]
style fixes

Attachment 191337 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16990032

[Mark Hahnenberg]

Created attachment 191526 [details]
fixing issues

[Early Warning System Bot]

Comment on attachment 191526 [details]
fixing issues

Attachment 191526 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/16997091

[Early Warning System Bot]

Comment on attachment 191526 [details]
fixing issues

Attachment 191526 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17051230

[Mark Hahnenberg]

Created attachment 191536 [details]
qt build fix

[Early Warning System Bot]

Comment on attachment 191536 [details]
qt build fix

Attachment 191536 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17000225

[Mark Hahnenberg]

Created attachment 191542 [details]
build fix

[Early Warning System Bot]

Comment on attachment 191542 [details]
build fix

Attachment 191542 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17013062

[Mark Hahnenberg]

Created attachment 191553 [details]
moar build fix

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSAPIWrapperObject.cpp', u'Source/JavaScriptCore/API/JSAPIWrapperObject.h', u'Source/JavaScriptCore/API/JSBase.cpp', u'Source/JavaScriptCore/API/JSBasePrivate.h', u'Source/JavaScriptCore/API/JSCallbackObject.cpp', u'Source/JavaScriptCore/API/JSCallbackObject.h', u'Source/JavaScriptCore/API/JSContext.mm', u'Source/JavaScriptCore/API/JSManagedValue.h', u'Source/JavaScriptCore/API/JSManagedValue.mm', u'Source/JavaScriptCore/API/JSObjectRef.cpp', u'Source/JavaScriptCore/API/JSValue.mm', u'Source/JavaScriptCore/API/JSValueRef.cpp', u'Source/JavaScriptCore/API/JSVirtualMachine.mm', u'Source/JavaScriptCore/API/JSWrapperMap.mm', u'Source/JavaScriptCore/API/JavaScriptCore.h', u'Source/JavaScriptCore/API/tests/testapi.mm', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.gypi', u'Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h']" exit_code: 1
Source/JavaScriptCore/API/JSAPIWrapperObject.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 24 files


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

[Mark Hahnenberg]

Created attachment 191555 [details]
style fixes

[Mark Hahnenberg]

Created attachment 191571 [details]
fix JSManagedValue init

We want JSManagedValues to instantly return nil when value is invoked if they're created using plain init or if they're passed a JSValue that isn't an object.

[Geoffrey Garen]

Comment on attachment 191571 [details]
fix JSManagedValue init

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

r=me

> Source/JavaScriptCore/API/JSBase.cpp:114
> +void JSSynchronousGarbageCollect(JSContextRef ctx)

Let's add "ForDebugging" to the end of the name, in case that helps.

> Source/JavaScriptCore/API/JSBasePrivate.h:55
> +/*!
> +@function
> +@abstract Synchronously runs a full mark-sweep garbage collection.
> +@param ctx The execution context to use.
> +@discussion This function is only used internally for tests. If you're not 
> +sure you need to use this function then you don't need to.
> +*/
> +JS_EXPORT void JSSynchronousGarbageCollect(JSContextRef ctx);

Let's not declare this in the header at all. Just put the declaration in testapi.mm. That way, we'll reduce the chances that someone uses it accidentally.

> Source/JavaScriptCore/API/JSManagedValue.h:39
> ++ (JSManagedValue *)weakValueWithValue:(JSValue *)value;

This should be "managedValueWithValue" -- that's the idiomatic ObjC naming scheme.

Not sure that we need this, but I guess it's OK to have it.

> Source/JavaScriptCore/API/JSManagedValue.mm:91
> +        JSC::JSObject* object = toJS(JSValueToObject([value.context globalContextRef], [value JSValueRef], 0));

You can cast to JSObjectRef here instead of calling JSValueToObject(), since you've checked JSValueIsObject().

[Mark Hahnenberg]

Committed r145119: <http://trac.webkit.org/changeset/145119>