Fix subsurface visibility lifecycle

* compositor.h: Update prototypes.
* dmabuf.c (MakeFeedback): Fix comment.
* shm.c (Pool): New field `flags'.
(DereferencePool, ResizePool, CreatePool): Detect whether or not
accessing a pool cannot result in SIGBUS, and refrain from
adding bus traps if that is the case.
* subcompositor.c (IsSkipped, SetSkipped, ClearSkipped): New
view state.
(SubcompositorUpdateBounds, SubcompositorUpdateBoundsForInsert)
(ViewRecomputeChildren, ViewAfterSizeUpdate, ViewMove, ViewFree)
(SubcompositorUpdate, SubcompositorExpose,
SubcompositorLookupView): Skip "skipped" views.
* subsurface.c (AfterParentCommit): Unskip views.
(Setup): Mark view as unskipped.

compositor.h

@@ -650,6 +650,8 @@ extern void ViewMove (View *, int, int);
 extern void ViewDetach (View *);
 extern void ViewMap (View *);
 extern void ViewUnmap (View *);
+extern void ViewSkip (View *);
+extern void ViewUnskip (View *);
 extern void ViewMoveFractional (View *, double, double);
 
 extern void ViewSetViewport (View *, double, double, double, double,

dmabuf.c

@@ -831,7 +831,7 @@ static struct zwp_linux_dmabuf_feedback_v1_interface zld_feedback_v1_impl =
   };
 
 /* TODO: dynamically switch tranche for surface feedbacks based on the
-   crtc of the provider the surface is in.  */
+   provider of the crtc the surface is in.  */
 
 static void
 MakeFeedback (struct wl_client *client, struct wl_resource *resource,

shm.c

@@ -21,11 +21,17 @@ along with 12to11.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <stdio.h>
 #include <string.h>
 #include <errno.h>
+#include <fcntl.h>
 
 #include <sys/mman.h>
 
 #include "compositor.h"
 
+enum
+  {
+    PoolCannotSigbus = 1,
+  };
+
 typedef struct _Pool
 {
   /* The file descriptor corresponding to this pool.  */
@@ -37,6 +43,9 @@ typedef struct _Pool
   /* The number of references to this pool.  */
   int refcount;
 
+  /* Various flags.  */
+  int flags;
+
   /* Pointer to the raw data in this pool.  */
   void *data;
 
@@ -83,7 +92,10 @@ DereferencePool (Pool *pool)
 
   if (pool->data != (void *) -1
       /* If the pool is of size 0, no busfault was installed.  */
-      && pool->size)
+      && pool->size
+      /* If reading from the pool cannot possibly cause SIGBUS, then
+	 no bus fault trap was installed.  */
+      && !(pool->flags & PoolCannotSigbus))
     XLRemoveBusfault (pool->data);
 
   close (pool->fd);
@@ -339,6 +351,10 @@ ResizePool (struct wl_client *client, struct wl_resource *resource,
 {
   Pool *pool;
   void *data;
+#ifdef F_GET_SEALS
+  int seals;
+  struct stat statb;
+#endif
 
   pool = wl_resource_get_user_data (resource);
 
@@ -365,14 +381,27 @@ ResizePool (struct wl_client *client, struct wl_resource *resource,
 
   /* Now cancel the existing bus fault handler, should it have been
      installed.  */
-  if (pool->size)
+  if (pool->size && !(pool->flags & PoolCannotSigbus))
     XLRemoveBusfault (pool->data);
 
+  pool->flags = 0;
+
+  /* Recheck whether or not reading from the pool can cause
+     SIGBUS.  */
+#ifdef F_GET_SEALS
+  seals = fcntl (pool->fd, F_GET_SEALS);
+
+  if (seals != -1 && seals & F_SEAL_SHRINK
+      && fstat (pool->fd, &statb) >= 0
+      && statb.st_size >= size)
+    pool->flags |= PoolCannotSigbus;
+#endif
+
   pool->size = size;
   pool->data = data;
 
   /* And add a new handler.  */
-  if (pool->size)
+  if (pool->size && !(pool->flags & PoolCannotSigbus))
     XLRecordBusfault (pool->data, pool->size);
 }
 
@@ -394,6 +423,18 @@ CreatePool (struct wl_client *client, struct wl_resource *resource,
 	    uint32_t id, int32_t fd, int32_t size)
 {
   Pool *pool;
+#ifdef F_GET_SEALS
+  int seals;
+  struct stat statb;
+#endif
+
+  if (size <= 0)
+    {
+      wl_resource_post_error (resource, WL_SHM_ERROR_INVALID_STRIDE,
+			      "invalid size given to create_pool");
+      close (fd);
+      return;
+    }
 
   pool = XLSafeMalloc (sizeof *pool);
 
@@ -411,6 +452,7 @@ CreatePool (struct wl_client *client, struct wl_resource *resource,
     {
       XLFree (pool);
       wl_resource_post_no_memory (resource);
+      close (fd);
 
       return;
     }
@@ -423,6 +465,7 @@ CreatePool (struct wl_client *client, struct wl_resource *resource,
       XLFree (pool);
       wl_resource_post_error (resource, WL_SHM_ERROR_INVALID_FD,
 			      "mmap: %s", strerror (errno));
+      close (fd);
 
       return;
     }
@@ -432,10 +475,23 @@ CreatePool (struct wl_client *client, struct wl_resource *resource,
 
   pool->size = size;
 
+  /* Try to determine whether or not the accessing the pool data
+     cannot result in SIGBUS, as the file is already larger (or equal
+     in size) to the pool and the size is sealed.  */
+  pool->flags = 0;
+#ifdef F_GET_SEALS
+  seals = fcntl (fd, F_GET_SEALS);
+
+  if (seals != -1 && seals & F_SEAL_SHRINK
+      && fstat (fd, &statb) >= 0
+      && statb.st_size >= size)
+    pool->flags |= PoolCannotSigbus;
+#endif
+
   /* Begin trapping SIGBUS from this pool.  The client may truncate
      the file without telling us, in which case accessing its contents
      will cause crashes.  */
-  if (pool->size)
+  if (!(pool->flags & PoolCannotSigbus) && pool->size)
     XLRecordBusfault (pool->data, pool->size);
 
   pool->fd = fd;

subcompositor.c

@@ -193,10 +193,14 @@ enum
     /* This means that the view and all its inferiors should be
        skipped in bounds computation, input tracking, et cetera.  */
     ViewIsUnmapped   = 1,
+    /* This means that the view itself (not including its inferiors)
+       should be skipped for bounds computation and input
+       tracking, etc.  */
+    ViewIsSkipped    = 1 << 2,
     /* This means that the view has a viewport specifying its size,
-       effectively decoupling its relation to the buffer width and
+       effectively decoupling its relation to the buffer width	and
        height.  */
-    ViewIsViewported = 1 << 2,
+    ViewIsViewported = 1 << 3,
   };
 
 #define IsViewUnmapped(view)			\
@@ -206,6 +210,13 @@ enum
 #define ClearUnmapped(view)			\
   ((view)->flags &= ~ViewIsUnmapped)
 
+#define IsSkipped(view)				\
+  ((view)->flags & ViewIsSkipped)
+#define SetSkipped(view)			\
+  ((view)->flags |= ViewIsSkipped)
+#define ClearSkipped(view)			\
+  ((view)->flags &= ~ViewIsSkipped)
+
 #define IsViewported(view)			\
   ((view)->flags & ViewIsViewported)
 #define SetViewported(view)			\
@@ -528,6 +539,10 @@ SubcompositorUpdateBounds (Subcompositor *subcompositor, int doflags)
 	      goto next;
 	    }
 
+	  if (IsSkipped (list->view))
+	    /* Skip past the view itself should it be skipped.  */
+	    goto next;
+
 	  if ((doflags & DoMinX) && min_x > list->view->abs_x)
 	    min_x = list->view->abs_x;
 
@@ -566,7 +581,7 @@ SubcompositorUpdateBoundsForInsert (Subcompositor *subcompositor,
 {
   XLAssert (view->subcompositor == subcompositor);
 
-  if (!ViewIsMapped (view))
+  if (!ViewIsMapped (view) || IsSkipped (view))
     /* If the view is unmapped, do nothing.  */
     return;
 
@@ -718,7 +733,9 @@ ViewRecomputeChildren (View *view, int *doflags)
 	      && attached
 	      /* Or if it isn't mapped, or none of its parents are
 		 mapped.  */
-	      && mapped)
+	      && mapped
+	      /* Or if it is skipped.  */
+	      && !IsSkipped (view))
 	    {
 	      if (child->abs_x < view->subcompositor->min_x)
 		{
@@ -1242,7 +1259,7 @@ ViewAfterSizeUpdate (View *view)
   Bool mapped;
 
   if (!view->subcompositor || !ViewVisibilityState (view, &mapped)
-      || !mapped)
+      || !mapped || IsSkipped (view))
     return;
 
   /* First, assume we will have to compute both max_x and max_y.  */
@@ -1354,9 +1371,9 @@ ViewMove (View *view, int x, int y)
 	}
 
       if (view->subcompositor && ViewVisibilityState (view, &mapped)
-	  /* If this view isn't mapped, then do nothing.  The bounds
+	  /* If this view isn't mapped or is skipped, then do nothing.
-	     will be recomputed later.  */
+	     The bounds will be recomputed later.  */
-	  && mapped)
+	  && mapped && !IsSkipped (view))
 	{
 	  /* First assume everything will have to be updated.  */
 	  doflags |= DoMaxX | DoMaxY | DoMinY | DoMinX;
@@ -1491,6 +1508,52 @@ ViewUnmap (View *view)
     }
 }
 
+void
+ViewUnskip (View *view)
+{
+  if (!IsSkipped (view))
+    return;
+
+  ClearSkipped (view);
+
+  if (view->subcompositor && view->buffer)
+    {
+      /* Garbage the subcompositor and recompute bounds, if something
+	 is attached to the view.  */
+      SetGarbaged (view->subcompositor);
+      SubcompositorUpdateBounds (view->subcompositor, DoAll);
+    }
+}
+
+void
+ViewSkip (View *view)
+{
+  if (IsSkipped (view))
+    return;
+
+  /* Mark the view as skipped.  */
+  SetSkipped (view);
+
+  if (view->subcompositor)
+    {
+      /* Mark the subcompositor as having unmapped or skipped
+	 views.  */
+      SetPartiallyMapped (view->subcompositor);
+
+      /* If nothing is attached, the subcompositor need not be
+	 garbaged.  */
+      if (view->buffer)
+	{
+	  /* Recompute the bounds of the subcompositor.  */
+	  SubcompositorUpdateBounds (view->subcompositor,
+				     DoAll);
+
+	  /* Garbage the view's subcompositor.  */
+	  SetGarbaged (view->subcompositor);
+	}
+    }
+}
+
 void
 ViewFree (View *view)
 {
@@ -1969,6 +2032,15 @@ SubcompositorUpdate (Subcompositor *subcompositor)
 	      goto next;
 	    }
 
+	  if (IsSkipped (view))
+	    {
+	      /* We must skip this view, as it represents (for
+		 instance) a subsurface that has been added, but not
+		 committed.  */
+	      SetPartiallyMapped (subcompositor);
+	      goto next;
+	    }
+
 	  if (!view->buffer)
 	    goto next;
 
@@ -2225,6 +2297,15 @@ SubcompositorUpdate (Subcompositor *subcompositor)
 	  goto next_1;
 	}
 
+      if (IsSkipped (view))
+	{
+	  /* We must skip this view, as it represents (for
+	     instance) a subsurface that has been added, but not
+	     committed.  */
+	  SetPartiallyMapped (subcompositor);
+	  goto next_1;
+	}
+
       if (!view->buffer)
 	goto next_1;
 
@@ -2539,6 +2620,15 @@ SubcompositorExpose (Subcompositor *subcompositor, XEvent *event)
 	  goto next;
 	}
 
+      if (IsSkipped (list->view))
+	{
+	  /* We must skip this view, as it represents (for
+	     instance) a subsurface that has been added, but not
+	     committed.  */
+	  SetPartiallyMapped (subcompositor);
+	  goto next;
+	}
+
       if (!list->view->buffer)
 	goto next;
 
@@ -2669,6 +2759,14 @@ SubcompositorLookupView (Subcompositor *subcompositor, int x, int y,
 	  continue;
 	}
 
+      if (IsSkipped (list->view))
+	{
+	  /* We must skip this view, as it represents (for instance) a
+	     subsurface that has been added, but not committed.  */
+	  SetPartiallyMapped (subcompositor);
+	  continue;
+	}
+
       if (!list->view->buffer)
 	continue;
 

subsurface.c

@@ -593,8 +593,12 @@ AfterParentCommit (Surface *surface, void *data)
 	 the scanout area of.  */
       MaybeUpdateOutputs (subsurface);
     }
-  subsurface->pending_commit = False;
 
+  /* Mark the subsurface as unskipped.  (IOW, make it visible).  */
+  ViewUnskip (subsurface->role.surface->view);
+  ViewUnskip (subsurface->role.surface->under);
+
+  subsurface->pending_commit = False;
   subsurface->pending_substate.flags = 0;
 }
 
@@ -720,6 +724,22 @@ Setup (Surface *surface, Role *role)
     = XLListPrepend (subsurface->parent->subsurfaces,
 		     surface);
 
+  /* And mark the view as "skipped"; this differs from unmapping,
+     which we cannot simply use, in that children remain visible, as
+     the specification says the following:
+
+       Adding sub-surfaces to a parent is a double-buffered operation
+       on the parent (see wl_surface.commit).  The effect of adding a
+       sub-surface becomes visible on the next time the state of the
+       parent surface is applied.
+
+    So if a child is added to a desynchronized subsurface whose parent
+    toplevel has not yet committed, and commit is called on the
+    desynchronized subsurface, the child should become indirectly
+    visible on the parent toplevel through the child.  */
+  ViewSkip (surface->view);
+  ViewSkip (surface->under);
+
   return True;
 }