Update subsurface tests to handle reparenting and fix discovered bugs

* 12to11-test.xml (test_manager) <set_buffer_label>: New
request.
* 12to11.man:
* README: Add missing documentation.
* buffer.c (ExtBufferDestroy): Free buffer label.
* compositor.h (struct _ExtBuffer): New field `label'
* linux-dmabuf-unstable-v1.xml: Update from wayland-protocols.
* subcompositor.c (IsSkipped, SetSkipped, ClearSkipped): Delete
macros.
(SubcompositorUpdateBounds, SubcompositorUpdateBoundsForInsert)
(SkipSlug): Adjust accordingly.
(DamageIncludingInferiors): Fix function.
(SubcompositorInsert, SubcompositorInsertBefore)
(SubcompositorInsertAfter, ViewIsVisible, ViewRecomputeChildren)
(ViewInsert, ViewInsertAfter, ViewInsertBefore): Call
DamageIncludingInferiors on child, not view.
(ViewSetSubcompositor, ViewAfterSizeUpdate, ViewMove): Get rid
of "skipped" state.
(ViewUnskip, ViewSkip): delete functions.
(ViewFree, DoCull, SubcompositorLookupView): Get rid of
"skipped" state.
* subsurface.c (struct _Subsurface): New field `pending'.
(AfterParentCommit): Attach views whenever pending.
(Setup): Stop attaching views upon setup.
(Teardown): Only detach views when not pending.
(GetSubsurface): Add comment.
(XLSubsurfaceParentDestroyed): Set subcompositor to NULL when
parent is destroyed.
(XLSubsurfaceHandleParentCommit): New function.
* surface.c (RunCommitCallbacks): Run commit callbacks in the
order in which they were created.
(NotifySubsurfaceDestroyed): Assert that a role is present.
(HandleSurfaceDestroy): Clear subsurfaces before releasing role.
Set subsurfaces to NULL.
* test.c (SetBufferLabel): New function.
(test_manager_impl): Implement.
* tests/subsurface_test.c (enum test_kind): New
SUBSURFACE_REPARENT_KIND.
(test_names): Add names
(LAST_TEST): Set to SUBSURFACE_REPARENT_KIND.
(test_single_step): Implement new test.
* tests/test_harness.c (load_png_image): Set buffer debug label.

12to11-test.xml

@@ -110,6 +110,15 @@
       </description>
     </request>
 
+    <request name="set_buffer_label">
+      <description summary="set buffer label">
+	Set the label of the given buffer to the specified string.
+	The label is used only for debugging purposes.
+      </description>
+      <arg name="buffer" type="object" interface="wl_buffer"/>
+      <arg name="label" type="string"/>
+    </request>
+
     <event name="display_string">
       <description summary="X server name">
 	The display_string event sends the name of the X display to

12to11.man

@@ -276,6 +276,7 @@ wp_single_pixel_buffer_manager_v1	1
 zwp_pointer_constraints_v1	1
 zwp_relative_pointer_manager	1
 zwp_idle_inhibit_manager_v1	1
+xdg_activation_v1	1
 .TE
 .PP
 When the protocol translator is built with EGL support, the following

README

@@ -68,6 +68,7 @@ complete degree:
   'zwp_pointer_constraints_v1',                 version:  1
   'zwp_relative_pointer_manager_v1',            version:  1
   'zwp_idle_inhibit_manager_v1',                version:  1
+  'xdg_activation_v1',                          version:  1
 
 When built with EGL, the following Wayland protocol is also supported:
 

buffer.c

@@ -116,6 +116,9 @@ ExtBufferDestroy (ExtBuffer *buffer)
       item->func (buffer, item->data);
     }
 
+  /* Free the label if present.  */
+  XLFree (buffer->label);
+
   /* Not very efficient, since the list is followed through twice, but
      destroy listener lists should always be small.  */
   XLListFree (buffer->destroy_listeners, XLFree);

compositor.h

