GNOME Bugzilla – Bug 632159
adapt to removal of SlowNative functions
Last modified: 2010-11-12 23:21:19 UTC
Created attachment 172367 [details] [review] Git commit format-patch that replace all SlowNatives by FastNatives xulrunner upstream decide to remove SlowNatives JS calls, and to use only FastNatives ones. The related bug from Mozilla: https://bugzilla.mozilla.org/show_bug.cgi?id=581263 The Mercurial revision of the removal http://hg.mozilla.org/tracemonkey/rev/66c8ad02543b
Comment on attachment 172367 [details] [review] Git commit format-patch that replace all SlowNatives by FastNatives The patch allow GJS to compile and run quite correctly, it seems. But Gnome-Shell fails to start.
Created attachment 172373 [details] [review] Tweaks to allow compiling on xulrunner 1.9.x
An important concern here is ensuring we continue to build and check on xulrunner 1.9.2 (i.e. whatever is in Fedora 14 at least, and preferably 13).
I'm on a better patch with some "cleaning" of GJS code, and quite transparent compatibility with xulrunner 1.9.2
Created attachment 172625 [details] [review] Patch with kind of massive changes (and macros to do so) Might work as well on xulrunner 1.9.2 and 2.0, so segfaulting at the moment.
Created attachment 172629 [details] [review] Cleaner patch to adapt the code (remove some ugly debug forgotten code)
Created attachment 172638 [details] [review] Oops, typo, now compile
Review of attachment 172638 [details] [review]: A few comments; not a full review yet. ::: gi/object.c @@ +1088,3 @@ } + jsval rval = JSVAL_VOID; Avoid C99 declarations for now, please. ::: gjs/compat.h @@ +78,3 @@ + +#define GJS_NATIVE_CONSTRUCTOR_BEGIN(x) JSBool \ +gjs_##x##_constructor(JSContext *context, \ I'd prefer we take the full symbol name here and not do a hidden transform; the less magic in macros the better. So: GJS_NATIVE_CONSTRUCTOR_BEGIN(boxed_constructor) and not: GJS_NATIVE_CONSTRUCTOR_BEGIN(boxed) ::: gjs/jsapi-util.h @@ +227,3 @@ JSPropertySpec *static_ps, JSFunctionSpec *static_fs); +#ifndef HAVE_JS_ADDVALUEROOT This won't work because compat.h isn't installed, but jsapi-util.h is. (I'd actually like to make only context.h public, but we aren't there yet) Instead, let's make gjs_check_constructing always take jsval *vp, but document it must be NULL on earlier SpiderMonkey?
Created attachment 172672 [details] [review] New patch with apply of some comments, discuss about the last not applied (In reply to comment #8) > Review of attachment 172638 [details] [review]: > > A few comments; not a full review yet. > > ::: gi/object.c > @@ +1088,3 @@ > } > > + jsval rval = JSVAL_VOID; > > Avoid C99 declarations for now, please. Ok, done > ::: gjs/jsapi-util.h > @@ +227,3 @@ > JSPropertySpec *static_ps, > JSFunctionSpec *static_fs); > +#ifndef HAVE_JS_ADDVALUEROOT > > This won't work because compat.h isn't installed, but jsapi-util.h is. > > (I'd actually like to make only context.h public, but we aren't there yet) > > Instead, let's make gjs_check_constructing always take jsval *vp, but document > it must be NULL on earlier SpiderMonkey? Good idea, done. > ::: gjs/compat.h > @@ +78,3 @@ > + > +#define GJS_NATIVE_CONSTRUCTOR_BEGIN(x) JSBool \ > +gjs_##x##_constructor(JSContext *context, \ > > I'd prefer we take the full symbol name here and not do a hidden transform; the > less magic in macros the better. > > So: > > GJS_NATIVE_CONSTRUCTOR_BEGIN(boxed_constructor) > > and not: > > GJS_NATIVE_CONSTRUCTOR_BEGIN(boxed) There is some kind of magic in GJS_DEFINE_PROTO or other macro, and like mine, these use the given name for multiple use : gjs_x_constructor use the gjs_x_class to create the new object (to avoid doing this each time manually). So the only alternative I see might be GJS_NATIVE_CONSTRUCTOR_BEGIN(boxed_constructor, gjs_boxed_class) (or without the gjs_ prefix for class and let it use on contructor as I had to modify fox now)
Created attachment 172682 [details] [review] Same patch but with the JS_SealObject replacement You must apply this patch first: https://bugzilla.gnome.org/show_bug.cgi?id=632529
Okay; one thing to notice is that I think we can much more easily be compatible with both if we port the current code to use JS_FAST_NATIVE; that exists even in 1.9.2. Then this patch can simply stop using that flag #ifdef HAVE_MOZJS_2 (or #define it to 0). Look at for example: http://hg.mozilla.org/mozilla-central/diff/66c8ad02543b/js/src/jsdate.cpp
Review of attachment 172682 [details] [review]: ::: gjs/compat.h @@ +75,3 @@ +{ \ + JSObject *obj = JS_THIS_OBJECT(context, vp); \ + jsval *argv = JS_ARGV(context, vp); Rather than macroify this, I'd rather do step 0 which is porting all of the JSNative to JSFastNative. This is conceptually an independent step from porting to fast constructors.
Review of attachment 172682 [details] [review]: ::: modules/dbus.c @@ +373,3 @@ + GJS_SET_RVAL(JSVAL_VOID); + Also, we don't need to set the return value if we're throwing. All of the gjs code does this, I know, but we don't need to.
Created attachment 172880 [details] [review] Port all functions to JSFUN_FAST_NATIVE "slow" natives have been removed in mozjs; this prepares us for a patch which adapts to their removal. See upstream commit: http://hg.mozilla.org/mozilla-central/rev/66c8ad02543b
Comment on attachment 172880 [details] [review] Port all functions to JSFUN_FAST_NATIVE Attachment 172880 [details] pushed as c78646e - Port all functions to JSFUN_FAST_NATIVE
I've created a new gjs branch, wip/xulrunner-2 with some new patches: http://git.gnome.org/browse/gjs/log/?h=wip/xulrunner-2 The current status is that it compiles and works with the mozjs in F15 now (2.0b6) as well as earlier (1.9.3). It does segfault very early on mozjs HG tip though, which I'm debugging now.
Created attachment 173501 [details] [review] Bump required glib to 2.18 The verison of g-i we require itself needs 2.24. Now, I'm not actually going to claim we work with 2.18, but doing this allows me to drop the glib stuff from compat.h at least.
Created attachment 173502 [details] [review] Adapt to JS_GetFrameThis API change See upstraem commit 38cbd4e02afc. We can detect the difference by checking for JS_EndPC, which was introduced around the same time.
Created attachment 173503 [details] [review] Switch from JS_ARGV_CALLEE to JS_CALLEE
Created attachment 173504 [details] [review] boxed: Avoid recursion and conversion during construction boxed_new previously called gjs_invoke_c_function_uncached() on the first C constructor it found; this recursed and we would wrap the boxed, creating a JSObject etc., only to unwrap it and get the native pointer again. For a reason I'm not going to debug in depth, this started failing with xulrunner 2. Regardless, we should avoid this insanity and call the C function more directly with g_function_info_invoke, which gives us the raw pointer we wanted anyways.
Created attachment 173505 [details] [review] Handle JS_InitClass throwing an error more cleanly
Created attachment 173506 [details] [review] compat.h: Don't include config.h, instead do inspection of jsapi.h This allows us to sanely install this header file, which will allow us to make jsapi-util.h depend on it.
Created attachment 173507 [details] [review] Use fast constructors if available "slow" natives were removed in the mozjs commit: http://hg.mozilla.org/mozilla-central/rev/66c8ad02543b In order to work with both, add compatibility macros to compat.h, and also update the ones in jsapi-util.h to use these. Port all constructors to use these macros.
Created attachment 173508 [details] [review] Port more custom functions to be "fast" natives These were missed in the big conversion to fast natives in c78646ed3f24bd915c7cfe4aca
This patch series builds and "make check"s on Spidermonkey hg tip as of: changeset: 56650:16f18e41dcf3 tag: tip user: Patrick McManus <mcmanus@ducksong.com> date: Thu Oct 28 10:10:03 2010 -0700 as well as xulrunner-1.9.2.9-1.el6.x86_64 from RHEL6.
I plan to commit this series tomorrow; review would be nice before then.
Review of attachment 173508 [details] [review]: Just looks fine.
Sorry, was a little laggy…
Review of attachment 173501 [details] [review]: Looks fine.
Review of attachment 173502 [details] [review]: See upstraem commit 38cbd4e02afc. We can detect the difference by checking for JS_EndPC, which was introduced around the same time. Typo: 'upstraem'. Blech. But yeah, adding a AC_TRY_COMPILE() would be overkill. ::: gjs/stack.c @@ +121,3 @@ } + /* commit 38cbd4e02afc */ Definitely need to say 'mozilla-central commit .. ' or something @@ +126,3 @@ + jsval thisval; + if (JS_GetFrameThis(cx, fp, &thisval)) + this_obj = JSVAL_TO_OBJECT(thisval); The upstream commit was: If this is evaluated within strict mode code, then the this value is not coerced to an object. So I assume that assuming that thisval is always an OBJECT is wrong? It's probably fine to just set this_obj = NULL in the case it isn't an object and not try to format it. Or wait, this_obj is completely unused afterbeing assigned to.
Review of attachment 173503 [details] [review]: Looks fine
Review of attachment 173504 [details] [review]: ::: gi/boxed.c @@ +246,3 @@ g_base_info_unref((GIBaseInfo*) func_info); + priv->gboxed = g_boxed_copy (gtype, rval.v_pointer); Appears to leak the returned object. Maybe you just wanted priv->gboxed = rval.v_pointer ?
Review of attachment 173505 [details] [review]: Mostly looks fine, but: ::: gi/boxed.c @@ -427,3 @@ GJS_INC_COUNTER(boxed); - g_assert(priv_from_js(context, obj) == NULL); What does this have to do with the patch?
Review of attachment 173506 [details] [review]: I yield :-). If you feel this is the right way to go, go ahead.
Review of attachment 173507 [details] [review]: This looks solid to me. ::: gjs/compat.h @@ +86,3 @@ + * be at the very top. + */ +#define GJS_NATIVE_CONSTRUCTOR_VARIABLES(name) \ Including the name argument in the latter 3 macros but not using it seems like an invitation to cut-and-paste errors. Do you see a reason you'd want to use it? @@ +87,3 @@ + */ +#define GJS_NATIVE_CONSTRUCTOR_VARIABLES(name) \ + JSObject *object = NULL; \ Using 'object' here will break including jsapi-util.h in a Objective C program. Hmm. OK, I've moved on :-) ::: gjs/jsapi-util.h @@ +115,3 @@ #define GJS_DEFINE_PROTO(tn, cn) \ +GJS_NATIVE_CONSTRUCTOR_DECLARE(cn); \ +_GJS_DEFINE_PROTO_FULL(tn, cn, gjs_##cn##_constructor) Adding the gjs_ here seems like a bit of a random change to sneak in - nothing to do with this patch - and also maybe questionable in an installed header file that could be used by a third party? Seems like the caller should supply the namespace? (if one is needed - since everything is static, a namespace isn't really needed. I think the reason that the cairo stuff is using gjs_cairo is because while a namespace isn't needed, defining stuff in the *cairo* namespace could result in header file clashes and is ugly) But not a big deal.
Review of attachment 173508 [details] [review]: Found one typo here, and a question about functions with "no" return value. ::: gjs/context.c @@ +94,2 @@ static JSBool gjs_log(JSContext *context, See question below in dbus.c about maybe needing to mandatorily call JS_SET_RVAL from successful returns (was reviewing this patch backwards) ::: modules/console.c @@ +163,2 @@ { + JSObject *object = JS_THIS_OBJECT(context, vp); Sort of weird you changed this one to object while keeping the rest as obj in this patch :-) ::: modules/dbus.c @@ +365,3 @@ /* Args are bus_name, object_path, iface, method, out signature, in signature, args, and callback to get returned value */ static JSBool gjs_js_dbus_call_async(JSContext *context, http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/91ee3f1f5642e05b?pli=1 seems to apply that calling JS_SET_RVAL may be mandatory even if you want to return 'undefined'. Comment applies to a bunch of functions in this file. ::: modules/gettext-native.c @@ +33,2 @@ static JSBool gjs_textdomain(JSContext *context, Some more functions here without a return value @@ +140,3 @@ + result = gjs_string_from_utf8(context, translated, -1, &retval); + if (retval) I think this should be if (result) ::: modules/mainloop.c @@ +38,2 @@ static JSBool gjs_main_loop_quit(JSContext *context, And more without a return value in here
(In reply to comment #33) > It's > probably fine to just set this_obj = NULL in the case it isn't an object and > not try to format it. Or wait, this_obj is completely unused afterbeing > assigned to. Oh true, but we should be printing it. I'll do that as a separate patch.
(In reply to comment #38) > Review of attachment 173507 [details] [review]: > > This looks solid to me. > > ::: gjs/compat.h > @@ +86,3 @@ > + * be at the very top. > + */ > +#define GJS_NATIVE_CONSTRUCTOR_VARIABLES(name) \ > > Including the name argument in the latter 3 macros but not using it seems like > an invitation to cut-and-paste errors. Do you see a reason you'd want to use > it? For visual consistency, and in case we did want to require it later. I'm not concerned about c&p; if an error arises if we start requiring it later, callers can just fix it. > Adding the gjs_ here seems like a bit of a random change to sneak in - nothing > to do with this patch - and also maybe questionable in an installed header file > that could be used by a third party? So, on my agenda actually is to remove the ability to do custom third party modules (and fold g-i into the core, instead of being a module). It would have been saner e.g. to do gettext as a "hidden" g-i library, and a js override file. This means jsapi-util.h would stop being public.
(In reply to comment #39) > > See question below in dbus.c about maybe needing to mandatorily call > JS_SET_RVAL from successful returns (was reviewing this patch backwards) Hmm, you're right. Crap. Fixed.
Attachment 173501 [details] pushed as 32ea296 - Bump required glib to 2.18 Attachment 173502 [details] pushed as 4e4cd97 - Adapt to JS_GetFrameThis API change Attachment 173503 [details] pushed as 7c8546a - Switch from JS_ARGV_CALLEE to JS_CALLEE Attachment 173504 [details] pushed as 0854932 - boxed: Avoid recursion and conversion during construction Attachment 173505 [details] pushed as 24687a7 - Handle JS_InitClass throwing an error more cleanly Attachment 173506 [details] pushed as 50014a0 - compat.h: Don't include config.h, instead do inspection of jsapi.h Attachment 173507 [details] pushed as 7ae2fa5 - Use fast constructors if available Attachment 173508 [details] pushed as ec97bfc - Port more custom functions to be "fast" natives