GNOME Bugzilla – Bug 789223
gnome-shell crashes in meta_plugin_manager_event_simple when no plugin manager is set (yet?)
Last modified: 2017-10-24 09:45:36 UTC
This issue seems to be around in both Ubuntu (https://pad.lv/1725068) and redhat (https://bugzilla.redhat.com/show_bug.cgi?id=1452914) bug trackers. Stacktrace is:
+ Trace 238080
So basically, it's quite easy to see the crash and it probably happens because the compositor has not been managed yet... So, fixing the crash can be quite easy, but just wondering... Could this happen before the compositor is properly managed, and thus anticipating compositor->plugin_mgr = meta_plugin_manager_new (compositor); there could help, or just could happen that a window wants to be shown/hidden when no compositor is set yet? In any case I think in this case is just sane to no do any effect and, better, not to crash.
Created attachment 361907 [details] [review] MetaPluginManager: ensure we deference a valid pointer when processing events Alternative to this is actually to just not calling meta_plugin_manager_event_simple in MetaWindow's start_simple_effect if the compositor is not managed, thus just applying the requested window transformation. Up to you.
Review of attachment 361907 [details] [review]: ::: src/compositor/meta-plugin-manager.c @@ +159,3 @@ + gboolean retval; + + g_return_val_if_fail (plugin_mgr, FALSE); This is just a debug-build-only soft-assert, which is fine I guess, but the commit message should reflect that. Calling this function with NULL makes no sense anyhow, and is an application bug, so I don't think we need to handle that more than this though.
Created attachment 361912 [details] [review] MetaPluginManager: don't try to deference a NULL pointer when processing events This function might be called by components with invalid plugin manager (as it might happen to MetaWindow when the compositor isn't initialized properly), so we need to protect ourselves from crashes.
Review of attachment 361912 [details] [review]: Hmm, so this function is not exposed outside of mutter, then it's not the right thing to handle invalid input here, as it'll just hide the actual error. If the compositor isn't initialized properly, we should instead gracefully shut down without ever trying to start an effect (seems to be the only place this function is called from)
Created attachment 361917 [details] [review] MetaWindowActor: don't start any effect when no compositor is available There are cases when no compositor is available (yet) but a MetaWindow tries to start a simple effect using a compositor plugin which is not available. In that case we should just ignore any request and protect ourselves from crashes.
I wonder what these cases are. Is this reproducible? What's the JS stack trace?
Have discussed this issue a bit on IRC, and the only explanation I can think of is that an extension calls Mainloop.run() or something similar on startup, which happens to dispatch a clutter paint. We should not allow running the main loop from within JS this way, so we should rather catch the JS backtrace instead. A way to maybe do this is to add a gjs_dumpstack() as a SIGABRT handler, and add an g_assert() instead of the g_return_val_if_fail() as added in one of the patches.
Created attachment 362153 [details] [review] MetaWindowActor: don't start any effect when no compositor is available As said, let's just assert now... The bad guys will be easier to find now (as per changes of Bug 789237).
Review of attachment 362153 [details] [review]: commit message subject is incorrect. It should be something like "MetaWindowActor: Assert that we have plugin manager on simple effect" then explain that this is in order to catch bugs. The current message makes it sound like we actually try to handle it sanely. With that fixed, this lgtm.
The following fixes have been pushed: https://git.gnome.org/browse/mutter/commit/?id=91e3a0b3