GNOME Bugzilla – Bug 730101
Javascript errors in property getters and setter not always reported
Last modified: 2017-01-01 02:06:19 UTC
Created attachment 276501 [details] gjs program demonstrating the bug Lang.Class allows for GObject properties on javascript classes, and gjs will call javascript getters and setters for these properties as needed. If an error is raised during the body of one of these getters or setters it is not always reported. Usually the error will get reported after the script is finished running with the following warning... (gjs:23353): Gjs-WARNING **: EvaluateScript returned JS_TRUE but exception was pending; did somebody call gjs_throw() without returning JS_FALSE? However, if you use GObject.bind_property, these errors are silently ignored. I have no idea why. Added an attachment showing this, which when run gives no output about the error raised in the setter for property "prop".
Created attachment 276502 [details] [review] patch fix
Still have no idea why GObject.bind_property has any effect of the error output, but think the right thing to do is print out exceptions in property getters and setters as soon as they happen. Attached a patch. When run on the test case print out the stack trace as expected... (gjs:30317): Gjs-WARNING **: JS ERROR: TypeError: v.notathing is undefined TestObject<.prop@setters.js:18 _parent@resource:///org/gnome/gjs/modules/lang.js:131 TestObject<._init@setters.js:13 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 @setters.js:24
Created attachment 276508 [details] [review] patch fix
Created attachment 276685 [details] [review] Similar patch for override error reporting
Just added another similar patch for logging errors in javascript override files. Another thing that had been giving us some trouble
Review of attachment 276508 [details] [review]: (Sorry for taking this long for review) It makes sense to log the exception, but I suppose we should also avoid marshalling the value (which is likely to be garbage at that point) in case of failure?
Review of attachment 276685 [details] [review]: I'm not sure I understand this patch - what's preventing the exception from propagating up the stack in the current code?
So for the first patch in gjs_object_get_gproperty, don't call gjs_value_to_g_value if the JS call just threw an exception? That makes sense to me. The second patch is trickier, and I'm not confident its the right approach. The line that's preventing things from propagating up the stack is here https://git.gnome.org/browse/gjs/tree/gi/repo.cpp#n602 The override lookup will squash all pending exceptions, because it attempts to import an override function for all repo objects, and in the case no overrides/[name].js:_init function is found it will clear all exceptions and assume no override exists. But it doesn't distinguish between the two cases, that there was no override function found (where we don't want output) and that there was an exception in the override function (where we do).
The code in bug 737607 can be used as a test case as well (until the bug is fixed, if it is), though it throws empty errors like: (gjs:28024): Gjs-WARNING **: JS ERROR: (null) @test.js:53
Created attachment 287384 [details] [review] Report errors in js property getters/setters
Hey some action on this! Needed the reminder. Updated the js getter/setter patch to not marshal the junk javascript value after a failed call to a getter.
Still fails with an empty error in my tests.
Comment on attachment 287384 [details] [review] Report errors in js property getters/setters I now pushed the property getters/setters patch, along with a battery of unit tests to verify exceptions are logged as expected. Keeping this open to investigate the other patch.
The imports patch now logs exceptions from module imports even when the import statement is surrounded by a try block, which is not the correct behaviour in my opinion. (It causes the recently-re-added importer tests to fail.)
Created attachment 342518 [details] [review] jsapi-util-error: Allow throwing custom 'name' property According to MDN [1], an idiomatic way to indicate a custom error is by setting the 'name' property: var e = new Error('Malformed input'); // e.name is 'Error' e.name = 'ParseError'; throw e; // e.toString() would return 'ParseError: Malformed input' Add an extra 'name' argument to gjs_throw_custom() to allow this. If 'name' is NULL, then the previous behaviour is maintained. [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/name
Created attachment 342519 [details] [review] importer: Throw ImportError when module not found If a module is not found in the search path, we now throw an exception with its name property set to ImportError. This is so that we can distinguish between the case where there was an error importing the module (it threw an exception, it had a syntax error, etc.) and the case where it did not exist. This distinction is important when importing overrides files.
Created attachment 342520 [details] [review] repo: Ignore ImportError for overrides Previously, lookup_override_function() would unconditionally clear any exceptions, meaning that if an overrides file threw an exception or had a syntax error, it would be silently swallowed and the overrides file would be ignored. Now that we can distinguish between module imports that are not found and module imports with errors, we can clear the exception only in the case where no overrides file was found. The trick used for testing this probably bears some explanation; we want to provide some mock overrides files which cause various errors. We can't set the search path as normal, because the overrides importer (exposed to JS as "imports.overrides") already exists by the time we start our tests. So we set our search path to the mock overrides files directly on that object.
Review of attachment 342518 [details] [review]: Looks good to me.
Review of attachment 342519 [details] [review]: Looks good.
Review of attachment 342520 [details] [review]: Looks good to me.
Attachment 342518 [details] pushed as 5ce1116 - jsapi-util-error: Allow throwing custom 'name' property Attachment 342519 [details] pushed as 6836750 - importer: Throw ImportError when module not found Attachment 342520 [details] pushed as 6144178 - repo: Ignore ImportError for overrides
Created attachment 342677 [details] [review] repo: Add missing return Oops, this likely got lost in a rebase and I didn't re-run the tests.
Created attachment 342678 [details] [review] jsapi-util-error: Fix gjs_throw_custom A few stragglers from the signature change of gjs_throw_custom().
Wow, I really got sloppy here; omitted a return and didn't change all the instances of gjs_throw_custom() to match the new signature. Apparently I rebased and didn't run the tests after rebasing. The above two patches are the fixes I'm committing, for the record.