GNOME Bugzilla – Bug 787668
Crash on monitor config migration
Last modified: 2017-10-04 14:33:27 UTC
After upgrading an install to f27, gnome-shell started crashing on startup. (gdb) bt
+ Trace 237961
As the crash happens during monitor config migration and it never finishes, every next login will still try to perform the migration and fail at it. AFAICT the crash seems mostly down to slightly differing refresh_rate values. I'm attaching a patch to add some threshold to refresh rate comparisons that seems to fix the case here. However I'm also attaching a patch to bail out safely if no MetaMonitorModeSpec matches.
Created attachment 359780 [details] [review] backends: Add some wiggle room for refresh rate comparisons We have not enough control over the sources of the refresh rate float variable to make == comparisons reliable, add some room when comparing these.
Created attachment 359781 [details] [review] backends: Fallback on scale=1 if no matching monitor mode can be found It is possible to receive NULL here, so make it sure we handle it safely.
Review of attachment 359780 [details] [review]: lgtm.
Review of attachment 359781 [details] [review]: I don't think the caller should pass NULL to this function. I guess this is the migration code that doesn't have a matching mode, since it might try to migrate a config to different monitor, and that code should handle not finding the mode.
(In reply to Jonas Ådahl from comment #4) > Review of attachment 359781 [details] [review] [review]: > > I don't think the caller should pass NULL to this function. I guess this is > the migration code that doesn't have a matching mode, since it might try to > migrate a config to different monitor, and that code should handle not > finding the mode. Right, but I wonder what's the best way to proceed here, remove the affected MetaLogicalMonitorConfig altogether?
Comment on attachment 359780 [details] [review] backends: Add some wiggle room for refresh rate comparisons Attachment 359780 [details] pushed as 743e8cc - backends: Add some wiggle room for refresh rate comparisons
(In reply to Carlos Garnacho from comment #5) > (In reply to Jonas Ådahl from comment #4) > > Review of attachment 359781 [details] [review] [review] [review]: > > > > I don't think the caller should pass NULL to this function. I guess this is > > the migration code that doesn't have a matching mode, since it might try to > > migrate a config to different monitor, and that code should handle not > > finding the mode. > > Right, but I wonder what's the best way to proceed here, remove the affected > MetaLogicalMonitorConfig altogether? Yea, discard it (together with the whole MetaMonitorsConfig) with a warning. The alternative is to keep it unmigrated and maybe try when an "identical" (same connector/vendor/product/serial) monitor that is actually different is connected in the future. Not sure that's better though, to keep a bunch of possibly never applicable half migrated configurations left.
Created attachment 360389 [details] [review] monitor-config-migration: Discard if configured mode is missing If a configuration key matched a current system state, but no monitor mode was found (for example because of an incorrect refresh rate), discard it while logging a warning.
Created attachment 360390 [details] [review] monitor-tests: Test config migration with refresh rate wiggle room Check that we finish configurations within range, and discard the ones out of range.
Review of attachment 360389 [details] [review]: Sure
Review of attachment 360390 [details] [review]: LGTM ::: src/tests/monitor-unit-tests.c @@ +4947,3 @@ + old_config_file, + &error)) + g_error ("Failed to migrated config: %s", error->message); Typo: s|migrated|migrate| @@ +5091,3 @@ + old_config_file, + &error)) + g_error ("Failed to migrated config: %s", error->message); Dto.
Attachment 360389 [details] pushed as 43eeb00 - monitor-config-migration: Discard if configured mode is missing Attachment 360390 [details] pushed as 120db06 - monitor-tests: Test config migration with refresh rate wiggle room