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 751638 - crash in meta_window_move_between_rects()
crash in meta_window_move_between_rects()
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
3.16.x
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-06-29 09:01 UTC by Fabrice Bellet
Modified: 2015-11-29 18:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backtrace (4.04 KB, text/plain)
2015-06-29 09:01 UTC, Fabrice Bellet
  Details
monitor-manager: Fix the max potential number of logical monitors (1.42 KB, patch)
2015-07-03 16:36 UTC, Rui Matos
committed Details | Review

Description Fabrice Bellet 2015-06-29 09:01:25 UTC
Created attachment 306267 [details]
backtrace

I'm running Fedora 22, on a laptop with a small screen size. When I need to work
remotely on this box, a use vnc with a couple of xrandr commands, that _extend_ the size of the display on-demand. Something like that :

xrandr --newmode "2560x1440" 312.25 2560 2752 3024 3488 1440 1443 1448 1493
xrandr --addmode VGA1 2560x1440

-> to enlarge display:

xrandr --output VGA1 --mode 2560x1440
xrandr --output LVDS1 --off

-> to revert back to laptop panel display

xrandr --output LVDS1 --mode 1366x768
xrandr --output VGA1 --off

This hack works fine most of the time, but sometimes it causes a segfault in gnome-shell, in mutter/src/core/window.c:3773.
Comment 1 Fabrice Bellet 2015-06-29 13:24:50 UTC
I think the problem is located in make_logical_config() from meta-monitor-manager.c, where the MetaMonitorInfo  is handled as a pointer in a GArray with &g_array_index(), while the array is growing with g_array_append_val() : array elements are reallocated, and previous elements addresses become invalid.
Comment 2 Rui Matos 2015-07-01 13:02:29 UTC
(In reply to Fabrice Bellet from comment #0)
> xrandr --newmode "2560x1440" 312.25 2560 2752 3024 3488 1440 1443 1448 1493
> xrandr --addmode VGA1 2560x1440
> 
> -> to enlarge display:
> 
> xrandr --output VGA1 --mode 2560x1440

How does this work? It doesn't for me. Do you actually have a physical monitor connected to VGA1 ?

> xrandr --output LVDS1 --off
> 
> -> to revert back to laptop panel display
> 
> xrandr --output LVDS1 --mode 1366x768
> xrandr --output VGA1 --off
> 
> This hack works fine most of the time, but sometimes it causes a segfault in
> gnome-shell, in mutter/src/core/window.c:3773.

Can you print the values on gdb's prompt when you get that trace? It'd be interesting to see which value is "off".

(In reply to Fabrice Bellet from comment #1)
> I think the problem is located in make_logical_config() from
> meta-monitor-manager.c, where the MetaMonitorInfo  is handled as a pointer
> in a GArray with &g_array_index(), while the array is growing with
> g_array_append_val() : array elements are reallocated, and previous elements
> addresses become invalid.

This can't happen because the array is pre-allocated to a size that's always greater than needed AFAICT.
Comment 3 Fabrice Bellet 2015-07-01 21:59:55 UTC
(In reply to Rui Matos from comment #2)
> (In reply to Fabrice Bellet from comment #0)
> > xrandr --newmode "2560x1440" 312.25 2560 2752 3024 3488 1440 1443 1448 1493
> > xrandr --addmode VGA1 2560x1440
> > 
> > -> to enlarge display:
> > 
> > xrandr --output VGA1 --mode 2560x1440
> 
> How does this work? It doesn't for me. Do you actually have a physical
> monitor connected to VGA1 ?

No, there's no monitor on VGA1. This is a trick to increase the framebuffer size to something larger than what LVDS1 can handle. There's no output on LVDS1 after I switched to VGA1, but I can start a x11vnc that makes this 2560x1400 framebuffer available over VNC on another box where I have a real screen of this size. Even better, x11vnc/vncviewer can handle these dimensions changes without being relaunched. Something like that should make the bug happen:

xrandr --newmode "2560x1440" 312.25 2560 2752 3024 3488 1440 1443 1448 1493
xrandr --addmode VGA1 2560x1440
for i in $(seq 50)
do
  xrandr --output VGA1 --mode 2560x1440
  xrandr --output LVDS1 --off
  sleep 2
  xrandr --output LVDS1 --mode 1366x768
  xrandr --output VGA1 --off
  sleep 2
done

Here is also the output of xrandr -q :

[bellet@bonobo ~]$ DISPLAY=:0 xrandr -q
Screen 0: minimum 8 x 8, current 1366 x 768, maximum 32767 x 32767
LVDS1 connected primary 1366x768+0+0 (normal left inverted right x axis y axis) 277mm x 156mm
   1366x768      60.02*+
   1280x720      60.00  
   1024x768      60.00  
   1024x576      60.00  
   960x540       60.00  
   800x600       60.32    56.25  
   864x486       60.00  
   640x480       59.94  
   720x405       60.00  
   680x384       60.00  
   640x360       60.00  
DP1 disconnected (normal left inverted right x axis y axis)
DP2 disconnected (normal left inverted right x axis y axis)
DP3 disconnected (normal left inverted right x axis y axis)
HDMI1 disconnected (normal left inverted right x axis y axis)
HDMI2 disconnected (normal left inverted right x axis y axis)
HDMI3 disconnected (normal left inverted right x axis y axis)
VGA1 disconnected (normal left inverted right x axis y axis)
   2560x1440     59.96  
VIRTUAL1 disconnected (normal left inverted right x axis y axis)



> 
> > xrandr --output LVDS1 --off
> > 
> > -> to revert back to laptop panel display
> > 
> > xrandr --output LVDS1 --mode 1366x768
> > xrandr --output VGA1 --off
> > 
> > This hack works fine most of the time, but sometimes it causes a segfault in
> > gnome-shell, in mutter/src/core/window.c:3773.
> 
> Can you print the values on gdb's prompt when you get that trace? It'd be
> interesting to see which value is "off".

Program received signal	SIGSEGV, Segmentation fault.
0x00007ffff296096f in meta_window_move_between_rects (window=0x272f720 [MetaWindowX11], old_area=0x3f2c3f8, new_area=0x7a4932e8) at core/windo
w.c:3772
(gdb) print new_area
$135 = (const MetaRectangle *) 0x7a4932e8
(gdb) print *old_area
$136 = {
  x = 0,
  y = 0,
  width = 1366,
  height = 768
}
(gdb) print *new_area
Cannot access memory at address 0x7a4932e8
(gdb) list
3767	  int rel_x, rel_y;
3768	  double scale_x, scale_y;
3769
3770	  rel_x = window->unconstrained_rect.x - old_area->x;
3771	  rel_y = window->unconstrained_rect.y - old_area->y;
3772	  scale_x = (double)new_area->width / old_area->width;
3773	  scale_y = (double)new_area->height / old_area->height;
3774
3775	  window->unconstrained_rect.x = new_area->x + rel_x * scale_x;
3776	  window->unconstrained_rect.y = new_area->y + rel_y * scale_y;

I tracked it down to manager->primary_monitor_index value being wrong at the
end of make_logical_config().

> 
> (In reply to Fabrice Bellet from comment #1)
> > I think the problem is located in make_logical_config() from
> > meta-monitor-manager.c, where the MetaMonitorInfo  is handled as a pointer
> > in a GArray with &g_array_index(), while the array is growing with
> > g_array_append_val() : array elements are reallocated, and previous elements
> > addresses become invalid.
> 
> This can't happen because the array is pre-allocated to a size that's always
> greater than needed AFAICT.

In my case, I have these values (preallocation size = 1):
Breakpoint 1, make_logical_config (manager=0x64a2a0 [MetaMonitorManagerXrandr]) at backends/meta-monitor-manager.c:87
(gdb) print manager->n_outputs
$1 = 1
(gdb) print manager->n_crtcs
$2 = 3

Adding some printf() in make_logical_config() gives these values while issuing xrandr commands in a loop:

n_outputs=1
appending info (0,0,2560,1440)
n_outputs=1
appending info (0,0,2560,1440)
n_outputs=1
appending info (0,0,2560,1440)
n_outputs=1
appending info (0,0,1366,768)
appending info (0,0,2560,1440)
primary_monitor_index=0
n_outputs=1
appending info (0,0,1366,768)
primary_monitor_index=0
n_outputs=1
appending info (0,0,1366,768)
appending info (0,0,2560,1440)
primary_monitor_index=66926304
Comment 4 Rui Matos 2015-07-03 16:36:34 UTC
Created attachment 306736 [details] [review]
monitor-manager: Fix the max potential number of logical monitors

The max potential number of logical monitors (i.e. MetaMonitorInfos)
is the number of CRTCs, not the number of outputs.

In cases where we have more enabled CRTCs than connected outputs we
would end up appending more MetaMonitorInfos to the GArray than the
size it was initialized with which means the array would get
re-allocated rendering invalid some MetaCRTC->logical_monitor pointers
assigned previously and thus ending in crashes later on.
Comment 5 Jasper St. Pierre (not reading bugmail) 2015-07-03 17:42:58 UTC
Review of attachment 306736 [details] [review]:

Should this probably be MAX (n_crtcs, n_outputs);?
Comment 6 Rui Matos 2015-07-04 13:45:58 UTC
(In reply to Jasper St. Pierre from comment #5)
> Should this probably be MAX (n_crtcs, n_outputs);?

I actually had it like that at first but then, thinking about the semantics of these objects, I concluded CRTCs indeed bound the amount of logical monitors we can ever have since they're the viewports into the frame buffer. And the code a bit further down actually enforces this: MetaMonitorInfos are instanced in a

for (i = 0; i < manager->n_crtcs; i++)

loop.
Comment 7 Jasper St. Pierre (not reading bugmail) 2015-07-04 16:26:04 UTC
How do we handle mirrored outputs then?
Comment 8 Rui Matos 2015-07-06 14:49:30 UTC
Mirrored outputs are driven by the same crtc, i.e. they're a single logical monitor.
Comment 9 Fabrice Bellet 2015-07-06 15:34:21 UTC
Patch from comment 4 works for me.
Comment 10 Jasper St. Pierre (not reading bugmail) 2015-07-06 20:02:06 UTC
(In reply to Rui Matos from comment #8)
> Mirrored outputs are driven by the same crtc, i.e. they're a single logical
> monitor.

Right. But in that case, we have one CRTC and two outputs. Are you sure n_crtcs is correct in that case?
Comment 11 Rui Matos 2015-07-07 12:54:01 UTC
(In reply to Jasper St. Pierre from comment #10)
> Right. But in that case, we have one CRTC and two outputs. Are you sure
> n_crtcs is correct in that case?

Sure it is, it means we have one logical monitor. The number of outputs is totally irrelevant here. One active CRTC corresponds to exactly one MetaMonitorInfo even if there's no active output connected to that CRTC.

Note that the new tiled monitor support makes the CRTC <-> MetaMonitorInfo relation N <-> 1, i.e. one MetaMonitorInfo can now contain several CRTCs which doesn't change the fact that manager->n_crtcs bounds the possible number of MetaMonitorInfos.

I can't think of any case where you'd have more logical monitors than CRTCs. Can you?
Comment 12 Rui Matos 2015-11-26 14:47:21 UTC
Can I get an ack/nack here?
Comment 13 Jasper St. Pierre (not reading bugmail) 2015-11-26 21:15:16 UTC
Review of attachment 306736 [details] [review]:

OK.
Comment 14 Rui Matos 2015-11-29 18:28:43 UTC
Attachment 306736 [details] pushed as 82bdd1e - monitor-manager: Fix the max potential number of logical monitors