After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 697838 - Fix issues with debug assertions turned on
Fix issues with debug assertions turned on
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2013-04-11 22:01 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-05-07 17:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
object: Remove context parameter from peek_js_obj / set_js_obj (4.29 KB, patch)
2013-04-11 22:01 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Remove the use of priv_from_js in the finalizer (6.21 KB, patch)
2013-04-11 22:02 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
Correct the signature of finalizers (19.46 KB, patch)
2013-04-11 22:02 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-04-11 22:01:54 UTC
See patches. The main fix is the second patch, which removes the use of
JS_InstanceOf during finalizers, which is a new restriction in js17. The
third patch cleans up finalizers, and removes the need for wrappers.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-04-11 22:01:58 UTC
Created attachment 241306 [details] [review]
object: Remove context parameter from peek_js_obj / set_js_obj

It's not needed.
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-04-11 22:02:01 UTC
Created attachment 241307 [details] [review]
Remove the use of priv_from_js in the finalizer

In debug mode, JS_InstanceOf checks to make sure that we aren't in
a GC at the current moment, so the finalizer can't typecheck.
Comment 3 Jasper St. Pierre (not reading bugmail) 2013-04-11 22:02:04 UTC
Created attachment 241308 [details] [review]
Correct the signature of finalizers

With the context mostly unused, we can finally correct this.
Additionally, fix up the one or two places that still use a
context.
Comment 4 Ray Strode [halfline] 2013-04-12 14:22:54 UTC
Review of attachment 241306 [details] [review]:

makes sense to me.
Comment 5 Ray Strode [halfline] 2013-04-12 14:30:01 UTC
Review of attachment 241307 [details] [review]:

ok
Comment 6 Ray Strode [halfline] 2013-04-12 14:46:32 UTC
Review of attachment 241308 [details] [review]:

Okay, I'm missing some backstory here and that backstory should really be in the commit message.

Are you saying we've been using the wrong first argument since moz17 ? As I was reading through, I kept expecting to find some sort of JSCLASS_PASS_CONTEXT_TO_FINALIZER flag or something that would get cleared to turn off backward compatibility.  Am I missing it? Can a JSFreeOp be freely cast to a JSContext ?

::: gi/object.c
@@ +1385,3 @@
+        {
+            JSContext *context = JS_NewContext(fop->runtime, 8192);
+            gjs_keep_alive_remove_child(context, priv->keep_alive,

I think we should get rid of the context argument to gjs_keep_alive_remove_child. It only uses it to call JS_BeginRequest/JS_EndRequest.  We obviously not need to use JS_BeginRequest/JS_EndRequest in a finalizer function since it's getting called from the GC (and now we don't even have a context to pass if we wanted to).

It may makes sense to add the JS_BeginRequest/JS_EndRequest around the gjs_keep_alive_remove_child() call in handle_toggle_down.  Not sure.
Comment 7 Jasper St. Pierre (not reading bugmail) 2013-04-12 15:06:26 UTC
Review of attachment 241308 [details] [review]:

Yes, it was just flat out wrong for js17 as displayed by all the compile warnings. No, it cannot be straight out casted, but it seems that in most cases, we just weren't using the context anywhere, so it worked passing the wrong argument type around.
Comment 8 Ray Strode [halfline] 2013-04-12 15:21:08 UTC
crazy
Comment 9 Jasper St. Pierre (not reading bugmail) 2013-05-07 17:01:54 UTC
Attachment 241306 [details] pushed as 33cbec3 - object: Remove context parameter from peek_js_obj / set_js_obj
Attachment 241307 [details] pushed as 7fe64a1 - Remove the use of priv_from_js in the finalizer
Attachment 241308 [details] pushed as 4d8c513 - Correct the signature of finalizers


I'm going to push this for now and do the removal of the context from
keep-alive in another place.