GNOME Bugzilla – Bug 635707
Adapt to JS_GetStringBytes removal in xulrunner 2
Last modified: 2010-12-02 22:21:59 UTC
Created attachment 175177 [details] [review] First patch which try to fix the issue JS_GetStringBytes was removed in xulrunner 2. Functions need some thinking to adapt to this one because we need to malloc the string now. Mozilla HG changeset: http://hg.mozilla.org/mozilla-central/changeset/f7171a41a816
Created attachment 175178 [details] [review] Second part of the patching try
Created attachment 175180 [details] [review] Fixes to the preceding code Forgot some semi-colon and use proper cast.
Created attachment 175181 [details] [review] Fixes to the preceding code
Created attachment 175203 [details] [review] (Second) First patch which try to fix the issue Still three #error in jsapi-util-string.c because these functions are widely-used in GJS and the malloc/free part is quite hard to do in this case.
Created attachment 175294 [details] More work on JS_GetStringBytes removal porting Continuation of the adaptation to the JS_GetStringBytes removal This patch is not yet ready, it is just designed to show that SardemFF7's work is working, continuing it and adapting a few more things. All functions modified in this patch cause memleaks when compiled withouth JS_GetStringBytes since they are not free'd yet, but with this patch, gnome-shell finally shows up, so that's working.
Created attachment 175300 [details] [review] More work on JS_GetStringBytes removal Improve last patch with some frees, calls to gjs_string_get_ascii_checked still need to be free'd, normally the rest is done
Created attachment 175307 [details] [review] More work on JS_GetStringBytes removal Finish addind frees, normally no leaking left in this patch, and gnome-shell works well with it and SardemFF7's one. Dbus test fails for now, though
Created attachment 175315 [details] [review] More work on JS_GetStringBytes removal Avoid trying to free non alloc'd vars
Created attachment 175539 [details] [review] More work on JS_GetStringBytes removal missing initialization
Review of attachment 175315 [details] [review]: > gjs_string_get_ascii has also been removed in favor of gjs_string_get_ascii_checked since we now always need a context Would prefer that as a separate patch. One thing I don't like about this patch is that it introduces use of malloc(), but we pretty consistently use the GLib functions (ultimately, g_malloc+g_free). ::: gi/boxed.c @@ +114,1 @@ g_free ::: gjs/jsapi-util-string.c @@ +268,3 @@ + return NULL; + + ascii = malloc((len + 1) * sizeof(char)); g_malloc
Ok, will split it into two patches and give a look to g_free and g_malloc.
*** Bug 636135 has been marked as a duplicate of this bug. ***
Review of attachment 175539 [details] [review]: ::: gi/repo.c @@ +129,3 @@ { Repo *priv; + char *name; In other places in the code we often use a local boolean for the return value, rather than two gotos. So here: JSBool ret = JS_FALSE; @@ +139,3 @@ if (strcmp(name, "valueOf") == 0 || strcmp(name, "toString") == 0) + goto return_true; goto out; @@ +162,3 @@ + return_false: + free(name); + return JS_FALSE; ret = JS_TRUE; out: g_free(name); return ret; The point of this pattern is that it avoids duplicating the cleanup code. ::: gi/union.c @@ +99,2 @@ if (method_info != NULL) { JSObject *union_proto; Leaks name if priv->gboxed != NULL; move the g_free() to the end of the function. ::: gjs/importer.c @@ +375,2 @@ g_ptr_array_add(iter->elements, g_strdup(name)); + free(name); It's silly to g_strdup() then g_free(). In other words: /* Pass ownership of name */ g_ptr_array_add(iter->elements, name); @@ +951,3 @@ +return_true: + free(name); + return JS_TRUE; See earlier comment about duplicating exit paths. ::: gjs/stack.c @@ +76,3 @@ const char* found = strstr(value, "function "); + if(found && (value == found || value+1 == found || value+2 == found)) { + free (value); Also, gjs coding style has no space between identifier and parenthesis. So this should be: g_free(value);
Hmm, I just realized that your patch is on top of the first. This is definitely messy; can you merge them? I'll review the first now.
Review of attachment 175203 [details] [review]: ::: gjs/jsapi-util-array.c @@ +323,3 @@ ascii = JS_GetStringBytes(JSVAL_TO_STRING(value)); +#else +#error No JS_GetStringBytes function to test, maybe we need to change the test This isn't an improvement; we know what's broken, we need a patch to fix it, not to display an error. ::: gjs/jsapi-util-string.c @@ +342,3 @@ + return JS_FALSE; + + js_data = malloc((len + 1) * sizeof(char)); g_malloc @@ +344,3 @@ + js_data = malloc((len + 1) * sizeof(char)); + if (!js_data) + return JS_FALSE; And if you use g_malloc, you can remove this; we abort on OOM. @@ +354,3 @@ *len_p = JS_GetStringLength(JSVAL_TO_STRING(value)); *data_p = g_memdup(js_data, *len_p); +#if !HAVE_JS_GETSTRINGBYTES #ifndef ::: gjs/jsapi-util.c @@ +807,1 @@ If we have to malloc, we should switch callers to use gjs_string_to_utf8() or gjs_string_get_binary_data() as appropriate instead of duplicating the #ifdef handling for JS_GetStringBytes() in different places. For example here, for a debug string, since it's going to a log, we should use gjs_string_to_utf8(). Well, I guess in theory it'd be better to use a function which detected invalid UTF-8 in the input and replaced it with runs of hexadecimal or something, but it only matters if someone puts binary data as a property name, which isn't really an important case. @@ +860,1 @@ No; the function should always return g_malloc() data; having it be const char * in one version and not in the other would be a huge maintenance pain. ::: gjs/stack.c @@ +71,3 @@ + JS_EncodeStringToBuffer(value_str, value, len); + value[len] = '\0'; +#endif This one should also just use gjs_string_to_utf8() ::: modules/console.c @@ +233,3 @@ +#if !HAVE_JS_GETSTRINGBYTES + free(display_str); +#endif gjs_string_to_utf8() ::: modules/dbus-exports.c @@ +1446,3 @@ + JS_EncodeStringToBuffer(key_str, key, len); + key[len] = '\0'; +#endif gjs_string_to_utf8() ::: modules/dbus-values.c @@ +131,3 @@ + return JS_FALSE; + } + gjs_string_to_utf8()
Thanks a lot for looking at this, both of you!
I talked with SardemFF7 and I am gonna merge them and apply your suggestions. Thx
Created attachment 175645 [details] [review] get rid of gjs_string_get_ascii First little part split off
Created attachment 175646 [details] [review] Adapt to JS_GetStringBytes removal Merge of the two previous patched with reviews applied. Some tests are failing now, perhaps because of the use of utf8 now ? I didn't look at it really hard (dbus, importer and signals) Another one is failing really hard with a huge stack (everything basic) when calling JS_EvaluateScript, maybe for the same reasons. Dbus was failing with the 2 other patches but the 3 other were not, and the only consistent difference seems to be the use of gjs_string_to_utf8 (or me missing anything of course)
Review of attachment 175645 [details] [review]: This looks fine to me - no objections. However I will observe that you might as well remove the _checked part of the name now too. If you don't want to bother, no big deal.
Created attachment 175652 [details] [review] get rid of gjs_string_get_ascii Rename gjs_string_get_ascii_checked to gjs_string_get_ascii
Created attachment 175653 [details] [review] Adapt to JS_GetStringBytes removal adapt the patch to the rename in the first one
Comment on attachment 175203 [details] [review] (Second) First patch which try to fix the issue https://bugzilla.gnome.org/attachment.cgi?id=175646 is the new one from Keruspe
I think I get why there are those stacks (double frees), trying to fix it
Review of attachment 175653 [details] [review]: ::: gi/ns.c @@ +75,1 @@ More typically, we do: JSBool ret = JS_FALSE; under the assumption that most early paths will be failure. This is less commonly true in the GJS code than in some other C codebases, but I'd rather keep this pattern and add the ret = JS_TRUE then have "ret" sometimes be uninitialized, sometimes not. @@ +150,3 @@ + out: + g_free(name); + return ret; This is better, but we're still duplicating the g_free(), and actually we already were duplicating the JS_EndRequest(). I'll take a look at unifying this.
Created attachment 175660 [details] [review] Adapt to JS_GetStringBytes removal Fix the double frees testDbus, testImporter and testSignals still fail but gnome-shell now runs with it :)
Created attachment 175661 [details] [review] Adapt to JS_GetStringBytes removal initialize all the "JSBool ret;"
Review of attachment 175661 [details] [review]: ::: gi/repo.c @@ +92,3 @@ g_error_free(error); + if (version != NULL) + g_free(version); You don't need the test for != NULL; g_free handles NULL.
Review of attachment 175661 [details] [review]: ::: gjs/jsapi-util-array.c @@ +322,3 @@ g_assert(JSVAL_IS_STRING(value)); + str = JSVAL_TO_STRING(value); +#if HAVE_JS_GETSTRINGBYTES This should be #ifdef, not #if. #if is for things that are always defined, but configure simply doesn't define them if not found: $ grep HAVE_JS_GETSTRING config.h /* #undef HAVE_JS_GETSTRINGBYTES */ $
Review of attachment 175661 [details] [review]: ::: configure.ac @@ +158,3 @@ , [$JS_LIBS]) + AC_CHECK_LIB([mozjs], [JS_GetStringBytes], AC_DEFINE([HAVE_JS_GETSTRINGBYTES], [1], [Define if we still have JS_GetStringBytes]), + , [$JS_LIBS]) This configure check needs to be outside the conditional for xulrunner 2; it needs to be defined on xulrunner < 2.
Created attachment 175669 [details] [review] collection of fixes Small collection of fixes for 175661
Created attachment 175672 [details] [review] Adapt to JS_GetStringBytes removal Apply fixes and reviews
As far as debugging the error checks, I suggest: env GI_TYPELIB_PATH=. libtool --mode=execute valgrind ./gjs-unit (I'll add this to the Makefile soon, what's there is busted)
Review of attachment 175672 [details] [review]: ::: gjs/jsapi-util.c @@ +802,3 @@ + JS_EncodeStringToBuffer(str, bytes, len); + bytes[len] = '\0'; +#endif So, what we really want out of this function is something we can pass to a C logging function. That means it needs to be UTF-8. Currently this code (and it did before this) will return a byte array if that's what's in the JS string. What I think we need to do here is to suck in a copy of _g_utf8_make_valid, and use it. I'll do this myself, now. (See https://bugzilla.gnome.org/show_bug.cgi?id=610969 for background). @@ +849,3 @@ goto next; + gjs_string_to_utf8(context, propval, &value); You're not checking here for whether gjs_string_to_utf8 succeeds; if it doesn't, value is going to be garbage (which is why the dbus test is crashing I think). I think the right thing here is actually to use gjs_value_debug_string, which should be guaranteed to return *something* as UTF-8. Now, the implementation of that function is actually broken though; I'll comment there. @@ +892,2 @@ global = JS_GetGlobalObject(context); + gjs_string_to_utf8(context, OBJECT_TO_JSVAL(global), &global_str); Ditto here.
This is going to step on your patch a bit since I'll have to make gjs_value_debug_string allocate, but there's only a few users.
Committed: commit 52593563c9e9873231f5fdae3dc1668460bee37e Author: Colin Walters <walters@verbum.org> Date: Wed Dec 1 17:11:16 2010 -0500 gjs_value_debug_string: Always return UTF-8 Returning whatever JS_GetStringBytes gave us will blow up if the string contains non-UTF8 characters and we're trying to g_print to a UTF-8 terminal (the standard case). Since this is just a debugging string, import a copy of _g_utf8_make_valid which squashes it to UTF-8 in a useful way.
I may have suggested using gjs_string_to_utf8 in those places earlier; sorry, that was a bad suggestion. Among other serious flaws, it can leave a pending exception on the JS runtime we'd need to clear with JS_ClearPendingException if it did fail. Using gjs_value_debug_string() is the right thing to do if all we're doing is passing it to g_print() or gjs_log() or whatever.
One other thing - you should credit Sardem FF7 in your commit message. Actually the message is sort of wrong because we *are* still using JS_GetStringBytes if available. I'd suggest something like this: Conditionally handle availability of JS_GetStringBytes This upstream mozilla commit: http://hg.mozilla.org/mozilla-central/changeset/f7171a41a816 removed JS_GetStringBytes in favor of an API which requires the caller to allocate. We were using this function in a number of places, and most of those now need to malloc() with the new API. Rather than trying to use JS_GetStringBytes as "const", and malloc() only with the new API, wrap it and always malloc()/free(). Based on a patch originally from Sardeem FF7 <sardeemff7.pub@gmail.com>.
Created attachment 175677 [details] [review] Conditionally handle availability of JS_GetStringBytes Adapt to gjs master and change commit message
I still see this error with your patch: ==23865== Invalid read of size 1 ==23865== at 0x3405428FF3: ??? (in /lib64/libdbus-1.so.3.5.2) ==23865== by 0xE90DFE8: gjs_js_one_value_to_dbus (dbus-values.c:897) ==23865== by 0xE90EEA3: append_dict (dbus-values.c:865) ==23865== by 0xE90E2A3: gjs_js_one_value_to_dbus (dbus-values.c:989) ==23865== by 0xE90F080: gjs_js_values_to_dbus (dbus-values.c:1034) ==23865== by 0xE9114FA: prepare_call.clone.1 (dbus.c:195) ==23865== by 0xE91162E: gjs_js_dbus_call_async (dbus.c:415) ==23865== by 0x340786C74B: ??? (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x3407874CF7: js_Invoke (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x34078752BF: ??? (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x3407820479: JS_CallFunctionValue (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x4C186DB: gjs_call_function_value (jsapi-util.c:1141) ==23865== Address 0xced9f20 is 0 bytes inside a block of size 2 free'd ==23865== at 0x4A05187: free (vg_replace_malloc.c:325) ==23865== by 0x54C5872: g_free (gmem.c:263) ==23865== by 0xE90EE89: append_dict (dbus-values.c:863) ==23865== by 0xE90E2A3: gjs_js_one_value_to_dbus (dbus-values.c:989) ==23865== by 0xE90F080: gjs_js_values_to_dbus (dbus-values.c:1034) ==23865== by 0xE9114FA: prepare_call.clone.1 (dbus.c:195) ==23865== by 0xE91162E: gjs_js_dbus_call_async (dbus.c:415) ==23865== by 0x340786C74B: ??? (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x3407874CF7: js_Invoke (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x34078752BF: ??? (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x3407820479: JS_CallFunctionValue (in /usr/lib64/xulrunner-1.9.2/libmozjs.so) ==23865== by 0x4C186DB: gjs_call_function_value (jsapi-util.c:1141)
I have to leave now, but try "make valgrind-check" now - I made it work again. You can see the log messages in a file "valgrind.gjs-unit.log".
Ok, found why, I'm freeing value_signature while dbus is still using it, all make check pass now, but found a few leaks with valgrind, patch coming in a few hours
Created attachment 175705 [details] [review] Conditionally handle availability of JS_GetStringBytes make check succeeds fix some memleaks Btw, make valgrind check always says that the run-with-dbus script returns failure, even if every test pass
Review of attachment 175705 [details] [review]: This is getting close; a few more comments. One big picture item - we're mallocing a lot more, with resulting problems like fragmentation. We should observe that many of these strings are just short ASCII, and also that they're all scoped to the function call lifetime. Brainstorming a bit; we could add an internal pool allocator API with some convenience functions for JSString->ascii/utf8 conversion; something like: GjsMemPool *_gjs_mem_pool_thread_default_push(void); const char *_gjs_mem_pool_string_to_ascii(GjsMemPool *pool, JSContext *context, JSString *str); const char *_gjs_mem_pool_string_to_utf8(GjsMemPool *pool, JSContext *context, JSString *str); const guint8 *_gjs_mem_pool_string_to_byte_array(GjsMemPool *pool, JSContext *context, JSString *str); void _gjs_mem_pool_pop(GjsMemPool *pool); Actually for the common case of handling "jsid" we need more special magic to check whether the jsid is a string...anyways I'm not going to write this all out now, just noting for someone later to optimize. ::: gjs/stack.c @@ +59,3 @@ value_str = JS_ValueToString(cx, val); if (value_str) + gjs_string_to_utf8(cx, val, &value); Use gjs_value_debug_string() here, since that's all we're doing - stringifying a value to print in a debugging stack trace. ::: modules/console.c @@ +216,3 @@ + char *display_str; + gjs_string_to_utf8(context, result, &display_str); + if (display_str != NULL) { This will leave a pending exception on the interpreter if gjs_string_to_utf8() fails. Again it's basically always wrong to not check the return value. Now gjs_value_debug_string() is the quickest fix, but I think what we *really* want to do is print it as JSON (see JS_Stringify). I can create a jsapi utility for this. Anyways for now just use gjs_value_debug_string(). ::: modules/dbus-exports.c @@ +837,3 @@ + char *name; + char *signature; + char *access; It's a *lot* cleaner to initialize these to NULL here: name = NULL; signature = NULL; etc, then: @@ +876,3 @@ "Property %s has no access", name); + goto signature_fail; Just goto fail; @@ +882,3 @@ access_val); + if (access == NULL) + goto signature_fail; goto fail; @@ +895,3 @@ } else { gjs_throw(context, "Unknown access on property, should be readwrite read or write"); + goto access_fail; goto fail; @@ +909,3 @@ + g_free(signature); + fail: + g_free(name); Then here, just: fail: g_free(access); g_free(signature); g_free(name); (Remember g_free() handles NULL). @@ +1455,3 @@ + gjs_string_to_utf8(context, keyval, &key); + if (key == NULL) + goto out; This is OK but I'd prefer it written as: if (!gjs_string_to_utf8(context, keyval, &key)) goto out; ::: modules/dbus-values.c @@ +123,3 @@ JS_AddStringRoot(context, &key_str); + gjs_string_to_utf8(context, key_value, &key); + if (key == NULL) { Ditto: if (!gjs_string_to_utf8(context, key_value, &key)) ... ::: modules/dbus.c @@ +128,3 @@ gboolean auto_start; + char *out_signature; + char *in_signature; See earlier comment about initializing all of these to NULL instead of having lots of different gotos; @@ +713,3 @@ + char *object_path; + char *iface; + char *signal; Ditto here.
Created attachment 175723 [details] [review] Conditionally handle availability of JS_GetStringBytes applied
Review of attachment 175723 [details] [review]: Ok. This is mostly good, and I'd rather get it in first and iterate on some smaller details later. (You changed the goto's I commented on, but not others e.g.). I think we'll have to do a goto-cleanup pass after this.
Review of attachment 175723 [details] [review]: I should also note that while I reviewed *some* of the flow controls around the free()s, I didn't do all of it. We'll have to watch out carefully later for problems.
Thanks for the patch!