added 1 commit

• c824a586 - object: don't resolve or set,get properties on finalized elements

Compare with previous version

added 1 commit

• f94a9d07 - object: don't resolve or set,get properties on finalized elements

Compare with previous version

Thanks for the merge request. I read the background on the Bugzilla bug, to catch up a bit. On the one hand this Shouldn't Happen™ and I suspect the object is being destroyed on the C side where it shouldn't be. On the other hand we should definitely be robust to this.

I get your point that it might affect performance, since we do have to do these checks in what are probably very hot paths. But I think it's worth it to be able to prevent crashes and print a helpful message that will lead to fixing faulty JS code faster. Until we actually have a good way to measure performance regressions it's just guesswork anyway.

Returning false from a JSAPI function usually means an exception must be thrown. In this case, I would log a critical and return true. That will make the property get operation proceed on the JS side as if the object were a plain JS object.

Alternatively, you could throw an exception on the JS side with gjs_throw(). But based on my hypothesis that this only happens when C code is behaving badly, my first instinct is to go for the transparent-to-JS approach.

I've tried using g_critical only for this, but the problem of it is that it's not causing any JS stack to be shown, while I'd prefer to see it.

So I guess returning true and gjs_throw is fine anyway, as it seems to do both things requested.

You can also print a JS stack with gjs_dumpstack().

Yep, I new it... Not sure what was the preferred way here... g_critical + dumpstack or just a gjs_throw?

changed this line in version 4 of the diff

Same here, I would log a critical and return result.succeed().

changed this line in version 4 of the diff

Same here.

Returning true here too?

Yes, returning false here also means an exception must be thrown.

changed this line in version 4 of the diff

nitpick style rule that I haven't written down yet: in new code I prefer using auto if the variable's type is already clearly stated in a static_cast<> operator.

Seeing my C++1x backgroound (and love for it), I wanted to do it, but I noticed that auto wasn't much used yet... So I went classic. Happy to update it.

changed this line in version 4 of the diff

I would make this a separate condition and log a critical here too.

changed this line in version 4 of the diff

We should consider what to do if throw_error is false - should we log a critical there too? Or possibly log a critical in both cases.

Mh, yeah, I guess throwing should happen in any case here... Or just showing a critical otherwise.

changed this line in version 10 of the diff

This message isn't quite accurate, the object isn't %s.%s.prototype. In that case priv->gobj would have been nullptr.

In any case I would suggest making a more verbose error message that will help shell extension authors track down what they might be doing wrong. Here's an example of such a message from elsewhere in GJS.

changed this line in version 4 of the diff

I have a few other ideas for improvements. I think that if you do priv->keep_alive.reset() when a finalized GObject is detected, that should at least release the GC root that the GObject keeps on the JS object, making it more likely to be garbage collected next time around.

It would also be nice to have a regression test for this situation. The code in object.cpp is pretty complicated, so I'd like to be sure that when it's changed, we're not going back to making shell extension code crash. A good place for this test might be gjs/test/gjs-tests.cpp unless you know of a way to trigger the bug purely in JS code.

added 1 commit

• 6748006d - object: add better logging when invalid access happens, also don't block it

Compare with previous version

added 1 commit

• 73eb5486 - object: reset the keep alive flag on wrapper when object is disposed

Compare with previous version

Ok, I've updated the PR with some more commits doing what you required... I know I'm terrible when it comes to logging and being explicative, so please feel free to suggest better wording if you want.

have a few other ideas for improvements. I think that if you do priv->keep_alive.reset() when a finalized GObject is detected, that should at least release the GC root that the GObject keeps on the JS object, making it more likely to be garbage collected next time around.

Good idea, I've added that.

It would also be nice to have a regression test for this situation. The code in object.cpp is pretty complicated, so I'd like to be sure that when it's changed, we're not going back to making shell extension code crash.

Ok, let me look into this.

unless you know of a way to trigger the bug purely in JS code.

While I had some hacks to get that happening in G-S it's really not the best way, while for example a code like this below, triggers things

imports.gi.versions.Gtk = "3.0";

const GLib = imports.gi.GLib;
const Gtk = imports.gi.Gtk;

Gtk.init(null);

let loop = GLib.MainLoop.new(null, false);
let win = new Gtk.Window({type: Gtk.WindowType.TOPLEVEL})

GLib.timeout_add(GLib.PRIORITY_DEFAULT, 500, function() {
  print(win);
  print(win.title);
  //win.set_title("foo");
  win.title = "foo";
  loop.quit();
  return false;
})

win.destroy();

loop.run();

Or just without any timeout:

imports.gi.versions.Gtk = "3.0";

const GLib = imports.gi.GLib;
const Gtk = imports.gi.Gtk;

Gtk.init(null);

let win = new Gtk.Window({type: Gtk.WindowType.TOPLEVEL})
win.destroy()

print(win);
print(win.title);
win.title = "foo";
win.set_title("foo");

added 2 commits

• b64b5b45 - object: add better logging when invalid access happens, also don't block it
• a1d07c79 - object: reset the keep alive flag on wrapper when object is disposed

Compare with previous version

resolved all discussions

