Bug 111088

Summary: Objective-C API: Need a good way to reference event handlers without causing cycles
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, buildbot, ggaren, gyuyoung.kim, philn, rakuco, rego+ews, rniwa, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
barraclough: review-
patch
none
style fixes
ggaren: review+, webkit-ews: commit-queue-
fixing issues
webkit-ews: commit-queue-
qt build fix
webkit-ews: commit-queue-
build fix
webkit-ews: commit-queue-
moar build fix
none
style fixes
none
fix JSManagedValue init ggaren: review+

Description Mark Hahnenberg 2013-02-28 10:24:00 PST
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.
Comment 1 Mark Hahnenberg 2013-02-28 10:24:15 PST
<rdar://problem/12991581>
Comment 2 Mark Hahnenberg 2013-02-28 16:46:08 PST
Created attachment 190846 [details]
patch
Comment 3 Gavin Barraclough 2013-03-01 13:35:35 PST
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.
Comment 4 Mark Hahnenberg 2013-03-04 16:07:08 PST
Created attachment 191334 [details]
patch
Comment 5 WebKit Review Bot 2013-03-04 16:09:31 PST
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.
Comment 6 Mark Hahnenberg 2013-03-04 16:12:23 PST
Created attachment 191337 [details]
style fixes
Comment 7 WebKit Review Bot 2013-03-04 16:14:01 PST
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.
Comment 8 Early Warning System Bot 2013-03-04 16:22:56 PST
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
Comment 9 Early Warning System Bot 2013-03-04 16:28:09 PST
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
Comment 10 Geoffrey Garen 2013-03-04 16:56:53 PST
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".
Comment 11 Gavin Barraclough 2013-03-04 16:59:36 PST
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.
Comment 12 Build Bot 2013-03-04 17:06:26 PST
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
Comment 13 Gavin Barraclough 2013-03-04 17:19:23 PST
(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.
Comment 14 Build Bot 2013-03-04 17:33:51 PST
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
Comment 15 Geoffrey Garen 2013-03-04 17:39:12 PST
> > 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.
Comment 16 Gavin Barraclough 2013-03-04 18:06:35 PST
(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.
Comment 17 Geoffrey Garen 2013-03-04 18:10:35 PST
> 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.
Comment 18 EFL EWS Bot 2013-03-04 20:39:27 PST
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
Comment 19 Mark Hahnenberg 2013-03-05 11:50:42 PST
Created attachment 191526 [details]
fixing issues
Comment 20 Early Warning System Bot 2013-03-05 12:13:57 PST
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
Comment 21 Early Warning System Bot 2013-03-05 12:34:31 PST
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
Comment 22 Mark Hahnenberg 2013-03-05 12:38:40 PST
Created attachment 191536 [details]
qt build fix
Comment 23 Early Warning System Bot 2013-03-05 12:52:41 PST
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
Comment 24 Mark Hahnenberg 2013-03-05 12:53:05 PST
Created attachment 191542 [details]
build fix
Comment 25 Early Warning System Bot 2013-03-05 13:13:41 PST
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
Comment 26 Mark Hahnenberg 2013-03-05 13:37:54 PST
Created attachment 191553 [details]
moar build fix
Comment 27 WebKit Review Bot 2013-03-05 13:40:15 PST
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.
Comment 28 Mark Hahnenberg 2013-03-05 13:47:28 PST
Created attachment 191555 [details]
style fixes
Comment 29 Mark Hahnenberg 2013-03-05 14:48:55 PST
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.
Comment 30 Geoffrey Garen 2013-03-06 15:56:20 PST
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().
Comment 31 Mark Hahnenberg 2013-03-07 12:45:24 PST
Committed r145119: <http://trac.webkit.org/changeset/145119>