@@ -729,6 +729,9 @@ struct _ExtBuffer
   /* Functions for this buffer.  */
   ExtBufferFuncs funcs;
 
+  /* Label used for debugging.  */
+  char *label;
+
   /* List of destroy listeners.  */
   XLList *destroy_listeners;
 };
@@ -834,8 +837,6 @@ 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 ViewSetTransform (View *, BufferTransform);

linux-dmabuf-unstable-v1.xml

@@ -413,16 +413,16 @@
       configuration. In particular, compositors should avoid sending the exact
       same parameters multiple times in a row.
 
-      The tranche_target_device and tranche_modifier events are grouped by
+      The tranche_target_device and tranche_formats events are grouped by
       tranches of preference. For each tranche, a tranche_target_device, one
-      tranche_flags and one or more tranche_modifier events are sent, followed
+      tranche_flags and one or more tranche_formats events are sent, followed
       by a tranche_done event finishing the list. The tranches are sent in
       descending order of preference. All formats and modifiers in the same
       tranche have the same preference.
 
       To send parameters, the compositor sends one main_device event, tranches
       (each consisting of one tranche_target_device event, one tranche_flags
-      event, tranche_modifier events and then a tranche_done event), then one
+      event, tranche_formats events and then a tranche_done event), then one
       done event.
     </description>
 
@@ -495,9 +495,9 @@
 
     <event name="tranche_done">
       <description summary="a preference tranche has been sent">
-        This event splits tranche_target_device and tranche_modifier events in
+        This event splits tranche_target_device and tranche_formats events in
         preference tranches. It is sent after a set of tranche_target_device
-        and tranche_modifier events; it represents the end of a tranche. The
+        and tranche_formats events; it represents the end of a tranche. The
         next tranche will have a lower preference.
       </description>
     </event>

subcompositor.c

@@ -165,17 +165,13 @@ 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
        height.  */
-    ViewIsViewported		= 1 << 3,
+    ViewIsViewported		= 1 << 2,
     /* Whether or not damage can be trusted.  When set, non-buffer
        damage cannot be trusted, as the view transform changed.  */
-    ViewIsPreviouslyTransformed = 1 << 4,
+    ViewIsPreviouslyTransformed = 1 << 3,
   };
 
 #define IsViewUnmapped(view)			\
@@ -185,13 +181,6 @@ 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)			\
@@ -678,10 +667,6 @@ 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;
 
