GNOME Bugzilla – Bug 729554
Incorrect JS types in callback signatures cause testsuite and gnome-shell crashes on some architectures
Last modified: 2014-08-27 01:29:48 UTC
Created attachment 275875 [details] [review] Fix JS types in callback signatures After upgrading my PowerBook to gnome-shell 3.12 / gjs 1.40 / mozjs24, gnome-shell and a lot of gjs tests crashed. After a lot of head scratching (for a good part of what was supposed to be a long weekend), I noticed that the *_new_resolve callbacks didn't use the same addresses for JS objects as the JS code calling them. This turned out to be due to the callback signatures using the wrong JS types. The attached patch fixes this for the gnome-3-12 branch. It allows me to run gnome-shell and fixes the gjs testsuite[0]. [0] Up to commit d6078d7786c43b2e0fbf551e686be6175810c333, which introduces a failure of the GIMarshalling test for reasons I don't understand yet. But I can still run gnome-shell with the 1.40.1 release plus this patch.
Created attachment 275876 [details] [review] Patch against master
Review of attachment 275876 [details] [review]: Same comments on pretty much all files, only repeating them once. ::: gjs/importer.cpp @@ -867,3 @@ - *objp = NULL; - This should be replaced with objp.set(0), not removed. @@ +882,1 @@ + gjs_debug_jsprop(GJS_DEBUG_IMPORTER, "Resolve prop '%s' hook obj %p priv %p", name, obj, priv); Here we want the object, not the handle (which I'm not even sure how it gets promoted to varargs), so *obj is correct.
(In reply to comment #2) > ::: gjs/importer.cpp > @@ -867,3 @@ > > - *objp = NULL; > - > > This should be replaced with objp.set(0), not removed. From the jsapi.h comment about the JSNewResolveOp hook: * [...] The hook may assume *objp is null on entry. So setting it to NULL in the hook is a no-op. > @@ +882,1 @@ > + gjs_debug_jsprop(GJS_DEBUG_IMPORTER, "Resolve prop '%s' hook obj %p priv > %p", name, obj, priv); > > Here we want the object, not the handle (which I'm not even sure how it gets > promoted to varargs), so *obj is correct. Thanks, I'll attach new patches with that fixed, hopefully tomorrow.
Created attachment 276359 [details] [review] [PATCH v2 master] Fix JS types in callback signatures v2: Don't change gjs_debug_jsprop() lines.
Created attachment 276360 [details] [review] [PATCH v2 gnome-3-12] Fix JS types in callback signatures
BTW, I didn't check for similar problems with other hooks.
(In reply to comment #3) > (In reply to comment #2) > > ::: gjs/importer.cpp > > @@ -867,3 @@ > > > > - *objp = NULL; > > - > > > > This should be replaced with objp.set(0), not removed. > > From the jsapi.h comment about the JSNewResolveOp hook: > > * [...] The hook may assume *objp is null on entry. > > So setting it to NULL in the hook is a no-op. Are you insisting on this anyway, or why hasn't the fix been applied after over three months?
Nah, it's just that we forgot more than anything. Pushed now, thanks for the patch.
Created attachment 284275 [details] [review] Fix JS types in more callback signatures This patch fixes more instances of the same problem which have crept in in the meantime.
Thanks, please push the followup patch I just attached as well. Basically, whenever a JS callback assignment needs a cast to (JS*Op), the callback signature needs to be checked carefully against the definition in jsapi.h. Please keep this in mind. The GIMarshalling test still fails for me, no idea why yet: Gjs-Message: JS LOG: running test testCArray Gjs-Message: JS LOG: running test testGArray Gjs-Message: JS LOG: running test testByteArray Gjs-Message: JS LOG: running test testGBytes Gjs-Message: JS LOG: running test testPtrArray Gjs-Message: JS LOG: running test testGValue Gjs-Message: JS LOG: running test testGType Gjs-Message: JS WARNING: [testGIMarshalling.js 246]: "name" is read-only Gjs-Message: JS LOG: running test testGTypePrototype Gjs-Message: JS LOG: running test testGValueGType Gjs-Message: JS LOG: running test testCallbacks Gjs-Message: JS LOG: running test testVFuncs Gjs-Message: JS LOG: running test testInterfaces (lt-gjs-console:16404): Gjs-WARNING **: JS ERROR: TypeError: GIMarshallingTests.InterfaceImpl is not a constructor testInterfaces@testGIMarshalling.js:405 gjstestRun@resource:///org/gnome/gjs/modules/jsUnit.js:438 gjstestRun@testGIMarshalling.js:3 @testGIMarshalling.js:411 JS_EvaluateScript() failed
Pushed this one too.
Michel, that test failure is caused by a missing g-i patch https://git.gnome.org/browse/gobject-introspection/commit/?h=gnome-3-12&id=21e51026d74bca48b814ace73eb588e6542a27cd