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 730101 - Javascript errors in property getters and setter not always reported
Javascript errors in property getters and setter not always reported
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
1.40.x
Other Linux
: Normal normal
: ---
Assigned To: Philip Chimento
gjs-maint
Depends on:
Blocks:
 
 
Reported: 2014-05-14 02:27 UTC by Matt Watson
Modified: 2017-01-01 02:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gjs program demonstrating the bug (753 bytes, application/javascript)
2014-05-14 02:27 UTC, Matt Watson
  Details
patch fix (1.55 KB, patch)
2014-05-14 02:34 UTC, Matt Watson
none Details | Review
patch fix (1.53 KB, patch)
2014-05-14 04:46 UTC, Matt Watson
reviewed Details | Review
Similar patch for override error reporting (2.34 KB, patch)
2014-05-16 20:34 UTC, Matt Watson
reviewed Details | Review
Report errors in js property getters/setters (1.64 KB, patch)
2014-09-29 17:55 UTC, Matt Watson
none Details | Review
jsapi-util-error: Allow throwing custom 'name' property (10.57 KB, patch)
2016-12-28 06:46 UTC, Philip Chimento
committed Details | Review
importer: Throw ImportError when module not found (2.10 KB, patch)
2016-12-28 06:46 UTC, Philip Chimento
committed Details | Review
repo: Ignore ImportError for overrides (9.40 KB, patch)
2016-12-28 06:46 UTC, Philip Chimento
committed Details | Review
repo: Add missing return (733 bytes, patch)
2017-01-01 02:04 UTC, Philip Chimento
committed Details | Review
jsapi-util-error: Fix gjs_throw_custom (2.08 KB, patch)
2017-01-01 02:04 UTC, Philip Chimento
committed Details | Review

Description Matt Watson 2014-05-14 02:27:38 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".
Comment 1 Matt Watson 2014-05-14 02:34:55 UTC
Created attachment 276502 [details] [review]
patch fix
Comment 2 Matt Watson 2014-05-14 02:35:06 UTC
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
Comment 3 Matt Watson 2014-05-14 04:46:06 UTC
Created attachment 276508 [details] [review]
patch fix
Comment 4 Matt Watson 2014-05-16 20:34:29 UTC
Created attachment 276685 [details] [review]
Similar patch for override error reporting
Comment 5 Matt Watson 2014-05-16 20:35:54 UTC
Just added another similar patch for logging errors in javascript override files. Another thing that had been giving us some trouble
Comment 6 Giovanni Campagna 2014-06-15 12:42:38 UTC
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?
Comment 7 Giovanni Campagna 2014-06-15 12:44:16 UTC
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?
Comment 8 Matt Watson 2014-06-15 19:44:02 UTC
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).
Comment 9 Bastien Nocera 2014-09-29 17:24:17 UTC
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
Comment 10 Matt Watson 2014-09-29 17:55:19 UTC
Created attachment 287384 [details] [review]
Report errors in js property getters/setters
Comment 11 Matt Watson 2014-09-29 17:57:11 UTC
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.
Comment 12 Bastien Nocera 2014-09-29 18:00:38 UTC
Still fails with an empty error in my tests.
Comment 13 Cosimo Cecchi 2015-06-12 01:38:22 UTC
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.
Comment 14 Philip Chimento 2016-12-22 05:13:35 UTC
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.)
Comment 15 Philip Chimento 2016-12-28 06:46:11 UTC
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
Comment 16 Philip Chimento 2016-12-28 06:46:18 UTC
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.
Comment 17 Philip Chimento 2016-12-28 06:46:25 UTC
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.
Comment 18 Cosimo Cecchi 2016-12-28 14:56:30 UTC
Review of attachment 342518 [details] [review]:

Looks good to me.
Comment 19 Cosimo Cecchi 2016-12-28 14:57:11 UTC
Review of attachment 342519 [details] [review]:

Looks good.
Comment 20 Cosimo Cecchi 2016-12-28 15:02:57 UTC
Review of attachment 342520 [details] [review]:

Looks good to me.
Comment 21 Philip Chimento 2016-12-29 04:33:42 UTC
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
Comment 22 Philip Chimento 2017-01-01 02:04:08 UTC
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.
Comment 23 Philip Chimento 2017-01-01 02:04:17 UTC
Created attachment 342678 [details] [review]
jsapi-util-error: Fix gjs_throw_custom

A few stragglers from the signature change of gjs_throw_custom().
Comment 24 Philip Chimento 2017-01-01 02:06:19 UTC
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.