@@ -735,7 +720,7 @@ SubcompositorUpdateBoundsForInsert (Subcompositor *subcompositor,
 {
   XLAssert (view->subcompositor == subcompositor);
 
-  if (!ViewIsMapped (view) || IsSkipped (view))
+  if (!ViewIsMapped (view))
     /* If the view is unmapped, do nothing.  */
     return;
 
@@ -794,15 +779,6 @@ SubcompositorSetTarget (Subcompositor *compositor,
 	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;						\
 								\
@@ -846,12 +822,13 @@ DamageIncludingInferiors (View *parent)
   View *view;
   Subcompositor *subcompositor;
 
-  if (parent->subcompositor)
+  if (!parent->subcompositor)
     /* No subcompositor is attached... */
     return;
 
-  pixman_region32_union_rect (&parent->damage, &parent->damage,
+  /* Ignore unmapped views.  */
-			      0, 0, parent->width, parent->height);
+  if (IsViewUnmapped (parent))
+    return;
 
   /* Now, damage each inferior.  */
   list = parent->link;
@@ -863,8 +840,7 @@ DamageIncludingInferiors (View *parent)
 
       /* Union the view damage with its bounds.  */
       pixman_region32_union_rect (&view->damage, &view->damage,
-				  view->abs_x, view->abs_y,
+				  0, 0, view->width, view->height);
-				  view->width, view->height);
 
     next:
 
@@ -886,6 +862,13 @@ SubcompositorInsert (Subcompositor *compositor, View *view)
   ListRelinkBefore (view->link, view->inferior,
 		    compositor->last);
 
+  /* We don't know whether or not the subcompositor is partially
+     mapped.  Set the IsPartiallyMapped flag if the view has children;
+     it will be cleared upon the next update if the subcompositor is
+     not partially mapped.  */
+  if (IsViewUnmapped (view) || view->link != view->inferior)
+    SetPartiallyMapped (compositor);
+
   /* And update bounds.  */
   SubcompositorUpdateBoundsForInsert (compositor, view);
 
@@ -906,6 +889,13 @@ SubcompositorInsertBefore (Subcompositor *compositor, View *view,
   /* Make view's inferiors part of the compositor.  */
   ListRelinkBefore (view->link, view->inferior, sibling->link);
 
+  /* We don't know whether or not the subcompositor is partially
+     mapped.  Set the IsPartiallyMapped flag if the view has children;
+     it will be cleared upon the next update if the subcompositor is
+     not partially mapped.  */
+  if (IsViewUnmapped (view) || view->link != view->inferior)
+    SetPartiallyMapped (compositor);
+
   /* And update bounds.  */
   SubcompositorUpdateBoundsForInsert (compositor, view);
 
@@ -925,6 +915,13 @@ SubcompositorInsertAfter (Subcompositor *compositor, View *view,
   /* Make view's inferiors part of the compositor.  */
   ListRelinkAfter (view->link, view->inferior, sibling->inferior);
 
+  /* We don't know whether or not the subcompositor is partially
+     mapped.  Set the IsPartiallyMapped flag if the view has children;
+     it will be cleared upon the next update if the subcompositor is
+     not partially mapped.  */
+  if (IsViewUnmapped (view) || view->link != view->inferior)
+    SetPartiallyMapped (compositor);
+
   /* And update bounds.  */
   SubcompositorUpdateBoundsForInsert (compositor, view);
 
@@ -963,9 +960,6 @@ ViewIsVisible (View *view)
   if (!ViewVisibilityState (view, &mapped))
     return False;
 
-  if (IsSkipped (view))
-    return False;
-
   return mapped;
 }
 
@@ -996,9 +990,7 @@ 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)
 		{
@@ -1075,8 +1067,14 @@ ViewInsert (View *view, View *child)
 	parent->inferior = child->inferior;
     }
 
-  /* Now that the view hierarchy has been changed, garbage the
+  /* We don't know whether or not the subcompositor is partially
-     subcompositor.  */
+     mapped.  Set the IsPartiallyMapped flag if the view has children;
+     it will be cleared upon the next update if the subcompositor is
+     not partially mapped.  */
+  if (view->subcompositor
+      && (IsViewUnmapped (child)
+	  || child->link != child->inferior))
+    SetPartiallyMapped (view->subcompositor);
 
   /* Also update the absolute positions of the child.  */
   child->abs_x = view->abs_x + child->x;
@@ -1088,9 +1086,9 @@ ViewInsert (View *view, View *child)
 
   /* Now, if the subcompositor is still not garbaged, damage each
      inferior of the view.  */
-  if (view->subcompositor
+  if (child->subcompositor
-      && !IsGarbaged (view->subcompositor))
+      && !IsGarbaged (child->subcompositor))
-    DamageIncludingInferiors (view);
+    DamageIncludingInferiors (child);
 }
 
 void
@@ -1128,6 +1126,15 @@ ViewInsertAfter (View *view, View *child, View *sibling)
 	}
     }
 
+  /* We don't know whether or not the subcompositor is partially
+     mapped.  Set the IsPartiallyMapped flag if the view has children;
+     it will be cleared upon the next update if the subcompositor is
+     not partially mapped.  */
+  if (view->subcompositor
+      && (IsViewUnmapped (child)
+	  || child->link != child->inferior))
+    SetPartiallyMapped (view->subcompositor);
+
   /* Also update the absolute positions of the child.  */
   child->abs_x = view->abs_x + child->x;
   child->abs_y = view->abs_y + child->y;
