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 636927 - dbus: Fix dict entry conversion path
dbus: Fix dict entry conversion path
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2010-12-09 23:00 UTC by Colin Walters
Modified: 2010-12-10 16:08 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
dbus: Fix dict entry conversion path (7.27 KB, patch)
2010-12-09 23:00 UTC, Colin Walters 		accepted-commit_now Details | Review

Description Colin Walters 2010-12-09 23:00:10 UTC 
Multiple issues:

* We need to handle JS_ValueToString possibly returning NULL, a
  look at the SpiderMonkey sources shows it can happen in unusual
  circumstances.
* The JS_Add/RemoveRoot pairing was utterly fucked.  Fix it by
  rooting everything at the top.
* Move it to a separate function for sanity.
Comment 1 Colin Walters 2010-12-09 23:00:12 UTC 
Created attachment 176157 [details] [review]
dbus: Fix dict entry conversion path
Comment 2 Owen Taylor 2010-12-09 23:40:38 UTC 
Review of attachment 176157 [details] [review]:

So much cleaner!

::: modules/dbus-values.c
@@ +80,3 @@
+        key_str = JS_ValueToString(context, key_value);
+        if (key_str == NULL) {
+            gjs_throw(context, "Couldn't convert value to string");

From looking through the code like JS_ValueToString will always set an exception if it fails.. "can't convert <whatever> to string" or something like that. (And certainly code calling it within spidermonkey assumes that.) So calling gjs_throw() here is going to overwrite that exception. Hmm, but that's OK, looking at gjs_throw()

       /* Often it's unclear whether a given jsapi.h function
         * will throw an exception, so we will throw ourselves
         * "just in case"; in those cases, we don't want to
         * overwrite an exception that already exists.
         * (Do log in case our second exception adds more info,
         * but don't log as topic ERROR because if the exception is
         * caught we don't want an ERROR in the logs.)
         */

so I think this is fine unless you think that this message is more important than whatever spidermonkey produced.
Comment 3 Mathieu Bridon 2010-12-09 23:44:30 UTC 
I've been running with this patch for more than half an hour, and gnome-shell didn't crash (the crash for which I provided backtraces on IRC which made you write this patch).

The crash was pretty random anyway, so it's probably not conclusive, but it used to crash more often than that.
Comment 4 Johannes Schmid 2010-12-10 13:03:21 UTC 
Confirming that this fixes the problem here on f14-64bit. gnome-shell was just crashing before the patch on startup.
Comment 5 Colin Walters 2010-12-10 16:08:18 UTC 
Thanks for the confirmation of the fix!