Thanks, Chris, for analyzing and reporting this. This issue prevents me from upgrading to Ubuntu 14.04 -- is there any chance include the fix in Gnome 3.10 so that it's be available for Ubuntu 14.04?

When would we ever get an XRandr output with no modes?

When they are all deleted. The mode lists are user modifiable.

OK. What should we do in that case? Turn the output off? Skip it?

(In reply to comment #4)
> OK. What should we do in that case? Turn the output off? Skip it?

Well this can be best answered after we know *why* a user would delete all modes ... i.e what is he/she trying to achieve by doing that? Just playing around and doing it by accident? Or ?

We are getting this bug ("no monitors found") in this situation:

- Boot a laptop in a docking station with the lid closed.
- Plug the external monitor until later.

For a sketchy way to test this:

  xrandr --output eDP1 --off; sleep 5s; xrandr --output eDP1 --auto

(change the "eDP1" for your output's actual name.)

A lot of the code in Mutter assumes that there is at least one monitor in screen->monitor_infos[]; not all of the code checks for screen->n_monitor_infos being nonzero.

I tried looking for that pattern and adding null checks, but it's not always clear what the code should do in case there are no monitors.

I'm going to try doing the following.  If the RANDR code comes up with n_monitor_infos == 0, then it will set it to 1, and create a dummy monitor struct for screen->monitor_infos.  The rest of the code will lay out windows as usual, and hopefully do the right thing once a real RANDR output comes up.

See https://bugzilla.novell.com/show_bug.cgi?id=879109 for this.

Created attachment 277634 [details] [review]
0001-MonitorManager-generate-a-null-configuration-of-Meta.patch

Well, this turned out simpler than I expected :)  The patch is pretty unobtrusive; it creates a dummy configuration of MetaMonitorInfo, and makes it available to the callers of MetaMonitorManager.

Meanwhile, callers of the DBus API *do* get the real RANDR state.

For me this fixes the crash that happens when you turn off the outputs with xrandr(1).

Owen suggested on IRC that the dummy configuration should try to preserve the real monitor's size, so windows are not squashed when a real monitor is plugged back in again.  I'm not sure how to do this.

Review of attachment 277634 [details] [review]:

Mh... this patch looks like an mixture of hackish attempts to get stuff going.
IMHO, we should not create fake outputs or fake modes - even if off, the outputs are still there (and the case of a desktop pc with the vga cable unplugged needs to be handled properly in meta-display-config.c). In other words, all callers of get_outputs()/get_resources() need to be able the handle 0 outputs. What is even worse is the fake CRTC, because in the end, CRTCs are there.
And that means obviously that all backends need to be able to cope with all CRTCs configured off (I believe they already do).

OTOH, faking a single MetaMonitorInfo when all outputs are disabled makes sense (because we need a place to put the windows) - but the fix is incomplete: you also need to make sure the framebuffer is properly sized (XRRSetScreenSize, for the x11 backend), even if no CRTC is scanning out. Obviously this becomes an even bigger hack, because the right fix would be to stop painting until at least a monitor is up again instead of painting into the void, but at least on X11 stuff breaks badly if you resize the screen below a certain point.

Note also that metadisplayconfig will try as hard as it can to make sure at least one monitor is always on, so at least for automatic configuration and dock/laptop lid we're covered (assuming you do have a monitor plugged, that is). Obviously xrandr or the dbus API can make arbitrary change, so we need to cope with them.

Jasper says on IRC that this is what the code did before the introduction of MetaMonitorManager (see commit 214f31257b0accc38f2bc3d4e012335386bc8b23):

src/core/screen.c:
-  /* If no Xinerama, fill in the single screen info so
-   * we can use the field unconditionally
-   */
-  if (screen->n_monitor_infos == 0)
-    {
-      meta_topic (META_DEBUG_XINERAMA,
-                  "No Xinerama screens, using default screen info\n");
-          
-      screen->monitor_infos = g_new0 (MetaMonitorInfo, 1);
-      screen->n_monitor_infos = 1;
-          
-      screen->monitor_infos[0].number = 0;
-      screen->monitor_infos[0].rect = screen->rect;
-      screen->monitor_infos[0].in_fullscreen = -1;
-    }

This is what my make_null_monitor_info() does.  Mine is a bit more verbose because we have more fields now.

(In reply to comment #8)

> IMHO, we should not create fake outputs or fake modes - even if off, the
> outputs are still there (and the case of a desktop pc with the vga cable
> unplugged needs to be handled properly in meta-display-config.c). In other
> words, all callers of get_outputs()/get_resources() need to be able the handle
> 0 outputs.

Normally I'd agree that all code needs to be able to deal with 0 usable outputs.

However, *none* of the code is written to do that.

There are various parts to the problem:

1. MetaMonitorManager provides a leaky abstraction.  It wants to present a view where the array of MetaMonitorInfo is the only knowledge that the rest of the Mutter code has into the current monitors, *BUT* it turns out that key parts of the code also want to access the MetaOutputs.

2. For example, window.c does, "give me the outputs.  Now, which output does this window live in?  Now, which MetaMonitor corresponds to that output?".

Look at this stack trace (it's from 3.10, but the code is practically the same in master - it just got moved to backends/):

Program received signal SIGSEGV, Segmentation fault.
0x00007f433fd01ee0 in get_work_area_monitor (window=0x365e390,
area=0x7fffa5b77410, which_monitor=87653696) at core/window.c:9830
9830      *area = window->screen->monitor_infos[which_monitor].rect;
(gdb) p which_monitor
$1 = 87653696
(gdb) l
9825      GList *tmp;
9826
9827      g_assert (which_monitor >= 0);
9828
9829      /* Initialize to the whole monitor */
9830      *area = window->screen->monitor_infos[which_monitor].rect;
9831
9832      tmp = meta_window_get_workspaces (window);
9833      while (tmp != NULL)
9834        {
(gdb) where

+ Trace 233720

Frame 1, meta_window_get_work_area_for_monitor(), gets a which_monitor=87653696 because meta_window_get_work_area_current_monitor() does this:

void
meta_window_get_work_area_current_monitor (MetaWindow    *window,
                                           MetaRectangle *area)
{
  const MetaMonitorInfo *monitor = NULL;
  monitor = meta_screen_get_monitor_for_window (window->screen,
                                                window);

  meta_window_get_work_area_for_monitor (window,
                                         monitor->number,
                                         area);
}

In turn, meta_screen_get_monitor_for_window():

const MetaMonitorInfo*
meta_screen_get_monitor_for_window (MetaScreen *screen,
                                    MetaWindow *window)
{
  MetaRectangle window_rect;

  meta_window_get_frame_rect (window, &window_rect);

  return meta_screen_get_monitor_for_rect (screen, &window_rect);
}

And then:

const MetaMonitorInfo*
meta_screen_get_monitor_for_rect (MetaScreen    *screen,
                                  MetaRectangle *rect)
{
  int i;
  int best_monitor, monitor_score, rect_area;

  if (screen->n_monitor_infos == 1)
    return &screen->monitor_infos[0];

  best_monitor = 0;
  monitor_score = -1;

  rect_area = meta_rectangle_area (rect);
  for (i = 0; i < screen->n_monitor_infos; i++)
    {
      gboolean result;
      int cur;

      if (rect_area > 0)
        {
          MetaRectangle dest;
          result = meta_rectangle_intersect (&screen->monitor_infos[i].rect,
                                             rect,
                                             &dest);
          cur = meta_rectangle_area (&dest);
        }
      else
        {
          result = meta_rectangle_contains_rect (&screen->monitor_infos[i].rect,
                                                 rect);
          cur = rect_area;
        }

      if (result && cur > monitor_score)
        {
          monitor_score = cur;
          best_monitor = i;
        }
    }

  return &screen->monitor_infos[best_monitor];
}

Here, best_monitor always remains at 0, but screen->monitor_infos[0] is uninitialized memory!  That happens because meta_monitor_manager_get_monitor_infos() simply returns the buffer from g_array_free(), which is *valid* allocated memory, but with *uninitialized* contents since there were zero usable monitors when processing the RANDR configuration.

I started fixing this bug by putting checks wherever window.c or whoever assumed that there was at least one monitor, but it turned too cumbersome.  There are no "exit routes" in a lot of that code in case there are no monitors.  This is why I thought it would be easier to just use a temporary, null monitor configuration.

I don't like generating null outputs/CRTCs.  Honestly I didn't try generating *just* the null MetaMonitorInfo, without touching the outputs/CRTCs, but since the code does a "second pass" to adjust the Xinerama output numbers, I didn't want to figure out the implications of using a set of outputs that wasn't in sync with the generated MetaMonitorInfo.

Anyway, the bug is easy to reproduce.  Run this:

  xrandr --output eDP1 --off; sleep 5s; xrandr --output eDP1 --auto

(using your output's correct name, of course), and gnome-shell will crash.

(In reply to comment #9)
> (In reply to comment #8)
> 
> > IMHO, we should not create fake outputs or fake modes - even if off, the
> > outputs are still there (and the case of a desktop pc with the vga cable
> > unplugged needs to be handled properly in meta-display-config.c). In other
> > words, all callers of get_outputs()/get_resources() need to be able the handle
> > 0 outputs.
> 
> Normally I'd agree that all code needs to be able to deal with 0 usable
> outputs.
> 
> However, *none* of the code is written to do that.
> 
> There are various parts to the problem:
> 
> 1. MetaMonitorManager provides a leaky abstraction.  It wants to present a view
> where the array of MetaMonitorInfo is the only knowledge that the rest of the
> Mutter code has into the current monitors, *BUT* it turns out that key parts of
> the code also want to access the MetaOutputs.

There are two users of MetaOutput: MetaDisplayConfig and MetaWaylandCompositor.
The latter deals with 0 outputs, the former doesn't, but
1) You can (and should) fix MetaDisplayConfig
2) If there are 0 outputs, there is nothing to configure

> 2. For example, window.c does, "give me the outputs.  Now, which output does
> this window live in?  Now, which MetaMonitor corresponds to that output?".

There is no reference to MetaMonitorManager in window.c, so I don't know where you're getting this from.

> Look at this stack trace (it's from 3.10, but the code is practically the same
> in master - it just got moved to backends/):
> 
> [...]

This is caused by 0 monitor infos, not 0 outputs.

*** Bug 736054 has been marked as a duplicate of this bug. ***

Endless also has a patch for this in our mutter fork:

https://github.com/endlessm/mutter/commit/e9fbb10cfa15cb3ce8cc17b4959b0ebdbdca7f5f

Would be nice to merge something for this at some point...

*** Bug 771212 has been marked as a duplicate of this bug. ***

gnome-shell crashes on Fedora 25 when wayland is started and all monitor connections are off (e.g., the monitor connected by cable but is switched off by the user). Then gdm session falls back to use Xorg instead of Xwayland!

When the monitor was on or in powersave mode (but not switched of by the user), then gdm starts with Xwayland. While gdm is running, Xwayland stays running, independent of the state of the monitor - it doesn't matter if it is switched off.

But when I reboot the desktop computer, e.g. remote by ssh, then it may end up with Xorg instead of Xwayland. This should be fixed!

I don't know if the problem described has exactly the same reason as the bug described in this bug, but the symptoms seem to be the same. See also fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=1419561

This bug is open nearly three years. I think it's time to fix it now!

*** Bug 780964 has been marked as a duplicate of this bug. ***

Created attachment 349706 [details] [review]
backend: Only try to center pointer when there not headless

Don't attempt to center the pointer when there is nothing to center on.

Created attachment 349707 [details] [review]
monitor-manager/kms: Don't ever set an empty screen size

The screen size determines the stage size, and the stage size may never
be empty, so when there is no configuration, or otherwise no active
outputs, set the screen size to 1x1.

Created attachment 349708 [details] [review]
monitor-manager/dummy: Never set an empty screen size

The stage must always be at least 1x1 large, so make the screen size
have the same constraints.

Created attachment 349709 [details] [review]
core/window: Don't set a preferred output when there is none

If there are no active logical monitors, don't try to dereference a
NULL one to get a preferred output winsys id. Instead just set an
invalid one.

Created attachment 349710 [details] [review]
core/screen: Make logical monitor getters handle being headless

Don't crash or return invalid indices when we are headless.

Created attachment 349711 [details] [review]
tests: Add tests for starting headless

Add tests that check that mutter can managed to start headless, i.e.
without any monitors connected.

Created attachment 349712 [details] [review]
ui: Improve handling being headless

Don't assume there will always be a primary (logical) monitor, or any
(logical) monitor at all. This includes not allocating / layouting
correctly when being headless, which would not be possible anyway,
since the stage size will be 1x1.

The initial background loading will also be delayed until there are any
(logical) monitors connected.

----

This patch was done pretty ad-hoc, by starting, fixing the issue, rince and
repeat. There are still some clutter warnings related to trying to allocate
things on a 1x1 stage, but with these I can start gnome-shell with no
connectors/crtcs and then later when they appear, the shell appears as it
should.


The mutter patches also generate warning, related to the screen being thought
to be 1x1, but things seems to work fine when properly set up.

*** Bug 780285 has been marked as a duplicate of this bug. ***

Why we are assuming 1x1 ? Can't we assume VGA (640x480) ?

(In reply to Frediano Ziglio from comment #25)
> Why we are assuming 1x1 ? Can't we assume VGA (640x480) ?

That's also an option, I guess.

Yeah, for example KDE assumes VGA

I think the problem you can have with 1x1 are that some computations could be cause negative numbers leading to even possibly division by 0 or crashes.

Review of attachment 349711 [details] [review]:

::: src/Makefile-tests.am
@@ +57,3 @@
+mutter_test_headless_start_test_SOURCES = \
+	tests/headless-start-test.c \
+	tests/headless-start-test.h \

There is no headless-start-test.h

To avoid crashing completely I have to add one more check. With that it stops crashing. Unfortunately the screen stays black till I connect a monitor

diff --git a/src/core/window.c b/src/core/window.c
index 121e48c46..5b9e38b6f 100644
--- a/src/core/window.c
+++ b/src/core/window.c
@@ -2846,6 +2846,7 @@ meta_window_is_monitor_sized (MetaWindow *window)
       MetaRectangle window_rect, monitor_rect;
 
       meta_window_get_frame_rect (window, &window_rect);
+      g_return_val_if_fail(window->monitor != NULL, FALSE);
       meta_screen_get_monitor_geometry (window->screen, window->monitor->number, &monitor_rect);
 
       if (meta_rectangle_equal (&window_rect, &monitor_rect))

(In reply to Pavel Grunt from comment #30)
>        meta_window_get_frame_rect (window, &window_rect);
> +      g_return_val_if_fail(window->monitor != NULL, FALSE);
>        meta_screen_get_monitor_geometry (window->screen,
> window->monitor->number, &monitor_rect);

If you want to support the monitor == NULL case rather than just catching code that calls meta_window_is_monitor_sized() in the no-monitor case, then don't use g_return_if_fail(). The macro is only intended to catch programmer errors (that is, calling a method with invalid preconditions), not as a shorthand for early returns. In fact, the macro is a complete no-op if G_DISABLE_CHECKS is defined.

*** Bug 783853 has been marked as a duplicate of this bug. ***

I have a Crossover 324K monitor, the monitor is 4K and looks nice. Unfortunately it has a lot of problems with firmware and it periodically stuck in some weird low-power mode. If I try to wakeup my Arch Linux in this state mutter crashes and kills Desktop Environment completely. I had the same crash as in bug 783853 and after applying a patch from that bug I see other related crash.

My understanding that I hit this (730551) bug with output modes. I would definitely love to see it fixed. A questions here:

- what is the current state of this bug? Does anyone actively work on it?
- what is ETA for fixing it?
- where I can find the latest development patch for mutter? Is there a git branch that consolidates all the patches for this bug?

I am eager to help with testing for this bug.

Any progress with this issue? I have to reboot my Linux computer almost daily to "fix" this mutter crash. Is there any quick solution/workaround for this problem?

Created attachment 357851 [details] [review]
backend: Only try to center pointer when there not headless

Don't attempt to center the pointer when there is nothing to center on.

Created attachment 357852 [details] [review]
monitor-manager: Fall back to minimum screen size of 640 x 480

When headless, we don't have any logical monitors to derive a screen
size from, but we can't set it to empty as that will cause issues with
the clutter stage, UI widget layout and other things. To avoid such
issues, just fall back to a 640 x 480 screen size when headless.

Created attachment 357853 [details] [review]
core/window: Don't set a preferred output when there is none

If there are no active logical monitors, don't try to dereference a
NULL one to get a preferred output winsys id. Instead just set an
invalid one.

Created attachment 357854 [details] [review]
core/screen: Make logical monitor getters handle being headless

Don't crash or return invalid indices when we are headless.

Created attachment 357855 [details] [review]
tests: Move out test client helper from test-runner.c

It could be useful for running other types of test clients in other
tests.

Created attachment 357856 [details] [review]
monitor: Add foreach output helper and fix foreach crtc helper

The foreach CRTC monitor mode helper incorrectly iterated over outputs
without CRTC when non-tiled modes were set on tiled monitors. This was
not expected by callers, so fix the helper to only iterate over active
outputs (that has or should have a CRTC).

The test cases uses the incorrect behaviour of the foreach CRTC helper
to check that the disabled outputs mode are set to NULL, so add a
foreach output helper and change the tests to use that instead.

Created attachment 357857 [details] [review]
wayland/output: Flush clients after creating wl_output global

In order to give the clients the best chance to bind the wl_output
before we later remove it (for example on fast hot plugs or in the test
suite), flush the client sockets after creating the global.

Created attachment 357858 [details] [review]
wayland: Don't free the Wayland display name string

We accidentally freed the Wayland display name string, meaning
retrieving it later retrieved freed memory.

Created attachment 357859 [details] [review]
window: Handle being headless better

This avoids updating state (such as position, size etc) when going
headless. Eventually, when non-headless, things will be updated again,
and not until then will we be able to update to a valid state.

Created attachment 357860 [details] [review]
tests/utils: Add test_client_quit() helper

Created attachment 357861 [details] [review]
tests: Add headless start test case

Test that mutter starts properly when there are no monitors connected
yet, and that things work when a monitor is eventually connected.

Created attachment 357862 [details] [review]
tests/monitor-unit-tests: Sleep some after each hot plug

Give clients (such as Xwayland) a chance to bind the wl_output global
before we continue, otherwise there is an significant risk that mutter
won't see the bind request until after the next hot plug which might
have destroyed the global object.

Created attachment 357863 [details] [review]
tests/monitor-unit-tests: Run a client while testing

Run a client while testing and check that it's alive when checking each
monitor configuration.

Review of attachment 357851 [details] [review]:

looks fine

Review of attachment 357852 [details] [review]:

sure

Review of attachment 357853 [details] [review]:

ok

Review of attachment 357854 [details] [review]:

++

Review of attachment 357855 [details] [review]:

sure

Review of attachment 357856 [details] [review]:

looks fine

Review of attachment 357857 [details] [review]:

::: src/wayland/meta-wayland.h
@@ +64,3 @@
                                                                         ClutterInputDevice    *source);
 
+struct wl_display *     meta_wayland_compositor_get_wl_display (MetaWaylandCompositor *compositor);

I'd prefer a meta_wayland_compositor_flush() or so

Review of attachment 357858 [details] [review]:

::: src/wayland/meta-wayland.c
@@ +368,3 @@
   if (_display_name_override)
     {
+      compositor->display_name = g_steal_pointer (&_display_name_override);

not an issue in practice but this is now theoretically leaked since we don't free it on _finalize()

the api to override also leaks if called multiple times

@@ -374,3 @@
         g_error ("Failed to create_socket");
 
-      g_clear_pointer (&_display_name_override, g_free);

the extra empty line above can go too

Review of attachment 357859 [details] [review]:

lgtm

Review of attachment 357860 [details] [review]:

++

Review of attachment 357861 [details] [review]:

ok

Review of attachment 357862 [details] [review]:

how do we handle this in the real mutter? the bind fails and clients see the new wl_output global, right? so why is that not good enough here?

Review of attachment 357863 [details] [review]:

So the sleep in the previous patch was for this test to be successful on every step?

(In reply to Rui Matos from comment #54)
> Review of attachment 357857 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland.h
> @@ +64,3 @@
>                                                                         
> ClutterInputDevice    *source);
>  
> +struct wl_display *     meta_wayland_compositor_get_wl_display
> (MetaWaylandCompositor *compositor);
> 
> I'd prefer a meta_wayland_compositor_flush() or so

That is indeed nicer.

(In reply to Rui Matos from comment #55)
> Review of attachment 357858 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland.c
> @@ +368,3 @@
>    if (_display_name_override)
>      {
> +      compositor->display_name = g_steal_pointer (&_display_name_override);
> 
> not an issue in practice but this is now theoretically leaked since we don't
> free it on _finalize()

Good point.

> 
> the api to override also leaks if called multiple times
> 

I suppose it doesn't hurt to clear, even though it makes no sense to set it after having started.

(In reply to Rui Matos from comment #59)
> Review of attachment 357862 [details] [review] [review]:
> 
> how do we handle this in the real mutter? the bind fails and clients see the
> new wl_output global, right? so why is that not good enough here?

Exactly. This is just to makes the test pass indeed, fixing it "for real" is a different story: https://phabricator.freedesktop.org/T7722

(In reply to Rui Matos from comment #60)
> Review of attachment 357863 [details] [review] [review]:
> 
> So the sleep in the previous patch was for this test to be successful on
> every step?

Right. The T7722 issue needs to be fixed, then we should be able to remove the sleep.

Attachment 357851 [details] pushed as a119e58 - backend: Only try to center pointer when there not headless
Attachment 357852 [details] pushed as 0aa7405 - monitor-manager: Fall back to minimum screen size of 640 x 480
Attachment 357853 [details] pushed as 7562eb6 - core/window: Don't set a preferred output when there is none
Attachment 357854 [details] pushed as 24c91d9 - core/screen: Make logical monitor getters handle being headless
Attachment 357855 [details] pushed as be11c32 - tests: Move out test client helper from test-runner.c
Attachment 357856 [details] pushed as 522eec0 - monitor: Add foreach output helper and fix foreach crtc helper
Attachment 357857 [details] pushed as 2a318eb - wayland/output: Flush clients after creating wl_output global
Attachment 357858 [details] pushed as dcd15e6 - wayland: Don't free the Wayland display name string too early
Attachment 357859 [details] pushed as 2df4ccd - window: Handle being headless better
Attachment 357860 [details] pushed as 5b37901 - tests/utils: Add test_client_quit() helper
Attachment 357861 [details] pushed as 3f6a2d0 - tests: Add headless start test case
Attachment 357862 [details] pushed as fa9c09f - tests/monitor-unit-tests: Sleep some after each hot plug
Attachment 357863 [details] pushed as e2d904c - tests/monitor-unit-tests: Run a client while testing

Jonas, it looks like you forgot to commit a file.

src/Makefile-tests.am references this missing file:

src/tests/headless-start-test.h

It doesn't exist, so removed from Makefile-tests.am.

Created attachment 359051 [details] [review]
wayland: Don't free non-transferred string when cleaning up

When cleaning up the display name string management, the display name
string retrieved from libwayland-server was also passed to free() on
clean up. This is invalid as the display name string ownership is not
transferred to us. Fix this by strdup:ing the string before saving it.

Review of attachment 359051 [details] [review]:

OK

Comment on attachment 359051 [details] [review]
wayland: Don't free non-transferred string when cleaning up

Attachment 359051 [details] pushed as 807658e - wayland: Don't free non-transferred string when cleaning up

Any chance to see this bugfix backported to 3.24.x?

Review of attachment 349712 [details] [review]:

This patch has never been committed to master, but it's needed or otherwise the shell will crash when booting in headless mode.

I've applied it locally to my fork of the shell for Endless (currently based on 3.24) and I can confirm it works, so maybe worth landing?

*** Bug 787900 has been marked as a duplicate of this bug. ***

Created attachment 360376 [details] [review]
Do not crash when starting up with no monitor connected

(In reply to Mario Sánchez Prada from comment #69)
> Review of attachment 349712 [details] [review] [review]:
> 
> This patch has never been committed to master, but it's needed or otherwise
> the shell will crash when booting in headless mode.
> 
> I've applied it locally to my fork of the shell for Endless (currently based
> on 3.24) and I can confirm it works, so maybe worth landing?

Actually, not so fast: an additional patch is required or a crash in meta_screen_get_monitor_index_for_rect() will happen when starting up in this mode, which I did not notice at first since I already had it applied in our downstream fork of mutter: https://github.com/endlessm/mutter/commit/65ff0e0a1a3111c051bf8f04e428974a690044da

That, together with Jonas' patch for the shell made my fork of GNOME Shell 3.24 to gracefully handle a "headless mode situation" where (1) I'd boot with no screen connected at first, then (2) connected one and see how the shell's login screen would show up and, finally, (3) I'd be able to log in as usual and use the desktop.

For that reason, I'm reopening this bug and attaching the missing piece of the puzzle as yet another patch for mutter.

Let me know what you think.

(In reply to Mario Sánchez Prada from comment #71)
> Created attachment 360376 [details] [review] [review]
> Do not crash when starting up with no monitor connected
> 
> (In reply to Mario Sánchez Prada from comment #69)
> > Review of attachment 349712 [details] [review] [review] [review]:
> > 
> > This patch has never been committed to master, but it's needed or otherwise
> > the shell will crash when booting in headless mode.
> > 
> > I've applied it locally to my fork of the shell for Endless (currently based
> > on 3.24) and I can confirm it works, so maybe worth landing?
> 
> Actually, not so fast: an additional patch is required or a crash in
> meta_screen_get_monitor_index_for_rect() will happen when starting up in
> this mode, which I did not notice at first since I already had it applied in
> our downstream fork of mutter:
> https://github.com/endlessm/mutter/commit/
> 65ff0e0a1a3111c051bf8f04e428974a690044da

Looks like some of those changes is not needed when the gnome-shell patch is applied.

> 
> That, together with Jonas' patch for the shell made my fork of GNOME Shell
> 3.24 to gracefully handle a "headless mode situation" where (1) I'd boot
> with no screen connected at first, then (2) connected one and see how the
> shell's login screen would show up and, finally, (3) I'd be able to log in
> as usual and use the desktop.
> 
> For that reason, I'm reopening this bug and attaching the missing piece of
> the puzzle as yet another patch for mutter.

> 
> Let me know what you think.

There is also bug 787637.

Created attachment 360378 [details] [review]
ui: Improve handling being headless

Don't assume there will always be a primary (logical) monitor, or any
(logical) monitor at all. This includes not allocating / layouting /
styling correctly when being headless.

The initial background loading will also be delayed until there are any
(logical) monitors connected.

Created attachment 360379 [details] [review]
window: Make should_be_on_all_workspaces() handle being headless

With those two (the ui: one replacing the other, as gnome-shell had changed some), I seem to be able to start gnome-shell headless. With those, is attachment 360376 [details] [review] needed?

(In reply to Jonas Ådahl from comment #72)
> [...]
> > Actually, not so fast: an additional patch is required or a crash in
> > meta_screen_get_monitor_index_for_rect() will happen when starting up in
> > this mode, which I did not notice at first since I already had it applied in
> > our downstream fork of mutter:
> > https://github.com/endlessm/mutter/commit/
> > 65ff0e0a1a3111c051bf8f04e428974a690044da
> 
> Looks like some of those changes is not needed when the gnome-shell patch is
> applied.

It could be, I haven't authored that patch just happened to have it applied :). But I wouldn't be surprised if there was some overlap with the other patches here.

I'll try to investigate this further
 
> > That, together with Jonas' patch for the shell made my fork of GNOME Shell
> > 3.24 to gracefully handle a "headless mode situation" where (1) I'd boot
> > with no screen connected at first, then (2) connected one and see how the
> > shell's login screen would show up and, finally, (3) I'd be able to log in
> > as usual and use the desktop.
> > 
> > For that reason, I'm reopening this bug and attaching the missing piece of
> > the puzzle as yet another patch for mutter.
> 
> > 
> > Let me know what you think.
> 
> There is also bug 787637.

That bug relates to crashes disconnecting the monitor, but I can't get those in our fork of GNOME Shell 3.24, so perhaps it's a new issue?

(In reply to Jonas Ådahl from comment #75)
> With those two (the ui: one replacing the other, as gnome-shell had changed
> some), I seem to be able to start gnome-shell headless. With those, is
> attachment 360376 [details] [review] [review] needed?

I tested your patches to mutter and the (updated) one for the shell and mutter is still crashing for me, but you're right in that not the whole patch from attachment 360376 [details] [review] is needed, as I managed to get it not crashing and the shell working fine upon connection of the monitor (after booting) by applying this subset of the patch in mutter:

  diff --git a/src/core/screen.c b/src/core/screen.c
  index b1225bc..0952293 100644
  --- a/src/core/screen.c
  +++ b/src/core/screen.c
  @@ -1462,7 +1462,11 @@ meta_screen_get_monitor_index_for_rect (MetaScreen    *screen,
   
     logical_monitor =
       meta_monitor_manager_get_logical_monitor_from_rect (monitor_manager, rect);
  -  return logical_monitor->number;
  +
  +  if (logical_monitor)
  +    return logical_monitor->number;
  +  else
  +    return -1;
   }
   
   int

  diff --git a/src/core/window.c b/src/core/window.c
  index 3be1589..fc5ffce 100644
  --- a/src/core/window.c
  +++ b/src/core/window.c
  @@ -2860,6 +2860,9 @@ meta_window_is_monitor_sized (MetaWindow *window)
     if (meta_window_is_screen_sized (window))
       return TRUE;
   
  +  if (!window->monitor)
  +    return FALSE;
  +
     if (window->override_redirect)
       {
         MetaRectangle window_rect, monitor_rect;



With that patch on top of yours, neither mutter nor the shell would crash, and the shell would run when connecting a monitor, although I found that a small patch in the shell would be nice too, not to get a backtrace due to assuming the existence of the primary monitor in messageTray.js:

  diff --git a/js/ui/messageTray.js b/js/ui/messageTray.js
  index d0cb487..1bf7005 100644
  --- a/js/ui/messageTray.js
  +++ b/js/ui/messageTray.js
  @@ -1231,7 +1231,7 @@ const MessageTray = new Lang.Class({
           if (this._notificationState == State.HIDDEN) {
               let nextNotification = this._notificationQueue[0] || null;
               if (hasNotifications && nextNotification) {
  -                let limited = this._busy || Main.layoutManager.primaryMonitor.inFullscreen;
  +                let limited = this._busy || (Main.layoutManager.primaryMonitor && 
Main.layoutManager.primaryMonitor.inFullscreen);
                   let showNextNotification = (!limited || nextNotification.forFeedback || 
nextNotification.urgency == Urgency.CRITICAL);
                   if (showNextNotification)
                       this._showNotification();
 

@Jonas What do you think? Would you be happy to incorporate those additional patches to mutter & the shell?

(In reply to Mario Sánchez Prada from comment #77)
> [...]
> @Jonas What do you think? Would you be happy to incorporate those additional
> patches to mutter & the shell?

Quick correction: the first patch of the patch (in src/core/screen.c) I'm proposing for mutter is actually part of attachment 357854 [details] [review], so forget about that one. The second one (in src/core/window.c) still applies, though, as well as the patch for the shell.

Created attachment 360416 [details] [review]
core/window: Add extra NULL-check for existent monitors

This is the patch that would be needed for mutter, besides Jonas' last patch

Created attachment 360418 [details] [review]
messageTray: Check monitor's existence when checking if it's full screen

And this would be the additional patch for the shell, also besides Jonas patch

Review of attachment 360416 [details] [review]:

Locally I had turned the if you introduced into a g_assert(), because it seemed like incorrect usage to check weather a window is monitor sized during a state without monitors. But since it looks like this could be called from JS, I eventually changed it to g_return_val_if_fail() at least trigger warnings. Where is this called from currently, with all the patches applied?

(In reply to Jonas Ådahl from comment #81)
> Review of attachment 360416 [details] [review] [review]:
> 
> Locally I had turned the if you introduced into a g_assert(), because it
> seemed like incorrect usage to check weather a window is monitor sized
> during a state without monitors. But since it looks like this could be
> called from JS, I eventually changed it to g_return_val_if_fail() at least
> trigger warnings.

Hmm... that makes sense I guess. It's possible that this is not the problem to fix, but a symptom after all.

> Where is this called from currently, with all the patches applied?

Breaking with gdb spits this out:

  #0  0x00007f16d037ae83 in meta_window_is_monitor_sized (window=0x203e410 [MetaWindowX11]) at core/window.c:2868
  #1  0x00007f16d0352f92 in meta_surface_actor_x11_should_unredirect (actor=0x280a550 [MetaSurfaceActorX11]) at compositor/meta-surface-actor-x11.c:292
  #2  0x00007f16d0348209 in meta_pre_paint_func (data=0x14ed0a0) at compositor/compositor.c:1076
  #3  0x00007f16cf6fb024 in _clutter_run_repaint_functions (flags=flags@entry=CLUTTER_REPAINT_FLAGS_PRE_PAINT) at clutter-main.c:3433
  #4  0x00007f16cf6fbcd7 in master_clock_update_stages (master_clock=0x7f16ac003740 [ClutterMasterClockDefault], stages=0x0) at clutter-master-clock-default.c:437
  #5  0x00007f16cf6fbcd7 in clutter_clock_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at clutter-master-clock-default.c:567
  #6  0x00007f16ceb9e3f7 in g_main_dispatch (context=0xe863b0) at ../../../../glib/gmain.c:3148
  #7  0x00007f16ceb9e3f7 in g_main_context_dispatch (context=context@entry=0xe863b0) at ../../../../glib/gmain.c:3813
  #8  0x00007f16ceb9e668 in g_main_context_iterate (context=0xe863b0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../../../../glib/gmain.c:3886
  #9  0x00007f16ceb9e992 in g_main_loop_run (loop=0x10c1810) at ../../../../glib/gmain.c:4082
  #10 0x00007f16d0368cfc in meta_run () at core/main.c:648
  #11 0x0000000000401fb7 in main (argc=<optimized out>, argv=<optimized out>) at main.c:462


Looks like perhaps there should be an extra check at the compositor level, maybe in meta_pre_paint_func() or meta_surface_actor_x11_should_unredirect()?

(In reply to Mario Sánchez Prada from comment #82)
> (In reply to Jonas Ådahl from comment #81)
> > Review of attachment 360416 [details] [review] [review] [review]:
> > 
> > Locally I had turned the if you introduced into a g_assert(), because it
> > seemed like incorrect usage to check weather a window is monitor sized
> > during a state without monitors. But since it looks like this could be
> > called from JS, I eventually changed it to g_return_val_if_fail() at least
> > trigger warnings.
> 
> Hmm... that makes sense I guess. It's possible that this is not the problem
> to fix, but a symptom after all.
> 
> > Where is this called from currently, with all the patches applied?
> 
> Breaking with gdb spits this out:
> 
>   #0  0x00007f16d037ae83 in meta_window_is_monitor_sized (window=0x203e410
> [MetaWindowX11]) at core/window.c:2868
>   #1  0x00007f16d0352f92 in meta_surface_actor_x11_should_unredirect
> (actor=0x280a550 [MetaSurfaceActorX11]) at
> compositor/meta-surface-actor-x11.c:292
>   #2  0x00007f16d0348209 in meta_pre_paint_func (data=0x14ed0a0) at
> compositor/compositor.c:1076
>   #3  0x00007f16cf6fb024 in _clutter_run_repaint_functions
> (flags=flags@entry=CLUTTER_REPAINT_FLAGS_PRE_PAINT) at clutter-main.c:3433
>   #4  0x00007f16cf6fbcd7 in master_clock_update_stages
> (master_clock=0x7f16ac003740 [ClutterMasterClockDefault], stages=0x0) at
> clutter-master-clock-default.c:437
>   #5  0x00007f16cf6fbcd7 in clutter_clock_dispatch (source=<optimized out>,
> callback=<optimized out>, user_data=<optimized out>) at
> clutter-master-clock-default.c:567
>   #6  0x00007f16ceb9e3f7 in g_main_dispatch (context=0xe863b0) at
> ../../../../glib/gmain.c:3148
>   #7  0x00007f16ceb9e3f7 in g_main_context_dispatch
> (context=context@entry=0xe863b0) at ../../../../glib/gmain.c:3813
>   #8  0x00007f16ceb9e668 in g_main_context_iterate (context=0xe863b0,
> block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at
> ../../../../glib/gmain.c:3886
>   #9  0x00007f16ceb9e992 in g_main_loop_run (loop=0x10c1810) at
> ../../../../glib/gmain.c:4082
>   #10 0x00007f16d0368cfc in meta_run () at core/main.c:648
>   #11 0x0000000000401fb7 in main (argc=<optimized out>, argv=<optimized
> out>) at main.c:462
> 
> 
> Looks like perhaps there should be an extra check at the compositor level,
> maybe in meta_pre_paint_func() or meta_surface_actor_x11_should_unredirect()?

That issue is fixed by ' surface-actor/x11: Don't try to unredirect when headless' in bug 787637.

Comment on attachment 360416 [details] [review]
core/window: Add extra NULL-check for existent monitors

(In reply to Jonas Ådahl from comment #83)
> [...]
> > Looks like perhaps there should be an extra check at the compositor level,
> > maybe in meta_pre_paint_func() or meta_surface_actor_x11_should_unredirect()?
> 
> That issue is fixed by ' surface-actor/x11: Don't try to unredirect when
> headless' in bug 787637.

Indeed! I'm marking my last patch for mutter as obsolete then, as I just tested this and it's not necessary as long as the patches for 787637 are applied. Thanks!

Review of attachment 360379 [details] [review]:

A more robust approach would be to change meta_window_is_on_primary_monitor() itself to something like:

  if (window->monitor == NULL)
    return FALSE;
  return window->monitor->is_primary;

Review of attachment 360378 [details] [review]:

OK

Review of attachment 360418 [details] [review]:

::: js/ui/messageTray.js
@@ +1233,3 @@
             if (hasNotifications && nextNotification) {
+                let inFullScreen = Main.layoutManager.primaryMonitor ?
+                    Main.layoutManager.primaryMonitor.inFullscreen : false;

This should fix the exception, but how about doing this instead (at the top of the function):

  let hasMonitor = Main.layoutManager.primaryMonitor != null;
  this.actor.visible = !this._bannerBlocked && hasMonitor && this._banner != null;
  if (this._bannerBlocked || !hasMonitor)
      return;

It's not like "showing" a notification is useful with no monitor attached ...

(In reply to Florian Müllner from comment #85)
> Review of attachment 360379 [details] [review] [review]:
> 
> A more robust approach would be to change
> meta_window_is_on_primary_monitor() itself to something like:
> 
>   if (window->monitor == NULL)
>     return FALSE;
>   return window->monitor->is_primary;

For a bit I had it being g_return_val_if_fail (window->monitor, FALSE); because checking if a window is on a primary monitor on a headless doesn't seem to make much sense to me.

Dunno. If a window is not on any monitors, it is clearly not on the primary. Expecting callers to differentiate between "not on the primary because on some other monitor" and "not on the primary because on no monitors" doesn't seem overly useful to me ...

(In reply to Florian Müllner from comment #89)
> Dunno. If a window is not on any monitors, it is clearly not on the primary.
> Expecting callers to differentiate between "not on the primary because on
> some other monitor" and "not on the primary because on no monitors" doesn't
> seem overly useful to me ...

It's more about the reason some code path checks this condition; the places I've seen should not have made this check on headless to begin with. Making it a (soft) assert would make it easier to catch these cases.

Review of attachment 360418 [details] [review]:

::: js/ui/messageTray.js
@@ +1233,3 @@
             if (hasNotifications && nextNotification) {
+                let inFullScreen = Main.layoutManager.primaryMonitor ?
+                    Main.layoutManager.primaryMonitor.inFullscreen : false;

Yes, that would work as well (tested). Let me update the patch...

Created attachment 360608 [details] [review]
messageTray: Check monitor's existence when checking if it's full screen

Patch updated

Review of attachment 360608 [details] [review]:

LGTM

Comment on attachment 360608 [details] [review]
messageTray: Check monitor's existence when checking if it's full screen

(In reply to Florian Müllner from comment #93)
> Review of attachment 360608 [details] [review] [review]:
> 
> LGTM

Thanks. Landed the patch at https://git.gnome.org/browse/gnome-shell/commit/?id=023b50e7a7002226d6176bede22930d96da074a9

Pushed while adding a soft-assert to ..is_on_primary_... Left for fixing the rest of the related issues are the patches in bug 787637.

Attachment 360379 [details] pushed as 2c85bb4 - window: Make should_be_on_all_workspaces() handle being headless

Attachment 360378 [details] pushed as 5c37fac - ui: Improve handling being headless

Thank you very much. With your recent changes things got better for me. Now my display wakes up from sleep state.

Unfortunately I see another gnome/mutter crash when I try to wake the monitor:

[Current thread is 1 (Thread 0x7f26b68d0240 (LWP 1461))]
(gdb) bt

+ Trace 238064