@@ -1138,9 +1145,9 @@ ViewInsertAfter (View *view, View *child, View *sibling)
 
   /* Now, if the subcompositor is still not garbaged, damage each
      inferior of the view.  */
-  if (view->subcompositor
+  if (child->subcompositor
-      && !IsGarbaged (view->subcompositor))
+      && !IsGarbaged (child->subcompositor))
-    DamageIncludingInferiors (view);
+    DamageIncludingInferiors (child);
 }
 
 void
@@ -1156,6 +1163,15 @@ ViewInsertBefore (View *view, View *child, View *sibling)
   ListRelinkBefore (child->link, child->inferior,
 		    sibling->link);
 
+  /* We don't know whether or not the subcompositor is partially
+     mapped.  Set the IsPartiallyMapped flag if the view has children;
+     it will be cleared upon the next update if the subcompositor is
+     not partially mapped.  */
+  if (view->subcompositor
+      && (IsViewUnmapped (child)
+	  || child->link != child->inferior))
+    SetPartiallyMapped (view->subcompositor);
+
   /* Also update the absolute positions of the child.  */
   child->abs_x = view->abs_x + child->x;
   child->abs_y = view->abs_y + child->y;
@@ -1167,9 +1183,9 @@ ViewInsertBefore (View *view, View *child, View *sibling)
 
   /* Now, if the subcompositor is still not garbaged, damage each
      inferior of the view.  */
-  if (view->subcompositor
+  if (child->subcompositor
-      && !IsGarbaged (view->subcompositor))
+      && !IsGarbaged (child->subcompositor))
-    DamageIncludingInferiors (view);
+    DamageIncludingInferiors (child);
 
   /* Inserting inferiors before a sibling can never bump the inferior
      pointer.  */
@@ -1280,7 +1296,7 @@ ViewSetSubcompositor (View *view, Subcompositor *subcompositor)
 
       list = list->next;
     }
-  while (list != view->link);
+  while (list != view->inferior);
 }
 
 
@@ -1308,7 +1324,7 @@ ViewAfterSizeUpdate (View *view)
   view->height = ViewHeight (view);
 
   if (!view->subcompositor || !ViewVisibilityState (view, &mapped)
-      || !mapped || IsSkipped (view))
+      || !mapped)
     return;
 
   /* First, assume we will have to compute both max_x and max_y.  */
@@ -1433,7 +1449,7 @@ ViewMove (View *view, int x, int y)
       if (view->subcompositor && ViewVisibilityState (view, &mapped)
 	  /* If this view isn't mapped or is skipped, then do nothing.
 	     The bounds will be recomputed later.  */
-	  && mapped && !IsSkipped (view))
+	  && mapped)
 	{
 	  /* First assume everything will have to be updated.  */
 	  doflags |= DoMaxX | DoMaxY | DoMinY | DoMinX;
@@ -1613,50 +1629,6 @@ ViewUnmap (View *view)
   pixman_region32_fini (&damage);
 }
 
