Bug 184041 – [GLIB] Add JSCWeakValue to JavaScriptCore GLib API

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

[Carlos Garcia Campos]

To be able to keep references to JavaScript values without protecting them.

[Carlos Garcia Campos]

Created attachment 336585 [details]
Patch

[Yusuke Suzuki]

Why not porting and calling it as JSManagedValue?

[Michael Catanzaro]

Comment on attachment 336585 [details]
Patch

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

> Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:193
> +        g_signal_emit(weakValue, signals[CLEARED], 0, nullptr);

I'm hesitant about this signal. Wouldn't it be better for the user to pass in a GWeakNotify to be called instead of using a signal? The signal makes it hard to use in performance-sensitive code.

I even thought about suggesting that JSCWeakValue be a GBoxed or even an opaque struct instead of a GObject, but I suppose since JSCValue is a GObject it's better to parallel that.

> Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:260
> +            static_cast<GParamFlags>(WEBKIT_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)));

Might want to add a warning comment as to why it must never be made readable.

> Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:299
> + * Returns: (transfer full): a new #JSCValue or %NULL if @weak_value was cleared.

I was worried about the JSCValue being destroyed on the GC thread, but I suppose that cannot ever happen because you take a lock here and return a strong ref. That's the main thing I was looking to see in this review. Good.

> Source/JavaScriptCore/API/glib/JSCWeakValue.h:54
> +    void (*_jsc_reserved0) (void);

It has a signal, why not expose it here?

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #2)
> Why not porting and calling it as JSManagedValue?

I don't understand Yusuke's question, but please satisfy him before committing, of course. :)

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #4)
> (In reply to Yusuke Suzuki from comment #2)
> > Why not porting and calling it as JSManagedValue?
> 
> I don't understand Yusuke's question, but please satisfy him before
> committing, of course. :)

https://developer.apple.com/documentation/javascriptcore/jsmanagedvalue
Objective-C API has JSManagedValue, which is a bit extended version of this JSCWeakValue.
It is basically weak JSValue holder, in addition, it has a special mechanism to tell Objective-C graph to JSC's Garbage Collector by using JSVirtualMachine's API.

[Carlos Garcia Campos]

(In reply to Yusuke Suzuki from comment #2)
> Why not porting and calling it as JSManagedValue?

This is basically the same as JSManagedValue, but without the owners part. I prefer to use Weak rather tahn Managed, because I think it's more familiar for GLib APIs. I plan to add the owners part on top of this, it's a matter of adding new_with_owner() and add all the virtual machine part, of course.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #3)
> Comment on attachment 336585 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336585&action=review
> 
> > Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:193
> > +        g_signal_emit(weakValue, signals[CLEARED], 0, nullptr);
> 
> I'm hesitant about this signal. Wouldn't it be better for the user to pass
> in a GWeakNotify to be called instead of using a signal? The signal makes it
> hard to use in performance-sensitive code.

This is not a weak reference to itself like GWeakNotify. GWeakNotify receives a GObject, what do we pass there? we don't really keep the JSCValue, so we don't have anything to pass there. The JSCWeakValue is still alive, so it doesn't make sense to pass the weak value either.

> I even thought about suggesting that JSCWeakValue be a GBoxed or even an
> opaque struct instead of a GObject, but I suppose since JSCValue is a
> GObject it's better to parallel that.

What's the point? The main advantage of being a GObject is that we can receive the JSCValue as a write-only construct-only parameter.

> > Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:260
> > +            static_cast<GParamFlags>(WEBKIT_PARAM_WRITABLE | G_PARAM_CONSTRUCT_ONLY)));
> 
> Might want to add a warning comment as to why it must never be made readable.
> 
> > Source/JavaScriptCore/API/glib/JSCWeakValue.cpp:299
> > + * Returns: (transfer full): a new #JSCValue or %NULL if @weak_value was cleared.
> 
> I was worried about the JSCValue being destroyed on the GC thread, but I
> suppose that cannot ever happen because you take a lock here and return a
> strong ref. That's the main thing I was looking to see in this review. Good.
> 
> > Source/JavaScriptCore/API/glib/JSCWeakValue.h:54
> > +    void (*_jsc_reserved0) (void);
> 
> It has a signal, why not expose it here?

The signal is just a notification, we don't have a default impl, and I don't expect subclasses need to override it at all. Actually I don't expect subclasses at all, but I don't want to limit it for no reason.

[Carlos Garcia Campos]

(In reply to Yusuke Suzuki from comment #5)
> (In reply to Michael Catanzaro from comment #4)
> > (In reply to Yusuke Suzuki from comment #2)
> > > Why not porting and calling it as JSManagedValue?
> > 
> > I don't understand Yusuke's question, but please satisfy him before
> > committing, of course. :)
> 
> https://developer.apple.com/documentation/javascriptcore/jsmanagedvalue
> Objective-C API has JSManagedValue, which is a bit extended version of this
> JSCWeakValue.
> It is basically weak JSValue holder, in addition, it has a special mechanism
> to tell Objective-C graph to JSC's Garbage Collector by using
> JSVirtualMachine's API.

Yes, I'm just doing the same in two steps. I still need to understand how the owners thing work in a real use case to decide how to expose it in the API.

[Carlos Garcia Campos]

Committed r230024: <https://trac.webkit.org/changeset/230024>

[Radar WebKit Bug Importer]

<rdar://problem/38944365>