For future reference I try to use static_cast<> in new code instead of C casts, but I have a branch where I'm refactoring existing uses of this anyway, so no need to bother - I'll fix it up when I rebase.

I actually wanted to do it, but since it wans't used everywhere I wasn't aware of the policy.

changed this line in version 10 of the diff

resolved all discussions

I wasn't aware of auto *, I thought the pointer was already absorbed into auto! Should I start doing this too, to write more idiomatic C++ style?

You can use also auto& when needed to ensure it's a ref.

Here I think it's better when you're sure it's a pointer and you still want to make it clear, while it won't change the result, it will keep things easier to read IMHO.

Looks good now, let me know if you need any help with the test.

I did try the two JS snippets you posted, but they didn't crash for me. However, they did print the log messages when building with your branch, so I guess I was just lucky. I think the latter one is a good one to base the test on.

PS. If you have a C++1x background then can I ask for your help on some future reviews? 😄 I am not the most experienced in C++, I kind of picked it up when I took over maintainership of GJS.

As for the test I'm not sure how to proceed...

Also, probably (by running G-S whith this) I was thinking that maybe it would be better to to use a lower priority message in object_instance_trace, as it looks to lead to be very verbose, while the dumpstack doesn't seem to be often available.

PS. If you have a C++1x background then can I ask for your help on some future reviews? 😄 I am not the most experienced in C++, I kind of picked it up when I took over maintainership of GJS.

Sure, feel free! I've done quite a lot of lines of it...

As for the test I'm not sure how to proceed...

I'd suggest adding it to one of the files in installed-tests/js/. There's a "Garbage collection of introspected objects" section at the end of testEverythingEncapsulated.js which seems like a good place. We use Jasmine 2.5 for unit testing which you can read about here. To test that the messages are logged, you can use GLib.test_expect_message() and GLib.test_assert_expected_messages_internal(), there's an example of how to do that in testExceptions.js.

Also, probably (by running G-S whith this) I was thinking that maybe it would be better to to use a lower priority message in object_instance_trace, as it looks to lead to be very verbose, while the dumpstack doesn't seem to be often available.

That's a good point, I didn't think about it. Tracing will indeed take place many times during garbage collection, and that's exactly when the JS stack isn't so important. So probably a good idea to just silently exit from trace.

added 2 commits

• 1907067e - object: add better logging when invalid access happens, also don't block it
• 126f6f5e - object: reset the keep alive flag on wrapper when object is disposed

Compare with previous version

added 3 commits

• 95928e8e - object: add better logging when invalid access happens, also don't block it
• bf39334f - object: reset the keep alive flag on wrapper when object is disposed
• e35ccd24 - installed-tests/js: add testGObjectDestructionAccess to verify access to destryed objects

Compare with previous version

added 1 commit

• d7cfbcdc - installed-tests/js: add testGObjectDestructionAccess to verify access to destryed objects

Compare with previous version

resolved all discussions

Heh, it was good you wrote this test — I realize now that the implementation is kind of inconsistent, because calling a method through GObject-introspection will call gjs_typecheck_object() which will throw, even if the other faulty accesses just log.

I suppose we should pick either logging or throwing, and make it do only one. If we pick logging, then I guess gjs_typecheck_object() should not throw even if throw_error is requested. In a way that makes sense, the object was of the correct type, it's just gone 😄 If we pick throwing, then every operation should just throw (except tracing.)

As you wish, for me both ways are the same... Throwing is probably cleaner for some aspects (as it also dumps the stack), but it might have other side effects I'm not fully aware of.

So up to you... Just tell me the strategy you prefer, and I'll follow that. ;-)

I was originally leaning toward logging, but I've come to think throwing is better. It makes the error harder to ignore, so app and shell extension authors will be less likely to publish buggy JS code, and if the fault is not in their code then they will be more likely to report a gnome-shell bug to get it fixed. Also, execution can't meaningfully continue anyway when our invariant of JS objects wrapping only live GObjects is broken.

So, to throw, then all the operations should call gjs_throw(), not touch any output params, and return false.

resolved all discussions

resolved all discussions

added 3 commits

• b4cae4b3 - object: add better logging when invalid access happens
• 81225d06 - object: reset the keep alive flag on wrapper when object is disposed
• 516012f8 - installed-tests/js: add testGObjectDestructionAccess to verify access to destryed objects

Compare with previous version

resolved all discussions

added 1 commit

• c7293959 - installed-tests/js: add testGObjectDestructionAccess to verify access to destryed objects

Compare with previous version

resolved all discussions

added 1 commit

• 6be9eb13 - installed-tests/js: add testGObjectDestructionAccess to verify access to destryed objects

Compare with previous version

Updated as requested, I think it should good to go now :)

Technically according to the SpiderMonkey API you are supposed to leave this out parameter unchanged if you return false. I don't think it makes a difference in practice, but I'll make a quick followup commit to remove this line.

resolved all discussions

mentioned in commit db3a7832

merged

Thanks for all the work! 🎉

mentioned in commit 7a997610

mentioned in merge request !25 (merged)

mentioned in merge request !27 (merged)

mentioned in issue #21 (closed)

mentioned in issue #23 (closed)