-void
-ViewUnskip (View *view)
-{
-  if (!IsSkipped (view))
-    return;
-
-  ClearSkipped (view);
-
-  if (view->subcompositor && view->buffer)
-    /* Damage the whole view bounds.  */
-    pixman_region32_union_rect (&view->damage, &view->damage,
-				view->abs_x, view->abs_y,
-				view->width, view->height);
-}
-
-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)
 {
@@ -2408,11 +2380,6 @@ DoCull (Subcompositor *subcompositor, pixman_region32_t *damage,
 	/* Skip the unmapped view.  */
 	goto last;
 
-      if (IsSkipped (list->view))
-	/* We must skip this view, as it represents (for instance) a
-	   subsurface that has been added, but not committed.  */
-	goto last;
-
       if (!list->view->buffer)
 	goto last;
 
@@ -3200,14 +3167,6 @@ 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

@@ -102,6 +102,10 @@ struct _Subsurface
   /* Whether or not this subsurface is mapped.  */
   Bool mapped;
 
+  /* Whether or not this subsurface was just added to a parent that
+     has not yet committed.  */
+  Bool pending;
+
   /* The last dimensions and position that were used to update this
      surface's outputs.  */
   int output_x, output_y, output_width, output_height;
@@ -714,12 +718,27 @@ AfterParentCommit (Surface *surface, void *data)
       MoveFractional (subsurface);
     }
 
-  /* Mark the subsurface as unskipped.  (IOW, make it visible).  This
+  /* Attach the views to the subcompositor if they have not yet been
-     must come before XLCommitSurface, as doing so will apply the
+     attached, as the parent's state has been applied.  This must come
-     pending state, which will fail to update the subcompositor bounds
+     before XLCommitSurface, as doing so will apply the pending state,
-     if the subsurface is skipped.  */
+     which will fail to update the subcompositor bounds if the
-  ViewUnskip (subsurface->role.surface->view);
+     subsurface is not present.  */
-  ViewUnskip (subsurface->role.surface->under);
+
+  if (subsurface->pending)
+    {
+      /* Set the subcompositor here.  If the role providing the
+	 subcompositor hasn't been attached to the parent, then when
+	 it is it will call ViewSetSubcompositor on the parent's
+	 view.  */
+
+      ViewSetSubcompositor (subsurface->role.surface->under,
+			    ViewGetSubcompositor (surface->view));
+      ViewInsert (surface->view, subsurface->role.surface->under);
+      ViewSetSubcompositor (subsurface->role.surface->view,
+			    ViewGetSubcompositor (surface->view));
+      ViewInsert (surface->view, subsurface->role.surface->view);
+      subsurface->pending = False;
+    }
 
   /* And any cached surface state too.  */
   if (subsurface->pending_commit)
@@ -820,7 +839,6 @@ static Bool
 Setup (Surface *surface, Role *role)
 {
   Subsurface *subsurface;
-  View *parent_view;
 
   surface->role_type = SubsurfaceType;
 
@@ -830,17 +848,6 @@ Setup (Surface *surface, Role *role)
   subsurface->output_x = INT_MIN;
   subsurface->output_y = INT_MIN;
   role->surface = surface;
-  parent_view = subsurface->parent->view;
-
-  /* Set the subcompositor here.  If the role providing the
-     subcompositor hasn't been attached to the parent, then when it is
-     it will call ViewSetSubcompositor on the parent's view.  */
-  ViewSetSubcompositor (surface->under,
-			ViewGetSubcompositor (parent_view));
-  ViewInsert (parent_view, surface->under);
-  ViewSetSubcompositor (surface->view,
-			ViewGetSubcompositor (parent_view));
-  ViewInsert (parent_view, surface->view);
 
   /* Now move the subsurface to its initial location (0, 0) */
   MoveFractional (subsurface);
@@ -850,21 +857,18 @@ Setup (Surface *surface, Role *role)
     = XLListPrepend (subsurface->parent->subsurfaces,
 		     surface);
 
-  /* And mark the view as "skipped"; this differs from unmapping,
+  /* And mark the subsurface as pending.  A pending subsurface is not
-     which we cannot simply use, in that children remain visible, as
+     inserted into any subcompositor, but will be inserted upon the
-     the specification says the following:
+     parent commit callback being run.
 
-       Adding sub-surfaces to a parent is a double-buffered operation
+     The specification states that the "effect of adding a subsurface"
-       on the parent (see wl_surface.commit).  The effect of adding a
+     will take effect after its parent is applied.
-       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
+     The interpretation previously used was that the the subsurface
-    toplevel has not yet committed, and commit is called on the
+     would be made visible upon the parent's state being applied.  But
-    desynchronized subsurface, the child should become indirectly
+     that interpretation led to ambiguities, and contradicted with
-    visible on the parent toplevel through the child.  */
+     common sense and the implementation in Weston.  */
-  ViewSkip (surface->view);
+  subsurface->pending = True;
-  ViewSkip (surface->under);
 
   /* Subsurfaces are synchronous by default.  Make every child
      synchronous.  */
@@ -913,10 +917,19 @@ Teardown (Surface *surface, Role *role)
     {
       subcompositor = ViewGetSubcompositor (surface->view);
 
+      /* Assert that the subcompositor is NULL if the subsurface is
+	 pending.  */
+      XLAssert (!subsurface->pending || !subcompositor);
+
+      if (subcompositor)
+	{
+	  /* Detach the views if the subcompositor is set.  */
+
 	  ViewUnparent (surface->view);
 	  ViewSetSubcompositor (surface->view, NULL);
 	  ViewUnparent (surface->under);
 	  ViewSetSubcompositor (surface->under, NULL);
+	}
 
       client = XLSurfaceFindClientData (subsurface->parent,
 					SubsurfaceData);
@@ -1074,6 +1087,9 @@ GetSubsurface (struct wl_client *client, struct wl_resource *resource,
   subsurface->role.funcs.parent_rescale = ParentRescale;
 
   subsurface->parent = parent;
+
+  /* Note that for subsurfaces to be attached in the correct order,
+     commit callbacks must be run in the order they were created.  */
   subsurface->commit_callback
     = XLSurfaceRunAtCommit (parent, AfterParentCommit, subsurface);
   subsurface->synchronous = True;
@@ -1135,6 +1151,12 @@ XLSubsurfaceParentDestroyed (Role *role)
 
   if (subsurface->role.surface)
     {
+      /* Set the subcompositor to NULL, as it may no longer be
+	 present.  */
+      ViewSetSubcompositor (subsurface->role.surface->view,
+			    NULL);
+      ViewSetSubcompositor (subsurface->role.surface->under,
+			    NULL);
       ViewUnparent (subsurface->role.surface->view);
       ViewUnparent (subsurface->role.surface->under);
     }
@@ -1145,6 +1167,9 @@ XLSubsurfaceHandleParentCommit (Surface *parent)
 {
   SurfaceActionClientData *client;
 
+  /* Note that these actions will also work for pending subsurfaces,
+     as they will be attached by the time this is called.  */
+
   client = XLSurfaceFindClientData (parent, SubsurfaceData);
 
   if (client)

surface.c

@@ -116,12 +116,15 @@ RunCommitCallbacks (Surface *surface)
   CommitCallback *callback;
 
   /* first is a sentinel node.  */
-  callback = surface->commit_callbacks.next;
+  callback = surface->commit_callbacks.last;
 
+  /* Run commit callbacks in the order that they were created in.  The
+     subsurface code relies on this for subsurfaces to be confirmed in
+     the right order.  */
   while (callback != &surface->commit_callbacks)
     {
       callback->commit (surface, callback->data);
-      callback = callback->next;
+      callback = callback->last;
     }
 }
 
@@ -1374,7 +1377,8 @@ NotifySubsurfaceDestroyed (void *data)
 
   surface = data;
 
-  if (surface->role)
+  /* If a surface is in the subsurfaces list, it must have a role.  */
+  XLAssert (surface->role != NULL);
   XLSubsurfaceParentDestroyed (surface->role);
 }
 
@@ -1386,6 +1390,12 @@ HandleSurfaceDestroy (struct wl_resource *resource)
 
   surface = wl_resource_get_user_data (resource);
 
+  /* Free all subsurfaces.  This must come before the subcompositor is
+     destroyed.  */
+  XLListFree (surface->subsurfaces,
+	      NotifySubsurfaceDestroyed);
+  surface->subsurfaces = NULL;
+
   if (surface->role)
     XLSurfaceReleaseRole (surface, surface->role);
 
@@ -1394,10 +1404,6 @@ HandleSurfaceDestroy (struct wl_resource *resource)
      be available in unmap callbacks.  */
   surface->resource = NULL;
 
-  /* First, free all subsurfaces.  */
-  XLListFree (surface->subsurfaces,
-	      NotifySubsurfaceDestroyed);
-
   /* Then release all client data.  */
   data = surface->client_data;
 

test.c

@@ -577,12 +577,25 @@ GetSerial (struct wl_client *client, struct wl_resource *resource)
   test_manager_send_serial (resource, serial);
 }
 
+static void
+SetBufferLabel (struct wl_client *client, struct wl_resource *resource,
+		struct wl_resource *buffer_resource, const char *label)
+{
+  ExtBuffer *buffer;
+
+  buffer = wl_resource_get_user_data (buffer_resource);
+
+  XLFree (buffer->label);
+  buffer->label = XLStrdup (label);
+}
+
 static const struct test_manager_interface test_manager_impl =
   {
     .get_test_surface = GetTestSurface,
     .get_scale_lock = GetScaleLock,
     .get_test_seat = GetTestSeat,
     .get_serial = GetSerial,
+    .set_buffer_label = SetBufferLabel,
   };
 
 

tests/subsurface_test.c

@@ -35,6 +35,7 @@ enum test_kind
     SUBSURFACE_DESYNC_KIND,
     SUBSURFACE_COMPLEX_DAMAGE_KIND,
     SUBSURFACE_SCALE_KIND,
+    SUBSURFACE_REPARENT_KIND,
   };
 
 static const char *test_names[] =
@@ -50,6 +51,7 @@ static const char *test_names[] =
     "subsurface_desync",
     "subsurface_complex_damage",
     "subsurface_scale",
+    "subsurface_reparent",
   };
 
 struct test_subsurface
