GNOME Bugzilla – Bug 697838
Fix issues with debug assertions turned on
Last modified: 2013-05-07 17:02:02 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.
Created attachment 241306 [details] [review] object: Remove context parameter from peek_js_obj / set_js_obj It's not needed.
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.
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.
Review of attachment 241306 [details] [review]: makes sense to me.
Review of attachment 241307 [details] [review]: ok
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.
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.
crazy
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.