@@ -61,7 +63,7 @@ struct test_subsurface
   struct wl_surface *surface;
 };
 
-#define LAST_TEST       SUBSURFACE_SCALE_KIND
+#define LAST_TEST       SUBSURFACE_REPARENT_KIND
 
 /* The display.  */
 static struct test_display *display;
@@ -83,7 +85,7 @@ static struct test_surface *test_surface;
 static struct wl_surface *wayland_surface;
 
 /* Various subsurfaces.  */
-static struct test_subsurface *subsurfaces[9];
+static struct test_subsurface *subsurfaces[11];
 
 /* Various buffers.  */
 static struct wl_buffer *tiny_png;
@@ -98,6 +100,8 @@ static struct wl_buffer *big_png;
 static struct wl_buffer *small_png;
 static struct wl_buffer *subsurface_1_complex_png;
 static struct wl_buffer *subsurface_transparency_damage_png;
+static struct wl_buffer *subsurface_stack_1_png;
+static struct wl_buffer *subsurface_stack_2_png;
 
 /* The test image ID.  */
 static uint32_t current_test_image;
@@ -209,6 +213,26 @@ make_test_subsurface_with_parent (struct test_subsurface *parent)
   return NULL;
 }
 
+static void
+delete_subsurface_role (struct test_subsurface *subsurface)
+{
+  wl_subsurface_destroy (subsurface->subsurface);
+  subsurface->subsurface = NULL;
+}
+
+static void
+recreate_subsurface (struct test_subsurface *subsurface,
+		     struct wl_surface *parent)
+{
+  subsurface->subsurface
+    = wl_subcompositor_get_subsurface (subcompositor,
+				       subsurface->surface,
+				       parent);
+
+  if (!subsurface->subsurface)
+    report_test_failure ("failed to recreate subsurface");
+}
+
 static void
 test_single_step (enum test_kind kind)
 {
@@ -731,6 +755,7 @@ test_single_step (enum test_kind kind)
       wl_surface_set_buffer_transform (subsurfaces[8]->surface,
 				       WL_OUTPUT_TRANSFORM_NORMAL);
       wl_surface_attach (subsurfaces[8]->surface, small_png, 0, 0);
+      wl_surface_damage (subsurfaces[8]->surface, 0, 0, 150, 150);
       wl_surface_commit (subsurfaces[8]->surface);
 
       /* Nothing should appear.  */
@@ -811,6 +836,88 @@ test_single_step (enum test_kind kind)
 	 run.  */
       sleep (1);
       sleep_or_verify ();
+
+      /* Reset the scale.  */
+      test_set_scale (display, 1);
+      wait_frame_callback (wayland_surface);
+
+      /* Sleep for 1 second to wait for the scale hooks to completely
+	 run.  */
+      sleep (1);
+
+      test_single_step (SUBSURFACE_REPARENT_KIND);
+      break;
+
+    case SUBSURFACE_REPARENT_KIND:
+      subsurface_stack_1_png
+	= load_png_image (display, "subsurface_stack_1.png");
+
+      if (!subsurface_stack_1_png)
+	report_test_failure ("failed to load subsurface_stack_1.png");
+
+      subsurface_stack_2_png
+	= load_png_image (display, "subsurface_stack_2.png");
+
+      if (!subsurface_stack_2_png)
+	report_test_failure ("failed to load subsurface_stack_2.png");
+
+      /* Delete subsurfaces[6].  Verify that after doing so,
+	 subsurfaces[7] and subsurfaces[8] disappear.  */
+      delete_subsurface_role (subsurfaces[6]);
+      wait_frame_callback (wayland_surface);
+      sleep_or_verify ();
+
+      /* Next, recreate the subsurface.  The children should not
+	 become visible, as subsurfaces[4] was not committed.  */
+      recreate_subsurface (subsurfaces[6],
+			   subsurfaces[4]->surface);
+      wait_frame_callback (wayland_surface);
+      sleep_or_verify ();
+
+      /* Finally, create two new subsurfaces.  Both are children of
+	 subsurfaces[6].  The first, subsurfaces[9] has
+	 subsurface_stack_1 applied, and is placed at 600, 600.  */
+      subsurfaces[9]
+	= make_test_subsurface_with_parent (subsurfaces[6]);
+
+      if (!subsurfaces[9])
+	report_test_failure ("failed to create subsurface");
+
+      wl_surface_attach (subsurfaces[9]->surface, subsurface_stack_1_png,
+			 0, 0);
+      wl_surface_damage (subsurfaces[9]->surface, 0, 0, 100, 100);
+      wl_surface_commit (subsurfaces[9]->surface);
+      wl_subsurface_set_position (subsurfaces[9]->subsurface, 600, 600);
+
+      /* The second, subsurfaces[10] is a child of subsurfaces[6].  It
+	 is also placed at 600, 600, on top of subsurfaces[9].  */
+      subsurfaces[10]
+	= make_test_subsurface_with_parent (subsurfaces[6]);
+
+      if (!subsurfaces[10])
+	report_test_failure ("failed to create subsurface");
+
+      wl_surface_attach (subsurfaces[10]->surface, subsurface_stack_2_png,
+			 0, 0);
+      wl_surface_damage (subsurfaces[10]->surface, 0, 0, 100, 100);
+      wl_surface_commit (subsurfaces[10]->surface);
+      wl_subsurface_set_position (subsurfaces[10]->subsurface, 600, 600);
+
+      /* Now, commit subsurfaces[6].  Nothing should become
+	 visible.  */
+      wl_surface_commit (subsurfaces[6]->surface);
+      wait_frame_callback (wayland_surface);
+      sleep_or_verify ();
+
+      /* Finally, commit subsurfaces[4] and then subsurfaces[0].
+	 Everything should now show up: a completely white W on top of
+	 a translucent red cube, partly obscuring small.png, at 600,
+	 600.  */
+      wl_surface_commit (subsurfaces[4]->surface);
+      wait_frame_callback (subsurfaces[0]->surface);
+      sleep_or_verify ();
+
+      break;
     }
 
   if (kind == LAST_TEST)

tests/test_harness.c

@@ -570,6 +570,8 @@ load_png_image (struct test_display *display, const char *filename)
   /* Upload the image data.  */
   buffer = upload_image_data (display, (const char *) image_data,
 			      width, height, depth);
+  test_manager_set_buffer_label (display->test_manager, buffer,
+				 filename);
 
   /* Free the image data.  */
   free (image_data);